Re: [Xen-devel] [RFC v2] xSplice design

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.06.15 at 16:49, <konrad.wilk@xxxxxxxxxx> wrote:
> On Mon, May 18, 2015 at 01:41:34PM +0100, Jan Beulich wrote:
>> >>> On 15.05.15 at 21:44, <konrad.wilk@xxxxxxxxxx> wrote:
>> > As such having the payload in an ELF file is the sensible way. We would be
>> > carrying the various set of structures (and data) in the ELF sections under
>> > different names and with definitions. The prefix for the ELF section name
>> > would always be: *.xsplice_*
>> > 
>> > Note that every structure has padding. This is added so that the hypervisor
>> > can re-use those fields as it sees fit.
>> > 
>> > There are five sections *.xsplice_* sections:
>> 
>> A general remark: Having worked on ELF on different occasions,
>> UNIX'es (and hence Linux'es) use of section names to identify the
>> sections' purposes is pretty contrary to ELF's original intentions.
>> Section types (and architecture as well as OS extensions to them)
>> exist for a reason. Of course from the following sections it's not
>> really clear in how far you intended this to be done. If you did
>> and just didn't explicitly say so, pointing out that relations
>> between sections should then also be expressed suitably via ELF
>> mechanisms (sh_link, sh_info) would then be unnecessary too. If
>> you didn't, then I'd strongly suggest switching to a formally more
>> correct model, which would then include specifying section types
>> and section attributes (and optional other relevant aspects)
>> along with their (then no longer mandatory) names.
> 
> 
> Something like:
> http://docs.oracle.com/cd/E23824_01/html/819-0690/chapter7-1.html 

Yes, the SHT_SUNW_* ones are examples of what I mean.

> With (for example) an pointer to a specific section:
> http://docs.oracle.com/cd/E23824_01/html/819-0690/chapter7-28.html#scrolltoc 
> ?

Not sure what part of that page you mean to refer to.

>> >  * `.bss` section for the new code (function)
>> >  * `.data` and `.data.read_mostly` section for the new and old code 
>> > (function)
>> >  * `.rodata` section for the new and old code (function).
>> > 
>> > In short the *.xsplice_* sections represent various structures and the
>> > ELF provides the mechanism to glue it all together when loaded in memory.
>> 
>> As per above - I think ELF can do this in a more explicit way. Of
>> course that would imply sticking to strictly ELF types in the structure
>> definitions below. That would in turn have the advantage of
>> expressing e.g. "const char *" references to strings by section
>> offsets into a specified string section, reducing (with the ultimate
>> goal of eliminating) the need for processing relocations on the
>> ELF object (possible references to "external symbols" left aside for
>> the moment).
> 
> Do you think that going that way would be too complex for this design?
> Folks have expressed that perhaps I went overboard with this ELF and
> should have just stuck with structures and just mention that it is
> in an ELF file?

I think it should be done either completely ELF-agnostic (with ELF
just happening the container to convey the data) or fully ELF-
compliant.

>> > #define XSPLICE_HOWTO_RELOC_TIME    3 /* __TIME__ type chnage. */  
>> 
>> Assuming the former is DATE - what do you foresee date/time
>> kinds of changes to be good for?
> 
> In case we have source code which uses __DATE__ and the binary patch
> modifies it - we would need to sort out other users of the __DATE__
> and make sure that they are not affected (or perhaps they should be).

And how is this different from arbitrary other string literals?

>> > #define XSPLICE_HOWTO_BUG           4 /* BUG_ON being replaced.*/  
>> > #define XSPLICE_HOWTO_EXTABLE       5 /* exception_table change. */  
>> > #define XSPLICE_HOWTO_SYMBOL        6 /* change in symbol table. */  
>> 
>> For none of these I really understand their purpose.
> 
> This is to help the hypervisor to figure out which of the lookup
> mechanisms to use to find the relevant section. As in if we need
> to update (as part of the patch) a normal function, but also
> the exception table (because the new function is at a new virtual
> address) we want to search in the exception table for the old 
> one and replace it with the new virtual address.

I think this gets too specific. Telling apart code and data may make
sense, but I think beyond that thing should be expressed as
generically as possible. Imagine new special data kinds showing up
- do you envision adding a special type (and hence special handling)
for all of them?

>> > #define XSPLICE_SECTION_STRING 0x00000008 /* Section is in .str */  
>> > #define XSPLICE_SECTION_ALTINSTRUCTIONS 0x00000010 /* Section has 
> .altinstructions. */  
>> 
>> I don't see the purpose of adding flags for a few special sections we
>> may know about now - what if over time hundreds of sections appear?
> 
> We just need to know if it has - and then the hypervisor needs
> to call apply_altinstructions to redo the alternative instructions.
> 
> I presume you are advocating to move this flag to an 'higher' level part
> of the structures?

As above I'd rather see such things not special cased at all. You
already need to deal with the to be patched code having two
(or more) possible variants due to live patching at boot. I think
for the purpose of runtime patching it ought to be quite fine to
use correct, but less optimal instructions (i.e. no consideration of
alternative instructions at all).

>> > #define XSPLICE_SECTION_TEXT_INPLACE 0x00000200 /* Change is in place. */  
>> >  
>> > #dekine XSPLICE_SECTION_MATCH_EXACT 0x00000400 /* Must match exactly. */  
>> > #define XSPLICE_SECTION_NO_STACKCHECK 0x00000800 /* Do not check the 
>> > stack. */  
>> 
>> This isn't the first reference to "stack", without me having seen a
>> description of what the word is meant to stand for in such contexts.
>> Did I overlook it?
> 
> I mentioned it earlier in 'Example of trampoline and in-place splicing':
> 
> 126 However it has severe drawbacks - the safety checks which have to make 
> sure     
> 127 the function is not on the stack - must also check every caller. For some 
> 128 patches this could if there were an sufficient large amount of callers   
> 129 that we would never be able to apply the update.
> 
> But perhaps I should dive much deeper in about the patching
> mechanism having to check whether the function to be patched
> is on the stack.

Yeah - the thing is that the lines you quote are precisely what I
referred to by "this isn't the first reference..." - it's not being
clarified anywhere what "stack" you talk about. In some places
it looked like you meant the stack in memory that the local
variables and spilled registers go onto, but in other places it
wasn't clear whether something else was meant. And with us
supposedly being in stop-machine state, what is on the call
stacks of each CPU should be pretty deterministic.

>> > ### Alternative assembler
>> > 
>> > Alternative assembler is a mechanism to use different instructions 
>> > depending
>> > on what the CPU supports. This is done by providing multiple streams of 
>> > code
>> > that can be patched in - or if the CPU does not support it - padded with
>> > `nop` operations. The alternative assembler macros cause the compiler to
>> > expand the code to place a most generic code in place - emit a special
>> > ELF .section header to tag this location. During run-time the hypervisor
>> > can leave the areas alone or patch them with an better suited opcodes.
>> > 
>> > As we might be patching the alternative assembler sections as well - by
>> > providing a new better suited op-codes or perhaps with nops - we need to
>> > also re-run the alternative assembler patching after we have done our
>> > patching.
>> 
>> These sections are part of .init.* and as such can't reasonably be
>> subject to patching.
> 
> True, thought I thought we have some for run-time as well.

No, we're not switching between UP and SMP like Linux does (or did).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel