[v8.1] xSplice v1 design and implementation.

Commit Message

Konrad Rzeszutek Wilk April 14, 2016, 2:26 p.m. UTC

> Julien, Stefano, I dropped Julien's Ack on
> 
>  [PATCH v8.1 11/27] xsplice: Implement payload loading
> 
> as Jan's expressed a desire not to have BITS_PER_LONG exposed
> in the public headers. I've put CONFIG_ARM_32 in there. On IRC
> Stefano was OK (relucantly) with it.
> 
> Patchset has been tested on ARM (tweaking the Kconfig to build xSplice and then
> booting and using it under ARM CubieTruck - granted there is no patching
> yet but the hypercalls work), ARM64 (only built it), and x86 (legacy and EFI)
> 
.. snip..
> 
> *Are there any TODOs left from v5,v6,v7 reviews?*
> 
> Just how the sysctl.h definition for 'struct xsplice_patch_func' should be:
> 
> #define XSPLICE_PAYLOAD_VERSION 1
> /*
>  * .xsplice.funcs structure layout defined in the `Payload format`
>  * section in the xSplice design document.
>  *
>  * The size should be exactly 64 bytes (64) or 52 bytes (32).
>  *
>  * We guard this with __XEN__ as toolstacks do not need to use it.
>  */
> #ifdef __XEN__
> struct xsplice_patch_func {
>     const char *name;       /* Name of function to be patched. */
> #if CONFIG_ARM_32
>     uint32_t new_addr;
>     uint32_t old_addr;
> #else
>     uint64_t new_addr;
>     uint64_t old_addr;      /* Can be zero and name will be looked up. */
> #endif
>     uint32_t new_size;
>     uint32_t old_size;
>     uint8_t version;        /* MUST be XSPLICE_PAYLOAD_VERSION. */
>     uint8_t pad[31];        /* MUST be zero filled. */
> };
> typedef struct xsplice_patch_func xsplice_patch_func_t;
> #endif
> 
> Please note the __XEN__ so even if toolstack does include the public/sysctl.h
> it won't find it the structure.
> 
> With this I am able to compile early-test-cases-prototype-work xSplice on ARM32.

Jan suggested to just drop the uint32_t and uint64_t business and use
a void pointer. It is nicer so I've put a new version up with this.

The interdiff between this (v8.1) and (v9) is:


Julien, Stefano,

Are you guys OK with this? It does compile under ARM/ARM64 without
trouble. If you are OK I will re-instate Julien's Ack on the 
patch that uses and introduces this:
(xsplice: Implement support for applying/reverting/replacing patches.)

Comments

Julien Grall April 14, 2016, 2:29 p.m. UTC | #1

On 14/04/16 15:26, Konrad Rzeszutek Wilk wrote:
> Julien, Stefano,

Hi Konrad,

> Are you guys OK with this? It does compile under ARM/ARM64 without
> trouble. If you are OK I will re-instate Julien's Ack on the
> patch that uses and introduces this:
> (xsplice: Implement support for applying/reverting/replacing patches.)

I'm fine with this solution.

Regards,

Andrew Cooper April 14, 2016, 3:12 p.m. UTC | #2

On 14/04/16 15:26, Konrad Rzeszutek Wilk wrote:
> @@ -312,8 +307,8 @@ struct xsplice_patch_func {
>  };  
>  </pre>
>  
> -The size of the structure is 64 bytes or 52 bytes if compiled under 32-bit
> -hypervisors.
> +The size of the structure is 64 bytes on 64-bit hypervisors. It will be
> +52 on 32-bit hypervisors.

I would go further an explicitly declare this API unstable, as patches
must be built against an exact hypervisor source.  This also gives us
leaway in the future.

~Andrew

Jan Beulich April 14, 2016, 3:17 p.m. UTC | #3

>>> Andrew Cooper <[e-mail]> 04/14/16 5:14 PM >>>
>On 14/04/16 15:26, Konrad Rzeszutek Wilk wrote:
>> @@ -312,8 +307,8 @@ struct xsplice_patch_func {
>>  };  
>>  </pre>
>>  
>> -The size of the structure is 64 bytes or 52 bytes if compiled under 32-bit
>> -hypervisors.
>> +The size of the structure is 64 bytes on 64-bit hypervisors. It will be
>> +52 on 32-bit hypervisors.
>
>I would go further an explicitly declare this API unstable, as patches
>must be built against an exact hypervisor source.  This also gives us
>leaway in the future.

