Re: CVS commit: src/sys/arch

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

Hi,

On 9 November 2011 02:02, Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost> 
wrote:
> 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.
>

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