Paravirt_ops export.patch

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.

diff -r 80ddc95c2ab2 arch/i386/kernel/paravirt.c
@@ -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
@@ -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

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

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

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

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

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

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization

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?

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

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.

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

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

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

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.

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.

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.

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

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.

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

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.

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.

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

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.

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

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

Zach

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

-Andi

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

Rusty.