Re: CVS commit: src/sys/arch

[Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost>]

On Tue, 8 Nov 2011 08:49:22 +0530, Cherry G. Mathew wrote:
> > Please keep ci_pae_l3_pdir to a uint32_t and back out its paddr_t 
> > type.
> >
> > Unless you can convince me that your code will do the right 
> > thing(tm), the
> > PDP for PAE should to be allocated below the 4GiB physical boundary; 
> > unless
> > mistaken, the uvm_km_alloc(kernel_map, ...) does not enforce that. 
> > Besides,
> > %cr3 is still a 32 bits entity under PAE. Trying to stash a 64 bits 
> > paddr_t
> > in %cr3 is a really bad idea.
> >
> > See comments 
> > http://nxr.netbsd.org/xref/src/sys/arch/x86/x86/pmap.c#1588
>
> This doesn't hold true for Xen. See:
> http://lxr.xensource.com/lxr/source/xen/arch/x86/mm.c#L509
>
> Xen does the <4GB translation for the VM by maintaining its own
> shadow. I just took the easier route ( after initially using the x86
> pmap < 4GB cr3 code ), but if you're thinking of a unified a
> xen/non-xen implementation we'll have to re-think this one.

Okay; then all users of cr3 have to be documented (_especially_ the 
native x86 code), as there at least two places where there's implicit 
uint64_t => uint32_t truncation: lcr3() and pcb_cr3. A comment above 
these should be enough. Same goes for having paddr_t for L3 PD in struct 
cpu_info.

> > - are you sure that these have to be
> > "defined(PAE) || defined(__x86_64__)" ?
> >
> > - please use "for (i = 0; i < PTP_LEVELS - 1; i++ ) { ... }". I will 
> > have to
> > identify these blocks later when GENERIC will include both PAE and 
> > !PAE
> > pmaps. PTP_LEVELS makes it a lot easier.
>
> ok, will watchout for it - do you want to do this one yourself ?

Please do :)

--
Jean-Yves Migeon
jeanyves.migeon%free.fr@localhost