Which it effectively is, now being (iirc) inside a __XEN__ conditional.

Jan

Konrad Rzeszutek Wilk April 15, 2016, 12:55 a.m. UTC | #4

On Thu, Apr 14, 2016 at 09:17:14AM -0600, Jan Beulich wrote:
> >>> Andrew Cooper <[e-mail]> 04/14/16 5:14 PM >>>
> >On 14/04/16 15:26, Konrad Rzeszutek Wilk wrote:
> >> @@ -312,8 +307,8 @@ struct xsplice_patch_func {
> >>  };  
> >>  </pre>
> >>  
> >> -The size of the structure is 64 bytes or 52 bytes if compiled under 32-bit
> >> -hypervisors.
> >> +The size of the structure is 64 bytes on 64-bit hypervisors. It will be
> >> +52 on 32-bit hypervisors.
> >
> >I would go further an explicitly declare this API unstable, as patches
> >must be built against an exact hypervisor source.  This also gives us
> >leaway in the future.

It will present a problem to the xsplice-tools.git which are outside
the tree. Right now they have their own header file (which mirrors this
one). But if we make it unstable and start mucking around then folks
using the tool would need to pass parameters for 'what version' is
this.

I would rather not have this happen - and now that we do not need
the #ifdef BITS_PER_LONG we can also remove the __XEN__ conditional.

Either way, this is something that can be done later.
> 
> Which it effectively is, now being (iirc) inside a __XEN__ conditional.
> 
> Jan
>

Jan Beulich April 17, 2016, 8 a.m. UTC | #5

>>> Konrad Rzeszutek Wilk <[e-mail]> 04/15/16 2:55 AM >>>
>On Thu, Apr 14, 2016 at 09:17:14AM -0600, Jan Beulich wrote:
>> >>> Andrew Cooper <[e-mail]> 04/14/16 5:14 PM >>>
>> >On 14/04/16 15:26, Konrad Rzeszutek Wilk wrote:
>> >> @@ -312,8 +307,8 @@ struct xsplice_patch_func {
>> >>  };  
>> >>  </pre>
>> >>  
>> >> -The size of the structure is 64 bytes or 52 bytes if compiled under 32-bit
>> >> -hypervisors.
>> >> +The size of the structure is 64 bytes on 64-bit hypervisors. It will be
>> >> +52 on 32-bit hypervisors.
>> >
>> >I would go further an explicitly declare this API unstable, as patches
>> >must be built against an exact hypervisor source.  This also gives us
>> >leaway in the future.
>
>It will present a problem to the xsplice-tools.git which are outside
>the tree. Right now they have their own header file (which mirrors this
>one). But if we make it unstable and start mucking around then folks
>using the tool would need to pass parameters for 'what version' is
>this.
>
>I would rather not have this happen - and now that we do not need
>the #ifdef BITS_PER_LONG we can also remove the __XEN__ conditional.

I don't think that's the case: The mere use of pointer types in there makes it
so the type shouldn't be exposed to entities executing outside of hypervisor
context.

Jan

Patch

