diff mbox

[v8.1,12/27] xsplice: Implement support for applying/reverting/replacing patches.

Message ID 20160421202743.GA24122@localhost.localdomain (mailing list archive)
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk April 21, 2016, 8:27 p.m. UTC 
On Thu, Apr 21, 2016 at 12:44:41AM -0600, Jan Beulich wrote:
> >>> On 21.04.16 at 02:28, <konrad.wilk@oracle.com> wrote:
> >> >+    ASSERT(sec);
> >> >+    if ( sec->sec->sh_size % sizeof(*payload->funcs) )
> >> >+    {
> >> >+        dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .xsplice.funcs!\n",
> >> >+                elf->name);
> >> >+        return -EINVAL;
> >> >+    }
> >> >+
> >> >+    payload->funcs = sec->load_addr;
> >> >+    payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
> >> 
> >> Following to our discussion yesterday - can't we (ab)use the section merge
> >> flag here to report the structure size, along the lines of what relocation
> >> sections do for their elements?
> > 
> > 
> > In other words - the xsplice-tools.git and the test-cases (in
> > arch/x86/xen/test) would modify the sec->sh_entsize to have the
> > sizeof(xsplice_patch_func) aka 64 (0x40) ?
> > 
> > [Note this could also extend to .xsplice.hooks.load and
> > .xsplice.hooks.unload]
> > 
> > The xsplice-tools.git could surely do it, but the built-in test-cases -
> > not so much. I would need to do some in-place binary handrolling - unless you
> > know of some tool or some __section modifiers?
> 
> Well, that's why I suggest (ab) using the SHF_MERGE flag, i.e. to
> see whether we can make gas do what we want. Fiddling with the
> binary I would not recommend, i.e. if we can't get gas to do what
> we want we should rather stay with what there is.

This:


Makes the .xsplice.funcs have sh_entsize have the size of 64 bytes:


  [ 6] .xsplice.funcs    PROGBITS         0000000000000000  000000a0
       0000000000000040  0000000000000000  WA       0     0     32
vs

  [ 6] .xsplice.funcs    PROGBITS         0000000000000000  000000a0
       0000000000000040  0000000000000040 WAM       0     0     32

Unfortunatly the compilation throws this:

FC-64 <konrad@build-external:~/xtt-x86_64/xen/xen> make -j4 tests 1>1
dnsdomainname: Name or service not known
{standard input}: Assembler messages:
{standard input}:184: Warning: ignoring changed section attributes for .xsplice.funcs


This is with:
binutils-2.20.51.0.2-15.fc13.x86_64
and with:
binutils-2.25-9.fc22.x86_64

Ideas?

I could try to wrap the definition of these
struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_hello_world 
using an macro - but that did not work very well (I couldn't manage it
to take the value of say XSPLICE_PAYLOAD_VERSION).

Perhaps skip this?
> 
> Jan
>

Comments

