[RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Zachary Amsden]

A clever set of manipulations allows us to fix binary modules with paravirt-ops
by just not exporting paravirt_ops at all and using the new patching code.

What do you think?  Experimental patches here... only partially tested, but
booting and appear to be a working prototype.

Thanks,
Zach

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Andi Kleen]

On Friday 20 April 2007 03:52:14 Zachary Amsden wrote:
> A clever set of manipulations allows us to fix binary modules with paravirt-ops
> by just not exporting paravirt_ops at all and using the new patching code.
> 
> What do you think?  Experimental patches here... only partially tested, but
> booting and appear to be a working prototype.

Basic idea looks good.

But is the 5 argument support really needed? 	I don't see any paravirt 
ops functions that needs it and even if there was one it still wouldn't be clear
if it made sense to export it.

-Andi

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Jeremy Fitzhardinge]

Andi Kleen wrote:
> Basic idea looks good.
>
> But is the 5 argument support really needed? 	I don't see any paravirt 
> ops functions that needs it and even if there was one it still wouldn't be clear
> if it made sense to export it.

64-bit args are passed as a pair of 32-bit args, so PAE set_pte_at*
functions end up with 5 args.  They're used often, so they may be worth
patching, but I'm not sure if they're worth exporting.  They are
implicitly exported in a non-PARAVIRT build by the fact that they're
inline functions/macros in a header.  I don't know if any modules
actually use them.

    J

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Andi Kleen wrote:
>   
>> Basic idea looks good.
>>
>> But is the 5 argument support really needed? 	I don't see any paravirt 
>> ops functions that needs it and even if there was one it still wouldn't be clear
>> if it made sense to export it.
>>     
>
> 64-bit args are passed as a pair of 32-bit args, so PAE set_pte_at*
> functions end up with 5 args.  They're used often, so they may be worth
> patching, but I'm not sure if they're worth exporting.  They are
> implicitly exported in a non-PARAVIRT build by the fact that they're
> inline functions/macros in a header.  I don't know if any modules
> actually use them.
>   

Yes, I don't know either - but we should be consistent about their 
export regardless of whether PAE is selected or not.  So we should 
either have 5 arg macros or not export set_pte_at* at all, even in 4 arg 
version.

Zach

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Yes, I don't know either - but we should be consistent about their
> export regardless of whether PAE is selected or not.  So we should
> either have 5 arg macros or not export set_pte_at* at all, even in 4
> arg version.

So can you separately control exportability independently from patchability?

    J

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Yes, I don't know either - but we should be consistent about their
>> export regardless of whether PAE is selected or not.  So we should
>> either have 5 arg macros or not export set_pte_at* at all, even in 4
>> arg version.
>>     
>
> So can you separately control exportability independently from patchability?
>   

Nope.  But I think we should maintain parity between PAE and non-PAE - 
and probably also maintain parity between non-paravirt-ops and 
paravirt-ops, which means the 5 arg macros are a good idea.  It's not 
like they are very hard to create or maintain.

Zach

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Nope.  But I think we should maintain parity between PAE and non-PAE -
> and probably also maintain parity between non-paravirt-ops and
> paravirt-ops, which means the 5 arg macros are a good idea.  It's not
> like they are very hard to create or maintain. 

Yep.  Did you see the comment about not being able to use "rm" for arg4?

    J

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Nope.  But I think we should maintain parity between PAE and non-PAE -
>> and probably also maintain parity between non-paravirt-ops and
>> paravirt-ops, which means the 5 arg macros are a good idea.  It's not
>> like they are very hard to create or maintain. 
>>     
>
> Yep.  Did you see the comment about not being able to use "rm" for arg4?
>   

Yes, thanks, I fixed that.  The I got lost in info gcc looking for a 
non-stack memory constraint modifier, but it evaded me.

Zach

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Rusty Russell]

On Fri, 2007-04-20 at 14:36 -0700, Zachary Amsden wrote:
> Jeremy Fitzhardinge wrote:
> > Zachary Amsden wrote:
> >   
> >> Yes, I don't know either - but we should be consistent about their
> >> export regardless of whether PAE is selected or not.  So we should
> >> either have 5 arg macros or not export set_pte_at* at all, even in 4
> >> arg version.
> >>     
> >
> > So can you separately control exportability independently from patchability?
> >   
> 
> Nope.  But I think we should maintain parity between PAE and non-PAE - 
> and probably also maintain parity between non-paravirt-ops and 
> paravirt-ops, which means the 5 arg macros are a good idea.  It's not 
> like they are very hard to create or maintain.

Less exports good.  Consistency with all config options isn't a hard
requirement: I'd be tempted not to export the pte functions.

Rusty.

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Andi Kleen]

> Less exports good.  Consistency with all config options isn't a hard
> requirement: I'd be tempted not to export the pte functions.

Yes paravirt_ops should be probably split into two for internal and external
available functions. Any takers? 

-Andi

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Rusty Russell]

On Mon, 2007-04-23 at 01:49 +0200, Andi Kleen wrote:
> > Less exports good.  Consistency with all config options isn't a hard
> > requirement: I'd be tempted not to export the pte functions.
> 
> Yes paravirt_ops should be probably split into two for internal and external
> available functions. Any takers? 

Hi Andi!

	I'm a little uncomfortable with cutting the struct this way: I always
thought it'd be a function split if we did one.

	With the new patch code we could simply shuffle all the exportables to
the front and fail to patch calls past this point (then fail the module
load).

I'll see what I can come up with...
Rusty.

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Andi Kleen]

