Re: CVS commit: src/sys/arch

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

On 08.11.2011 14:53, Cherry G. Mathew wrote:
> On 8 November 2011 15:20, Jean-Yves Migeon<jeanyves.migeon%free.fr@localhost>
wrote:
> > 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.
>
> hmm... I wonder if it would be "cleaner" to do this within a  #ifdef
> XEN/#endif ?

The cleanest way would be to share the code between x86 and Xen, keep
the allocation "below 4GiB" boundary for both, and use it everywhere in
the pmap code. Only the manipulation of the vcpu_guest_context_t
ctrlregs members would have to force this use.

Avoiding the use of macros is a big plus; it helps modularity.

> > > > - are you sure that these have to be "defined(PAE) ||
> > > > defined(__x86_64__)" ?
>
> That's a crude way of making pmap_cpu_init_late() do nothing for
> i386 (non-pae) since i386 doesn't need shadow PMDs.

In that case, test against "i386_use_pae" (0 == !PAE, 1 == PAE), and
simply return if !PAE. Avoid having loooooonnng macro blocks with
different levels of #ifdef. It's fairly difficult to untangle and
unpleasant to read.

Macros should only be used to fix compile-time issues (like: symbol
missing), or help code readability by reuse. Not for jumping around C
code and take shortcuts. This has to be done at execution time rather
than compile time.

> > > > - 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 :)
>
> ok, I'll take flak for further breakage ;-P

Expect stress testing for MP in the near future :o

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