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 ? >>> - 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. >>> - 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 Cheers, -- ~Cherry