Subject: Re: port-mips/31915: provide centralized wired_map logic
To: None <tsutsui@netbsd.org, gnats-admin@netbsd.org,>
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
List: netbsd-bugs
Date: 11/02/2005 14:13:02
The following reply was made to PR port-mips/31915; it has been noted by GNATS.

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: garrett_damore@tadpole.com
Cc: soda@sra.co.jp, gnats-bugs@NetBSD.org, port-mips@NetBSD.org,
	tsutsui@ceres.dti.ne.jp
Subject: Re: port-mips/31915: provide centralized wired_map logic
Date: Wed, 2 Nov 2005 23:12:50 +0900

 In article <4368110F.5020905@tadpole.com>
 garrett_damore@tadpole.com wrote:
 
 > To set up a full TLB the MD code will have to call this routine once 
 > each for PA0 and PA1.
 
 Ok, it seems fine.
 
 > +++ sys/arch/mips/mips/mips3_wired_map.c	2 Nov 2005 01:06:10 -0000
 
 As I wrote before, this can be arch/mips/mips/wired_map.c.
 I'll rename arc/wired_map.c to wired_map_machdep.c or something.
 
 > +#include <mips/wired_map.h>
 
 Maybe we should have <machine/wired_map.h> for MD definitions,
 which includes <mips/wired_map.h> like <machine/vmparam.h>.
 
 > +static struct wired_map_entry {
 > +	paddr_t	pa0;
 > +	paddr_t	pa1;
 > +	vaddr_t	va;
 > +	vsize_t	pgmask;
 > +} wired_map[MIPS3_WIRED_ENTRIES];
 
 This struct wired_map_entry and wired_map[] variable should be
 exported for MD functions.
 
 > +static boolean_t mips3_wire_down_page(vaddr_t va, paddr_t pa);
 
 This should be removed (exported).
 
 > +static int	nwired;
 
 nwired also should be exported, though maybe it's better
 to rename it something like mips3_nwired_page.
 
 > +boolean_t
 > +mips3_wire_down_page(vaddr_t va, paddr_t pa, vsize_t pgsz)
 
 IMHO, "wire_down" seems a bit strange for this operation.
 It reminds me "pin down" memory (RAM) to avoid pageout.
 How about "mips3_wired_enter_page()" like pmap_enter(9)?
 
 > +	if ((va & (pgsz - 1)) || (pa & (pgsz - 1)))
 > +		panic("mips3_wire_down_page: not aligned");
 
 This check could be wrapped by #ifdef DIAGNOSTIC.
 
 > +			return 0;
  :
 > +	return 1;
 
 It's better to use TRUE and FALSE rather than 1 and 0?
 
 > +boolean_t
 > +mips3_wire_down(vaddr_t va, paddr_t pa, vsize_t size)
 
 How about mips3_wired_enter() for this?
 
 > Index: sys/arch/mips/include/wired_map.h
  :
 > +/*
 > + * Certain machines have peripheral busses which are only accessible
 > + * using the TLB.
  :
 > + * Note also that at the moment this is not supported on the MIPS-I
 > + * ISA (but it shouldn't need it anyway.)
 > + */
 
 Isn't it better to move these comments to (mips3_)wired_map.c?
 
 > +#define	MIPS3_SIZE_TO_PG_MASK(x)	(((x * 2) - 1) & ~0x1fff)
 
 This is also defined in <mips/mips3_pte.h> later?
 
 > +#ifndef	MIPS3_WIRED_ENTRIES
 > +#define MIPS3_WIRED_ENTRIES	8	/* upper limit */
 > +#endif	/* MIPS3_WIRED_ENTRIES */
 
 How about "MIPS3_NWIRED_ENTRY" like VM_NFREELIST in vmparam.h?
 
 > +/*
 > + * Wire down a mapping from a virtual to physical address.  The size
 > + * of the region must be a multiple of MIPS3_WIRED_ENTRY_SIZE, with
 > + * matching alignment.
 > + */
 
 > +/*
 > + * Lower layer API, to supply an explicit page size.  It only wires a
 > + * single page at a time.
 > + */
 
 These function descriptions should be in (mips3_)wired_map.c.
 
 
 Anyway, I'll try to adapt arc port to your patch shortly. Thanks!
 ---
 Izumi Tsutsui