On Monday 23 April 2007 02:24:05 Rusty Russell wrote:
> On Mon, 2007-04-23 at 01:49 +0200, Andi Kleen wrote:
> > > Less exports good.  Consistency with all config options isn't a hard
> > > requirement: I'd be tempted not to export the pte functions.
> > 
> > Yes paravirt_ops should be probably split into two for internal and external
> > available functions. Any takers? 
> 
> Hi Andi!
> 
> 	I'm a little uncomfortable with cutting the struct this way: I always
> thought it'd be a function split if we did one.

It's a functional split, isn't it? arch/mm internal and "exported" to other
users

> 	With the new patch code we could simply shuffle all the exportables to
> the front and fail to patch calls past this point (then fail the module
> load).

Two structures would seem cleaner to me than such hacks.

-Andi

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Jeremy Fitzhardinge]

Rusty Russell wrote:
> On Mon, 2007-04-23 at 01:49 +0200, Andi Kleen wrote:
>   
>>> Less exports good.  Consistency with all config options isn't a hard
>>> requirement: I'd be tempted not to export the pte functions.
>>>       
>> Yes paravirt_ops should be probably split into two for internal and external
>> available functions. Any takers? 
>>     
>
> Hi Andi!
>
> 	I'm a little uncomfortable with cutting the struct this way: I always
> thought it'd be a function split if we did one.
>
> 	With the new patch code we could simply shuffle all the exportables to
> the front and fail to patch calls past this point (then fail the module
> load).
>
> I'll see what I can come up with...

There was some discussion about hybrid paravirt/fully virt hypervisor
models at the Xen summit.  There are definitely some interesting things
you can do by having a mostly paravirtualized guest running within an
hvm container.

One of the things I was considering as a result was splitting the
privileged instruction ops (all the cr?, msr, segment stuff) into a
cpu_ops, and putting the pagetable stuff into pagetable_ops.  That would
make a broad split of shadow vs direct pagetables and hvm vs non-hvm
models pretty straightforward, and probably sharing more interfaces than
they might otherwise.

    J

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Rusty Russell]

On Mon, 2007-04-23 at 02:49 +0200, Andi Kleen wrote:
> On Monday 23 April 2007 02:24:05 Rusty Russell wrote:
> > On Mon, 2007-04-23 at 01:49 +0200, Andi Kleen wrote:
> > > > Less exports good.  Consistency with all config options isn't a hard
> > > > requirement: I'd be tempted not to export the pte functions.
> > > 
> > > Yes paravirt_ops should be probably split into two for internal and external
> > > available functions. Any takers? 
> > 
> > Hi Andi!
> > 
> > 	I'm a little uncomfortable with cutting the struct this way: I always
> > thought it'd be a function split if we did one.
> 
> It's a functional split, isn't it? arch/mm internal and "exported" to other
> users

Hi,

	When I said functional I was thinking "page table ops" vs "apic ops"
etc.  There's little logic to what needs exporting.

	Most modules only need the interrupt operations.  A small handful want
more, and then some (kvm, lguest) need a whole range of crap (these
should use the native_ versions directly, since nested paravirt is not
supported).

I did the work before; I'll drag it back out and see what the symbols
are again...

Rusty.

Re: [RFC, PATCH 0/5] Paravirt: fix export of paravirt-ops to binary modules

[Zachary Amsden]

Rusty Russell wrote:
> On Mon, 2007-04-23 at 02:49 +0200, Andi Kleen wrote:
>   
>> On Monday 23 April 2007 02:24:05 Rusty Russell wrote:
>>     
>>> On Mon, 2007-04-23 at 01:49 +0200, Andi Kleen wrote:
>>>       
>>>>> Less exports good.  Consistency with all config options isn't a hard
>>>>> requirement: I'd be tempted not to export the pte functions.
>>>>>           
>>>> Yes paravirt_ops should be probably split into two for internal and external
>>>> available functions. Any takers? 
>>>>         
>>> Hi Andi!
>>>
>>> 	I'm a little uncomfortable with cutting the struct this way: I always
>>> thought it'd be a function split if we did one.
>>>       
>> It's a functional split, isn't it? arch/mm internal and "exported" to other
>> users
>>     
>
> Hi,
>
> 	When I said functional I was thinking "page table ops" vs "apic ops"
> etc.  There's little logic to what needs exporting.
>
> 	Most modules only need the interrupt operations.  A small handful want
> more, and then some (kvm, lguest) need a whole range of crap (these
> should use the native_ versions directly, since nested paravirt is not
> supported).
>
> I did the work before; I'll drag it back out and see what the symbols
> are again...
>   

Interrupt control is needed in general.  CPUID is also widely used.
FPU control (clts, read / write cr0) is required for certain software 
operations.

There are non-obvious exports that are required for some modules; in 
particular, MSRs may be used in video drivers, and wbinvd must be 
available to hardware drivers.  We may wish to simply trap / emulate 
these instructions and remove the paravirt-ops altogether.  They are not 
performance critical in 32-bit, and will complicate loading non-GPL 
drivers for a wide range of hardware - hardware that is not exposed in a 
virtualized environment to begin with.

Some rather more interesting modules map kernel page tables to read the 
physical frame numbers from ptes; this is
safe to do if you get_pages or get_user_pages and keep the pages 
locked.  This requires kmap_atomic_pte to be available.  Similarly, 
pte_val and make_pte need to be available.  Mostly only modules that 
require a native environment (virtualization modules) need them, but we 
need some way to either resolve the symbol as load time or make the 
requirement for the symbol go away.  No matter how you look at it, 
failing to load a module because of an unresolved paravirt-op that is 
actually going to be reduced to a nop is a very silly situation to be in.

Zach