Re: [PATCH v8.1] xSplice v1 design and implementation.

[Konrad Rzeszutek Wilk <konrad@kernel.org>]

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

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. */

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