Re: CVS commit: src/sys/arch

["Cherry G. Mathew" <cherry.g.mathew%gmail.com@localhost>]

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 ?

>>> - 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