[RFC, PATCH 5/5] Paravirt_ops export.patch

[RFC, PATCH 5/5] Paravirt_ops export.patch

[Zachary Amsden]

Now that paravirt-ops is a fully patched, purely functional interface,
which appropriately generates bugs when patches fail to get applied,
it need no longer be exported to modules.  Modules will be patched at
load time, and need no boot time fallback which references paravirt-ops.

Now, all traditional functionality which was inlined into modules before
is still available to modules with paravirt_ops, but there is no danger
of exporting it to rogue or non-GPL modules.

Signed-off-by: Zachary Amsden <zach@vmware.com>

diff -r 80ddc95c2ab2 arch/i386/kernel/paravirt.c
--- a/arch/i386/kernel/paravirt.c	Thu Apr 19 16:17:36 2007 -0700
+++ b/arch/i386/kernel/paravirt.c	Thu Apr 19 16:22:25 2007 -0700
@@ -353,11 +353,3 @@ struct paravirt_ops paravirt_ops = {
 
 	.startup_ipi_hook = paravirt_nop,
 };
-
-/*
- * NOTE: CONFIG_PARAVIRT is experimental and the paravirt_ops
- * semantics are subject to change. Hence we only do this
- * internal-only export of this, until it gets sorted out and
- * all lowlevel CPU ops used by modules are separately exported.
- */
-EXPORT_SYMBOL_GPL(paravirt_ops);
diff -r 80ddc95c2ab2 include/asm-i386/paravirt.h
--- a/include/asm-i386/paravirt.h	Thu Apr 19 16:17:36 2007 -0700
+++ b/include/asm-i386/paravirt.h	Thu Apr 19 16:29:56 2007 -0700
@@ -266,8 +266,16 @@ unsigned paravirt_patch_insns(void *site
  * The type number, computed in PARAVIRT_PATCH, is derived from the
  * offset into the paravirt_ops structure, and can therefore be freely
  * converted back into a structure offset.
+ *
+ * For modules, generate a bogus BUG, which leaves enough space for
+ * the patch to a direct call to be made and eliminates the need to
+ * export paravirt_ops to modules.
  */
+#ifdef MODULE
+#define PARAVIRT_CALL	"ud2a; nop; nop; nop;"
+#else
 #define PARAVIRT_CALL	"call *(paravirt_ops+%c[paravirt_typenum]*4);"
+#endif
 
 /*
  * These macros are intended to wrap calls into a paravirt_ops

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Now that paravirt-ops is a fully patched, purely functional interface,
> which appropriately generates bugs when patches fail to get applied,
> it need no longer be exported to modules.  Modules will be patched at
> load time, and need no boot time fallback which references paravirt-ops.
>   

Hm, OK, I see what you're getting at.

I guess the BUG_ON()s need only fire if they're failing to patch over a
ud2a instruction.


I'm in two minds about this.  On the one hand, it's a clever hack which
does achieve the desired outcome.  On the other hand, it turns an
optimisation mechanism into a special-purpose ad-hoc linker which
doesn't seem like quite the right way to go.  The conversion from "best
effort, doing nothing is OK" to "doing nothing is a BUG" behaviour is a
good indication of this.

    J

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Eric W. Biederman]

Jeremy Fitzhardinge <jeremy@goop.org> writes:

> Hm, OK, I see what you're getting at.
>
> I guess the BUG_ON()s need only fire if they're failing to patch over a
> ud2a instruction.
>
>
> I'm in two minds about this.  On the one hand, it's a clever hack which
> does achieve the desired outcome.  On the other hand, it turns an
> optimisation mechanism into a special-purpose ad-hoc linker which
> doesn't seem like quite the right way to go.  The conversion from "best
> effort, doing nothing is OK" to "doing nothing is a BUG" behaviour is a
> good indication of this.

Yes.  Has anyone thought more about David Miller's suggesting of just
using the linker and not doing the fancy binary replacement?

Especially if you are beginning to reimplement the linker anyway.

Eric

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Jeremy Fitzhardinge]

Eric W. Biederman wrote:
> Yes.  Has anyone thought more about David Miller's suggesting of just
> using the linker and not doing the fancy binary replacement?
>
> Especially if you are beginning to reimplement the linker anyway.

I did spend some effort on it.  The first stumbling block was getting
the reloc info into the vmlinux itself, rather than putting it in the
bzImage wrapper.  It was getting into a complex kallsyms-like
arrangement, which I wasn't feeling equipped to solve.

But one of the objections to making paravirt_ops calls where there would
otherwise be small inline functions is that the calls significantly add
to the register pressure and therefore would result in worse code.  This
is particularly acute in the assember parts, since there are places in
entry.S where paravirt operations need to be performed, but there are no
available registers.

We currently deal with that by including push/pops around the actual
indirect callsite, which the patcher can either choose to overwrite (if
the inserted sequence doesn't clobber registers anyway), or replace with
a set which is appropriate for the callsite and the inserted code.

Also, in many cases we can usefully patch inline code and eliminate the
need for any call at all.  To do this, we need to know 1) the start and
size of the replacable code, 2) the register environment at that site,
and 3) what to actually put there.

The reloc info only gives us 3 (by looking at the call/jmp instruction's
target), and 1 to a limited extent (we get 5 bytes). Unfortunately, 5
bytes is too small for anything except patching in native versions, and
the surrounding generated code will still assume the patched code will
clobber all the normal C caller-save registers.

    J

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Zachary Amsden]

[-- Attachment #1.1: Type: text/plain, Size: 1931 bytes --]

On 4/22/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Jeremy Fitzhardinge <jeremy@goop.org> writes:
>
> > Hm, OK, I see what you're getting at.
> >
> > I guess the BUG_ON()s need only fire if they're failing to patch over a
> > ud2a instruction.
> >
> >
> > I'm in two minds about this.  On the one hand, it's a clever hack which
> > does achieve the desired outcome.  On the other hand, it turns an
> > optimisation mechanism into a special-purpose ad-hoc linker which
> > doesn't seem like quite the right way to go.  The conversion from "best
> > effort, doing nothing is OK" to "doing nothing is a BUG" behaviour is a
> > good indication of this.



Doing nothing is a BUG, even before this change.  If you can't patch in a
properly virtualizable substitute for a non-virtualizable sequence, the
kernel will not work.  The only way for the patching to fail is for lack of
space or failure to meet clobber constraints, both of which would be fatal
even without the patching.


Yes.  Has anyone thought more about David Miller's suggesting of just
> using the linker and not doing the fancy binary replacement?



I thought a lot about it and ended up concluding it was not tenable.  I
can't recall quite the logic that got me there now, but I do know that two
things immediately spring to mind:

1) Using the linker, all paravirt-ops, even performance critical ones,
become function calls, with 3 clobbered registers.  Thwarting this by using
non-C  function calls brings you directly back to the fancy inline assembly
goo you are trying to avoid.
2) You must support dynamic re-linking - the kernel has to boot and use
builtin native style operations before switching over to the virtualized
operations.  So you have to have some kind of jettisonable early binding
support.

Considering the performance element will be explored fully, I'm not sure
that using the linker results in any reduction in complexity.

Zach

[-- Attachment #1.2: Type: text/html, Size: 2632 bytes --]

[-- Attachment #2: Type: text/plain, Size: 184 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Doing nothing is a BUG, even before this change.  If you can't patch
> in a properly virtualizable substitute for a non-virtualizable
> sequence, the kernel will not work.  The only way for the patching to
> fail is for lack of space or failure to meet clobber constraints, both
> of which would be fatal even without the patching.

Could you give a specific example?  Because the intent is that if you do
nothing (ie, don't apply patching at all), then you'll just end up with
indirect calls which will be a bit expensive but completely functional. 
The idea is that every time there's a paravirt call, it must be
surrounded with push/pops to make the C call compatible with the
callsite's register usage.  Or are you talking about something else?

> 2) You must support dynamic re-linking - the kernel has to boot and
> use builtin native style operations before switching over to the
> virtualized operations.  So you have to have some kind of jettisonable
> early binding support.

I don't think there's any particular reason we can't do this very early,
at the same time we currently populate paravirt_ops.  I think the idea
is that if you do nothing, the calls will all point to the native
versions, so if you do the late-paravirtualization (do you still do
that?) then you'll get native ops initially.

    J

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Doing nothing is a BUG, even before this change.  If you can't patch
>> in a properly virtualizable substitute for a non-virtualizable
>> sequence, the kernel will not work.  The only way for the patching to
>> fail is for lack of space or failure to meet clobber constraints, both
>> of which would be fatal even without the patching.
>>     
>
> Could you give a specific example?  Because the intent is that if you do
> nothing (ie, don't apply patching at all), then you'll just end up with
> indirect calls which will be a bit expensive but completely functional. 
> The idea is that every time there's a paravirt call, it must be
> surrounded with push/pops to make the C call compatible with the
> callsite's register usage.  Or are you talking about something else?
>   

There are two specific examples, both of which are real BUGs.  If there 
is not enough space to do generic patching to a direct function call, 
then it is a bug, since there would not have even been enough space to 
do an indirect call in the first place.  Obviously, with the i386 ISA, 
this is impossible as long as there was an indirect in the first place.  
But an attempt to place native "cli" inline with no padding, to use as a 
patch point, would fire this BUG and rightly so.

The second BUG I added is also justified.  If the patch point has a 
smaller than normal clobber list, then the original indirect call is 
incorrect unless the code has taken care to save and restore clobbered 
registers around the patch point.  That is the case today, but there is 
nothing validating that in the patching code.

We could make the patching code more intelligent and have it emit the 
proper push / pop sequence around the call, but it is difficult to do 
and does not work for more than 3 argument function calls.  Or we could 
record the EIP of the call instruction in the patch information, 
allowing the patcher to either overwrite the whole thing, or do indirect 
/ direct conversion.

asm volatile("
patch_begin:
    push %edx
    push %ecx
 patch_call:
    call *(paravirt_ops+op*4)
   pop %ecx
   pop %edx
patch_end:
.section .paravirt_patch
.long patch_begin
.long patch_end
.long patch_call
.byte CLBR_ANY
" :: "eax");

Maybe that is too complicated for just this special case.

>> 2) You must support dynamic re-linking - the kernel has to boot and
>> use builtin native style operations before switching over to the
>> virtualized operations.  So you have to have some kind of jettisonable
>> early binding support.
>>     
>
> I don't think there's any particular reason we can't do this very early,
> at the same time we currently populate paravirt_ops.  I think the idea
> is that if you do nothing, the calls will all point to the native
> versions, so if you do the late-paravirtualization (do you still do
> that?) then you'll get native ops initially.
>   

Yes, we still do late paravirtualization.  My point is that because of 
that, you can't just link once and be done with it - you must relink the 
paravirt-ops later, which is more complex than a one time pre-execution 
link.

Zach

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Eric W. Biederman]

Zachary Amsden <zach@vmware.com> writes:

>
>>> 2) You must support dynamic re-linking - the kernel has to boot and
>>> use builtin native style operations before switching over to the
>>> virtualized operations.  So you have to have some kind of jettisonable
>>> early binding support.
>>>
>>
>> I don't think there's any particular reason we can't do this very early,
>> at the same time we currently populate paravirt_ops.  I think the idea
>> is that if you do nothing, the calls will all point to the native
>> versions, so if you do the late-paravirtualization (do you still do
>> that?) then you'll get native ops initially.
>>
>
> Yes, we still do late paravirtualization.  My point is that because of that, you
> can't just link once and be done with it - you must relink the paravirt-ops
> later, which is more complex than a one time pre-execution link.

Then if it is causing problems and making the code more complex (and it sounds
like it is) please let's get rid of late paravirtualization.

arch/i386 is in bad enough shape with tons of unnecessary special cases.
We don't need the paravirtualization support adding more.

Eric

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Zachary Amsden]

Eric W. Biederman wrote:
>> Yes, we still do late paravirtualization.  My point is that because of that, you
>> can't just link once and be done with it - you must relink the paravirt-ops
>> later, which is more complex than a one time pre-execution link.
>>     
>
> Then if it is causing problems and making the code more complex (and it sounds
> like it is) please let's get rid of late paravirtualization.
>
> arch/i386 is in bad enough shape with tons of unnecessary special cases.
> We don't need the paravirtualization support adding more.
>   

Late paravirtualization is not causing problems and making the code more 
complex, in fact it is making it simpler.  The fact that we don't need 
to intrude in the boot process at all, but can let the code run without 
inhibition and 100's of special hooks is saving a lot of complexity.  
Otherwise, we would need to dissect head.S even further with custom junk 
for VMI, including probing for ROMs and other things that are just 
naturally easier once you have basic kernel architecture like physical 
mapping, PAE-ness and memory management in place.  Paravirtualizing the 
init code to set both non-PAE and PAE ptes from non-PAE mode (see 
boot_ioremap.c) would be one such grotesque and unwarranted violation.

My point is that if we were to use a linker type approach, it needs to 
be done before the first instruction of the kernel (in particular, 
before the first cpuid ever gets run); this makes it tempting to do in 
some weird assembly goo in head.S, instead of doing it in proper C code 
that can be reused later on.  Doing it in C would be better coding 
practice, it just requires more effort to avoid taking shortcuts (to 
work around the non-C starting environment).

Zach

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> There are two specific examples, both of which are real BUGs.  If
> there is not enough space to do generic patching to a direct function
> call, then it is a bug, since there would not have even been enough
> space to do an indirect call in the first place.  Obviously, with the
> i386 ISA, this is impossible as long as there was an indirect in the
> first place.  But an attempt to place native "cli" inline with no
> padding, to use as a patch point, would fire this BUG and rightly so.

Well, the BUG is if the patch-size is smaller than sizeof(indirect
call).  Or more generally, if the patch site contains bogus crud.  But I
don't think checking for that in the patcher makes a great deal of sense.

> The second BUG I added is also justified.  If the patch point has a
> smaller than normal clobber list, then the original indirect call is
> incorrect unless the code has taken care to save and restore clobbered
> registers around the patch point.  That is the case today, but there
> is nothing validating that in the patching code.

Well, yes, it assumes that the original unpatched code is OK, and makes
sure that it remains correct after patching.  The patcher could
disassemble the unpatched sequence to make sure its consistent with the
site and clobbers, but that seems like overkill.


Currently the invariants on pv_ops patch sites are initially:

   1. the sequence is always at least an indirect call to an
      implementation of the operations, and
   2. it will always have appropriate register save/restores which allow
      a call to C code to work at that site

As a result, it is always OK to execute without doing any patching.

The callsite clobbers may be smaller than those needed for a naked C
call, but that means the site will have push/pops around the indirect
call.  This gives the patcher more space to work with if it wants to
inline something; if it just wants to generate a call to C code, it must
also generate its own register saves.

The indirect->direct call converter does check the clobbers, and makes
sure that inserting a naked direct call is OK.

> We could make the patching code more intelligent and have it emit the
> proper push / pop sequence around the call, but it is difficult to do
> and does not work for more than 3 argument function calls.  Or we
> could record the EIP of the call instruction in the patch information,
> allowing the patcher to either overwrite the whole thing, or do
> indirect / direct conversion.

Could do.  It would be useful to know how many args the function has,
and then the indirect->direct converter could generate the appropriate
push/pops for itself.  We're currently using 16 bits for the clobbers
field, which is overkill, we could use one byte as the arg count.  Just
an "args on stack" flag would be sufficient.

> Yes, we still do late paravirtualization.  My point is that because of
> that, you can't just link once and be done with it - you must relink
> the paravirt-ops later, which is more complex than a one time
> pre-execution link.

It's semantically no different from updating the paravirt_ops structure
followed by patching.  If the kernel comes up with all the sites
prelinked to the native versions, then its the same as booting on the
current kernel, with paravirt_ops initialized to the native versions.

    J

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
>
> Well, the BUG is if the patch-size is smaller than sizeof(indirect
> call).  Or more generally, if the patch site contains bogus crud.  But I
> don't think checking for that in the patcher makes a great deal of sense.
>   

True, it may not make a lot of sense to check there.  But it is the only 
place where we can check it at runtime.  Unless you want some kind of 
compile time checking in the code which generates the patch data, say 
perhaps a .abort.  Still, that is not safe against modules which have 
broken paravirt-ops embedding.

> Currently the invariants on pv_ops patch sites are initially:
>
>    1. the sequence is always at least an indirect call to an
>       implementation of the operations, and
>    2. it will always have appropriate register save/restores which allow
>       a call to C code to work at that site
>
> As a result, it is always OK to execute without doing any patching.
>
> The callsite clobbers may be smaller than those needed for a naked C
> call, but that means the site will have push/pops around the indirect
> call.  This gives the patcher more space to work with if it wants to
> inline something; if it just wants to generate a call to C code, it must
> also generate its own register saves.
>   

Since there are so few of these sites, and they are performance 
critical, the placement of a BUG() here verifying the clobbers are ok 
effectively makes it the paravirt-op backend's responsibility to ensure 
that these sites get patched.  This is what would allow the removal of 
dependence on the paravirt_ops symbol for the most important interrupt 
control operations.

> It's semantically no different from updating the paravirt_ops structure
> followed by patching.  If the kernel comes up with all the sites
> prelinked to the native versions, then its the same as booting on the
> current kernel, with paravirt_ops initialized to the native versions.
>   

It's semantically the same, but the code needs to be re-usable later, 
which is where the problems arise - you can't simply cram the prelink 
code into head.S, but have to write it to be properly callable later on.

Zach

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Jeremy Fitzhardinge wrote:
>>
>> Well, the BUG is if the patch-size is smaller than sizeof(indirect
>> call).  Or more generally, if the patch site contains bogus crud.  But I
>> don't think checking for that in the patcher makes a great deal of
>> sense.
>>   
>
> True, it may not make a lot of sense to check there.  But it is the
> only place where we can check it at runtime.  Unless you want some
> kind of compile time checking in the code which generates the patch
> data, say perhaps a .abort.  Still, that is not safe against modules
> which have broken paravirt-ops embedding.

Well, they shouldn't be doing it for themselves; it will always be as a
result of some macro expansion from asm/paravirt.h.  And if they're
broken in a module, they'll be broken in the kernel too.

And you should be able to check for patch-site sanity by booting on
native with patching disabled.  That should fall over pretty quickly if
there's something wrong.

> Since there are so few of these sites, and they are performance
> critical, the placement of a BUG() here verifying the clobbers are ok
> effectively makes it the paravirt-op backend's responsibility to
> ensure that these sites get patched.  This is what would allow the
> removal of dependence on the paravirt_ops symbol for the most
> important interrupt control operations.

I'm not keen on *requiring* patching to occur, in general.  The
performance without patching is only a few percent worse than
native/patched, so its not like its a desperate problem to defer
implementing it.  (I.e, requiring patching just raises the
implementation barrier for a pv_ops backend.)

Overloading patching for dealing with module exports is interesting, but
well, I guess I don't see the problem in just exporting paravirt_ops. 
The two arguments I've seen against it are "security" and "GPL issues",
but neither seems particularly good.

> It's semantically the same, but the code needs to be re-usable later,
> which is where the problems arise - you can't simply cram the prelink
> code into head.S, but have to write it to be properly callable later on. 

I think that's OK.  It can just be plain C code, with very few
infrastructure dependencies.  It wouldn't need to allocate memory, for
example.  It should be fairly simple to make the code callable either
from early in the kernel's life or later on.

    J

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> I'm not keen on *requiring* patching to occur, in general.  The
> performance without patching is only a few percent worse than
> native/patched, so its not like its a desperate problem to defer
> implementing it.  (I.e, requiring patching just raises the
> implementation barrier for a pv_ops backend.)
>   

Well, this approach could be used, but it is overloading a bit and does 
make some extra burden on the backends.  Can't argue that.

> Overloading patching for dealing with module exports is interesting, but
> well, I guess I don't see the problem in just exporting paravirt_ops. 
> The two arguments I've seen against it are "security" and "GPL issues",
> but neither seems particularly good.
>   

Yes, I agree on that.  The other argument was "interface flux".

So, Rusty, back to splitting exports or just EXPORT_SYMBOL the thing?

Zach

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Andi Kleen]

> So, Rusty, back to splitting exports or just EXPORT_SYMBOL the thing?

I already exported it here, but I would prefer if it was split too.

-Andi

Re: [RFC, PATCH 5/5] Paravirt_ops export.patch

[Rusty Russell]

On Sun, 2007-04-22 at 08:28 -0600, Eric W. Biederman wrote:
> Yes.  Has anyone thought more about David Miller's suggesting of just
> using the linker and not doing the fancy binary replacement?
> 
> Especially if you are beginning to reimplement the linker anyway.

Nope.  It doesn't do what we need.  See discussion which followed it.

Rusty.