Jan Beulich April 22, 2016, 7:44 a.m. UTC | #1 
>>> On 21.04.16 at 22:27, <konrad@kernel.org> wrote:
> On Thu, Apr 21, 2016 at 12:44:41AM -0600, Jan Beulich wrote:
>> >>> On 21.04.16 at 02:28, <konrad.wilk@oracle.com> wrote:
>> >> >+    ASSERT(sec);
>> >> >+    if ( sec->sec->sh_size % sizeof(*payload->funcs) )
>> >> >+    {
>> >> >+        dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .xsplice.funcs!\n",
>> >> >+                elf->name);
>> >> >+        return -EINVAL;
>> >> >+    }
>> >> >+
>> >> >+    payload->funcs = sec->load_addr;
>> >> >+    payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
>> >> 
>> >> Following to our discussion yesterday - can't we (ab)use the section merge
>> >> flag here to report the structure size, along the lines of what relocation
>> >> sections do for their elements?
>> > 
>> > 
>> > In other words - the xsplice-tools.git and the test-cases (in
>> > arch/x86/xen/test) would modify the sec->sh_entsize to have the
>> > sizeof(xsplice_patch_func) aka 64 (0x40) ?
>> > 
>> > [Note this could also extend to .xsplice.hooks.load and
>> > .xsplice.hooks.unload]
>> > 
>> > The xsplice-tools.git could surely do it, but the built-in test-cases -
>> > not so much. I would need to do some in-place binary handrolling - unless you
>> > know of some tool or some __section modifiers?
>> 
>> Well, that's why I suggest (ab) using the SHF_MERGE flag, i.e. to
>> see whether we can make gas do what we want. Fiddling with the
>> binary I would not recommend, i.e. if we can't get gas to do what
>> we want we should rather stay with what there is.
> 
> This:
> 
> diff --git a/xen/arch/x86/test/xen_hello_world.c 
> b/xen/arch/x86/test/xen_hello_world.c
> index a3da85d..92f2e97 100644
> --- a/xen/arch/x86/test/xen_hello_world.c
> +++ b/xen/arch/x86/test/xen_hello_world.c
> @@ -47,6 +47,13 @@ XSPLICE_UNLOAD_HOOK(hi_func);
>  
>  XSPLICE_UNLOAD_HOOK(Z_check_fnc);
>  
> +#define XSPLICE_FUNC() \
> +    asm ( \
> +    ".pushsection .xsplice.funcs, \"awM\", @progbits, 64\n " \
> +    ".popsection")
> +
> +XSPLICE_FUNC();
> +
>  struct xsplice_patch_func __section(".xsplice.funcs") 
> xsplice_xen_hello_world = {
>      .version = XSPLICE_PAYLOAD_VERSION,
>      .name = hello_world_patch_this_fnc,
> 
> Makes the .xsplice.funcs have sh_entsize have the size of 64 bytes:
> 
> 
>   [ 6] .xsplice.funcs    PROGBITS         0000000000000000  000000a0
>        0000000000000040  0000000000000000  WA       0     0     32
> vs
> 
>   [ 6] .xsplice.funcs    PROGBITS         0000000000000000  000000a0
>        0000000000000040  0000000000000040 WAM       0     0     32
> 
> Unfortunatly the compilation throws this:
> 
> FC-64 <konrad@build-external:~/xtt-x86_64/xen/xen> make -j4 tests 1>1
> dnsdomainname: Name or service not known
> {standard input}: Assembler messages:
> {standard input}:184: Warning: ignoring changed section attributes for 
> .xsplice.funcs
> 
> 
> This is with:
> binutils-2.20.51.0.2-15.fc13.x86_64
> and with:
> binutils-2.25-9.fc22.x86_64
> 
> Ideas?

Well, this presumably being a redundant .section directive
somewhere it would require seeing the full .c together with the
intermediate .s in order to tell. But I guess to avoid you spending
more time on this secondary aspect, let's just drop this for now
and just put it on the todo list (as long as the thing is tech preview
we can still alter requirements we place on the actual payloads).

Jan
Konrad Rzeszutek Wilk April 22, 2016, 10:51 a.m. UTC | #2 
> > Ideas?
> 
> Well, this presumably being a redundant .section directive
> somewhere it would require seeing the full .c together with the
> intermediate .s in order to tell. But I guess to avoid you spending
> more time on this secondary aspect, let's just drop this for now
> and just put it on the todo list (as long as the thing is tech preview
> we can still alter requirements we place on the actual payloads).

Yes to "Tech Preview". I put it on http://wiki.xen.org/wiki/XSplice#Hypervisor
so I would not forget.
diff mbox

Patch

diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
index a3da85d..92f2e97 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -47,6 +47,13 @@  XSPLICE_UNLOAD_HOOK(hi_func);
 
 XSPLICE_UNLOAD_HOOK(Z_check_fnc);
 
+#define XSPLICE_FUNC() \
+    asm ( \
+    ".pushsection .xsplice.funcs, \"awM\", @progbits, 64\n " \
+    ".popsection")
+
+XSPLICE_FUNC();
+
 struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_hello_world = {
     .version = XSPLICE_PAYLOAD_VERSION,
     .name = hello_world_patch_this_fnc,