Hi, On 9 November 2011 02:02, Jean-Yves Migeon <jeanyves.mig...@free.fr> wrote: > On 08.11.2011 14:53, Cherry G. Mathew wrote: >> >> On 8 November 2011 15:20, Jean-Yves Migeon<jeanyves.mig...@free.fr> > > 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. >
Fair enough. Although the <4G tests would be a bit deceptive (since they're cosmetic) - I guess leaving a note in the code about the rationale behind this will help. >>>>> - 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. > I agree - this looks better. Thanks, -- ~Cherry