On Fri, May 03, 2002 at 08:23:12AM -0700, Jason R Thorpe wrote:

 > I will send another message shortly with my specific comments.

UCHIYAMA-san...

Ok, sorry for the delay, but here are my comments on the new pmap module.

Global comments:

	* I notice there isn't any locking going on in here.  This is
	  probably OK for all existing SuperH systems, but it might
	  be nice to add it later, if only for debugging purposes when
	  LOCKDEBUG is defined.

	* You are using splvm() an awful lot.  It should not really
	  be necessary in very many parts of the pmap module.

pmap_create()

	* Since you are only using 512 slots out of a 1024 slot table,
	  you might want to consider using a pool for the L1 table,
	  which would allow you to using one page for every 2 pmaps,
	  rather than 1 page for each pmap.  Saving memory on SuperH
	  systems is good, since they tend not to have very much :-)

pmap_destroy()

	* See above about the L1 table.

	* Since you are throwing away the data for the pages you
	  are freeing, you might consider changing the wbinv_range
	  to just inv_range, so that it's faster if you're using
	  the cache in write-back mode.

pmap_enter():

	* You should be using the "flags" argument to seed the
	  REFERENCED and MODIFIED bits in the mdpage.  I.e. something
	  like this:

#ifdef DIAGNOSTIC
		if ((flags & VM_PROT_ALL) & ~prot)
			panic("pmap_enter: access type exceeds prot");
#endif
		if (flags & VM_PROT_WRITE)
			pg->mdpage.pvh_flags |= PVH_MODIFIED|PVH_REFERENCED;
		else if (flags & VM_PROT_ALL)
			pg->mdpage.pvh_flags |= PVH_REFRENCED;

		.
		.
		.

	  ...and then use the combination of the prot and the current
	  pvh_flags to set the protection in the PTE.

	  Note that you must also do modified/referenced emulation for
	  the kernel pmap -- the "flags" argument will always specify
	  the correct pvh_flags seed for wired kernel mappings.

	  Also, in regards to modified emulation, you the algorithm for
	  setting the protection in the PTE needs to be like this:

		if (prot & VM_PROT_WRITE) {
			if (page-is-modified)
				set dirty+valid in the PTE;
			else if (page-is-referenced)
				set valid in the PTE;
		}

	  Likewise when you take the modified emulation fault.  This
	  can reduce the number of faults you have to take.

	* pmap_enter() can be called when there is already a valid mapping
	  at the specified VA.  This means you need to:

		if (already-mapping-at-va) {
			if (old-pa == new-pa) {
				adjust protection if necessary;
				adjust pm_stats.wired_count if necessary;
			} else {
				remove old mapping;// decrements resident_count
				goto enter_it;
			}
		} else {
 enter_it:
			enter new mapping; // increments resident_count
		}

pmap_is_referenced()

	* Return TRUE or FALSE.

__pmap_pte_load():

	* I would change the KDASSERT() to read:

		KDASSERT(((intptr_t) va < 0 && pmap == pmap_kernel()) ||
			 ((intptr_t) va >= 0 && pmap != pmap_kernel()))

	  i.e. remove the "va != 0" bit -- whether or not something is
	  mapped at 0 is a policy decision made by the upper layers of
	  the VM code (unless va 0 is magic, like it is on the ARM).

__pmap_asid_alloc():

	* Would a generational ASID allocator work for the SuperH?  See
	  the Alpha pmap_asn_alloc().

Anyway, terrific work!  I'm looking forward to seeing this committed to
the source tree!

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>