diff --git a/docs/misc/xsplice.markdown b/docs/misc/xsplice.markdown
index fb13a7d..d5abab0 100644
--- a/docs/misc/xsplice.markdown
+++ b/docs/misc/xsplice.markdown
@@ -298,13 +298,8 @@  which describe the functions to be patched:
 <pre>
 struct xsplice_patch_func {  
     const char *name;  
-#if BITS_PER_LONG == 32  
-    uint32_t new_addr;  
-    uint32_t old_addr;  
-#else  
-    uint64_t new_addr;  
-    uint64_t old_addr;  
-#endif
+    void *new_addr;  
+    void *old_addr;  
     uint32_t new_size;  
     uint32_t old_size;  
     uint8_t version;  
@@ -312,8 +307,8 @@  struct xsplice_patch_func {
 };  
 </pre>
 
-The size of the structure is 64 bytes or 52 bytes if compiled under 32-bit
-hypervisors.
+The size of the structure is 64 bytes on 64-bit hypervisors. It will be
+52 on 32-bit hypervisors.
 
 * `name` is the symbol name of the old function. Only used if `old_addr` is
    zero, otherwise will be used during dynamic linking (when hypervisor loads
@@ -353,8 +348,8 @@  A simple example of what a payload file can be:
 /* MUST be in sync with hypervisor. */  
 struct xsplice_patch_func {  
     const char *name;  
-    uint64_t new_addr;  
-    uint64_t old_addr;  
+    void *new_addr;  
+    void *old_addr;  
     uint32_t new_size;  
     uint32_t old_size;  
     uint8_t version;
@@ -372,8 +367,8 @@  static unsigned char patch_this_fnc[] = "xen_extra_version";
 struct xsplice_patch_func xsplice_hello_world = {  
     .version = XSPLICE_PAYLOAD_VERSION,
     .name = patch_this_fnc,  
-    .new_addr = (unsigned long)(xen_hello_world),  
-    .old_addr = 0xffff82d08013963c, /* Extracted from xen-syms. */  
+    .new_addr = xen_hello_world,  
+    .old_addr = (void *)0xffff82d08013963c, /* Extracted from xen-syms. */  
     .new_size = 13, /* To be be computed by scripts. */  
     .old_size = 13, /* -----------""---------------  */  
 } __attribute__((__section__(".xsplice.funcs")));  
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 73798c3..3a76f55 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6102,7 +6102,7 @@  void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
             v += PAGE_SIZE;
 
             /*
-             * If we not destroying mappings, or are not done with the L2E,
+             * If we are not destroying mappings, or not done with the L2E,
              * skip the empty&free check.
              */
             if ( (nf & _PAGE_PRESENT) || ((v != e) && (l1_table_offset(v) != 0)) )
diff --git a/xen/arch/x86/test/xen_bye_world.c b/xen/arch/x86/test/xen_bye_world.c
index b2f25bd..be83b5a 100644
--- a/xen/arch/x86/test/xen_bye_world.c
+++ b/xen/arch/x86/test/xen_bye_world.c
@@ -17,8 +17,8 @@  extern const char *xen_extra_version(void);
 struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_bye_world = {
     .version = XSPLICE_PAYLOAD_VERSION,
     .name = bye_world_patch_this_fnc,
-    .new_addr = (unsigned long)(xen_bye_world),
-    .old_addr = (unsigned long)(xen_extra_version),
+    .new_addr = xen_bye_world,
+    .old_addr = (xen_extra_version,
     .new_size = NEW_CODE_SZ,
     .old_size = OLD_CODE_SZ,
 };
diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
index 22fe2e7..a67b798 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -16,8 +16,8 @@  extern const char *xen_extra_version(void);
 struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_hello_world = {
     .version = XSPLICE_PAYLOAD_VERSION,
     .name = hello_world_patch_this_fnc,
-    .new_addr = (unsigned long)(xen_hello_world),
-    .old_addr = (unsigned long)(xen_extra_version),
+    .new_addr = xen_hello_world,
+    .old_addr = xen_extra_version,
     .new_size = NEW_CODE_SZ,
     .old_size = OLD_CODE_SZ,
 };
diff --git a/xen/arch/x86/test/xen_replace_world.c b/xen/arch/x86/test/xen_replace_world.c
index 70516bc..6030139 100644
--- a/xen/arch/x86/test/xen_replace_world.c
+++ b/xen/arch/x86/test/xen_replace_world.c
@@ -17,8 +17,8 @@  extern const char *xen_extra_version(void);
 struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_replace_world = {
     .version = XSPLICE_PAYLOAD_VERSION,
     .name = xen_replace_world_name,
-    .new_addr = (unsigned long)(xen_replace_world),
-    .old_addr = (unsigned long)(xen_extra_version),
+    .new_addr = xen_replace_world,
+    .old_addr = xen_extra_version,
     .new_size = NEW_CODE_SZ,
     .old_size = OLD_CODE_SZ,
 };
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 3d47b47..3876b04 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -845,13 +845,8 @@  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_featureset_t);
 #ifdef __XEN__
 struct xsplice_patch_func {
     const char *name;       /* Name of function to be patched. */
-#if CONFIG_ARM_32
-    uint32_t new_addr;
-    uint32_t old_addr;
-#else
-    uint64_t new_addr;
-    uint64_t old_addr;      /* Can be zero and name will be looked up. */
-#endif
+    void *new_addr;
+    void *old_addr;
     uint32_t new_size;
     uint32_t old_size;
     uint8_t version;        /* MUST be XSPLICE_PAYLOAD_VERSION. */