Re: [PAE support] patch review

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

On 17.07.2010 16:01, Paul Goyette wrote:
> On Sat, 17 Jul 2010, Jean-Yves Migeon wrote:
> 
>> - introduce i386_cpu_switch_pmap(), used to switch pmap for the curcpu.
>> Due to the different handling of pmap mappings with PAE vs !PAE, Xen vs
>> native, I hid the details within this function. This helps calling it
>> from assembly, as some features, like BIOS calls, switch to pmap_kernel
>> before mapping trampoline code in low memory.
>>
>> WARNING: this function currently *breaks* the amd64 build (obviously,
>> there is no i386_cpu_switch_pmap() for amd64).
>>
>> ==> this part will be re-written with a cleaner abstraction, in sync
>> with rmind and his uvmplock branch. But I would like you to review the
>> rest of the patch.
> 
> You will fix the breakage before committing, right?

Yes, obviously. This review is just to gather opinions on the rest of
the patch, but I had to include this function to make the bios/kvm86
stuff "understandable."

> With this impact, should this discussion also include the port-amd64 list?

I have to discuss it with rmind privately first, so we can go in the
same direction. He already made improvements in this regard in his
branch, maybe I can take some inspiration from it. When we agree, he (or
me) will post to x86 lists (i386, xen and amd64).

>> Two questions:
>> - is it ok for everyone to add PAE to ALL kernel? This can raise paddr_t
>> issues within drivers, as well as some format string errors.
> 
> I think you should add it - better to find the issues now, and get them
> resolved, rather than finding them later.
> 
>>
>> - should I add a "#options PAE" within GENERIC config file, or provide
>> additional GENERIC_PAE/MONOLITHIC_PAE files?
> 
> I would prefer not to create more config files.  A
> 
>     #options PAE
> 
> should be fine.

Thanks!

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