[PATCH v2] Livepatch for ARM 64 and 32.

[PATCH v2] Livepatch for ARM 64 and 32.

[Konrad Rzeszutek Wilk]

Hey!

Since v1 (and RFC): [https://lists.xen.org/archives/html/xen-devel/2016-08/msg01835.html]
 - Acted on most all comments.
 - Added ARM32 support.

The patches are based on: [PATCH v4] Livepatch fixes and features for v4.8.
(https://lists.xen.org/archives/html/xen-devel/2016-08/msg02705.html)

And the git tree is:
 git://xenbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v4

There are two outstanding questions that should be addressed at some point:
 -  #16 "livepatch: tests: Make them compile under ARM64"
    We chatted about adding a specific CPU bit (LIVEPATCH) so that alternative
    code always gets exercised. This being for both x86 and ARM. But this has
    a side effect that it gets exposed to the toolstack and can also be
    exposed to the guests. I think it is better if we do not do that
    so left it to be enabled based on the most common errata (or feature
    on x86).
 -  #13 "livepatch: Initial ARM64 support."
    Need to look in erratum #843419 on some Cortex-A53 and figuring
    out how to avoid payloads having R_AARCH64_ADR_PREL_PG_HI21 relocations.
    
But beside that, please enjoy the patchset! Hopefully I didn't miss
any comments - if I did, please remind me!

 .gitignore                                         |   8 +-
 MAINTAINERS                                        |   2 +
 xen/Makefile                                       |   5 +-
 xen/arch/arm/Makefile                              |  16 +-
 xen/arch/arm/alternative.c                         |  50 ++--
 xen/arch/arm/arm32/Makefile                        |   1 +
 xen/arch/arm/arm32/livepatch.c                     | 286 ++++++++++++++++++
 xen/arch/arm/arm64/Makefile                        |   1 +
 xen/arch/arm/arm64/insn.c                          |  61 ++++
 xen/arch/arm/arm64/livepatch.c                     | 330 +++++++++++++++++++++
 xen/arch/arm/domain.c                              |   6 +
 xen/arch/arm/livepatch.c                           | 135 +++++++--
 xen/arch/arm/mm.c                                  |  27 +-
 xen/arch/arm/traps.c                               |   6 +
 xen/arch/x86/Kconfig                               |   8 +
 xen/arch/x86/Makefile                              |   5 -
 xen/arch/x86/alternative.c                         |  29 +-
 xen/arch/x86/livepatch.c                           |  27 +-
 xen/arch/x86/mm.c                                  |   5 +-
 xen/common/Kconfig                                 |   2 +-
 xen/common/livepatch.c                             |  29 +-
 xen/common/livepatch_elf.c                         |  27 +-
 xen/include/asm-arm/alternative.h                  |   7 +-
 xen/include/asm-arm/arm32/page.h                   |   9 +
 xen/include/asm-arm/arm64/insn.h                   |  23 ++
 xen/include/asm-arm/config.h                       |   9 +-
 xen/include/asm-arm/livepatch.h                    |  28 ++
 xen/include/asm-arm/page.h                         |  11 +
 xen/include/asm-x86/alternative.h                  |  11 +-
 xen/include/xen/elfstructs.h                       |  56 +++-
 xen/include/xen/livepatch.h                        |   4 +-
 xen/include/xen/mm.h                               |   2 +-
 xen/include/xen/types.h                            |   6 +
 xen/test/Makefile                                  |   7 +
 xen/{arch/x86/test => test/livepatch}/Makefile     |  14 +-
 .../x86/test => test/livepatch}/xen_bye_world.c    |   0
 .../test => test/livepatch}/xen_bye_world_func.c   |   0
 .../x86/test => test/livepatch}/xen_hello_world.c  |   0
 .../test => test/livepatch}/xen_hello_world_func.c |   8 +-
 .../test => test/livepatch}/xen_replace_world.c    |   0
 .../livepatch}/xen_replace_world_func.c            |   0
 41 files changed, 1139 insertions(+), 122 deletions(-)

Konrad Rzeszutek Wilk (20):
      livepatch: Bubble up sanity checks on Elf relocs
      x86/arm: Make 'make debug' work properly.
      x86/arm64: Expose the ALT_[ORIG|REPL]_PTR macros to header files.
      alternatives: x86 rename and change parameters on ARM
      arm64/alternatives: Make it possible to patch outside of hypervisor.
      arm/alternative: Use _start instead of _stext
      arm/x86: Add ALTERNATIVE and HAS_EX_TABLE
      x86: change modify_xen_mappings to return error
      arm/mm: Introduce modify_xen_mappings
      arm64/insn: introduce aarch64_insn_gen_{nop|branch_imm}() helper functions
      arm/arm64: Update comment about VA layout.
      x86,arm: Change arch_livepatch_quiesce() decleration.
      livepatch: Initial ARM64 support.
      livepatch: ARM 32|64: Ignore mapping symbols: $[d,a,x,t]
      livepatch: Move test-cases to common
      livepatch: tests: Make them compile under ARM64
      xen/arm32: Add an helper to invalidate all instruction caches
      xen/arm32/livepatch: Add BPICALLIS to helper to invalidate all instruction caches
      livepatch/elf: Adjust section aligment to word
      livepatch: ARM32 support.


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

[PATCH v2 01/20] livepatch: Bubble up sanity checks on Elf relocs

[Konrad Rzeszutek Wilk]

The checks for SHT_REL[,A] ELF sanity checks does not need to
be in the platform specific file and can be bubbled up
in the platform agnostic file.

This makes the ARM 32/64 implementation easier as the
duplicate checks don't have to be in the platform specific files.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v1: First submission
v2: Mirror checks for SHT_REL case.
---
 xen/arch/x86/livepatch.c   | 12 ------------
 xen/common/livepatch_elf.c | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 952e897..5b0c863 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -146,18 +146,6 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
     uint64_t val;
     uint8_t *dest;
 
-    /* Nothing to do. */
-    if ( !rela->sec->sh_size )
-        return 0;
-
-    if ( rela->sec->sh_entsize < sizeof(Elf_RelA) ||
-         rela->sec->sh_size % rela->sec->sh_entsize )
-    {
-        dprintk(XENLOG_ERR, LIVEPATCH "%s: Section relative header is corrupted!\n",
-                elf->name);
-        return -EINVAL;
-    }
-
     for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
     {
         r = rela->data + i * rela->sec->sh_entsize;
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index 789e8fc..cda9b27 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -335,6 +335,7 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf)
     struct livepatch_elf_sec *r, *base;
     unsigned int i;
     int rc = 0;
+    size_t sz;
 
     ASSERT(elf->sym);
 
@@ -365,6 +366,22 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf)
         }
 
         if ( r->sec->sh_type == SHT_RELA )
+            sz = sizeof(Elf_RelA);
+        else
+            sz = sizeof(Elf_Rel);
+
+        if ( !r->sec->sh_size )
+            continue;
+
+        if ( r->sec->sh_entsize < sz || r->sec->sh_size % r->sec->sh_entsize )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section relative header is corrupted!\n",
+                    elf->name);
+            rc = -EINVAL;
+            break;
+        }
+
+        if ( r->sec->sh_type == SHT_RELA )
             rc = arch_livepatch_perform_rela(elf, base, r);
         else /* SHT_REL */
             rc = arch_livepatch_perform_rel(elf, base, r);
-- 
2.4.11


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

[PATCH v2 02/20] x86/arm: Make 'make debug' work properly.

[Konrad Rzeszutek Wilk]

When doing cross-compilation we should use proper $(OBJDUMP).
Otherwise decompiling say ARM 32 code using x86 objdump
won't help much.

Acked-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>

v1: First submission
v2: Added Jan's Acked-by.
    Reworded commit description.
---
 xen/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/Makefile b/xen/Makefile
index 294fb9e..d68c84d 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -103,7 +103,7 @@ _uninstall:
 
 .PHONY: _debug
 _debug:
-	objdump -D -S $(TARGET)-syms > $(TARGET).s
+	$(OBJDUMP) -D -S $(TARGET)-syms > $(TARGET).s
 
 .PHONY: _clean
 _clean: delete-unfresh-files
-- 
2.4.11


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

[PATCH v2 03/20] x86/arm64: Expose the ALT_[ORIG|REPL]_PTR macros to header files.

[Konrad Rzeszutek Wilk]

That way common code can use the same macro to access
the most common attributes without much #ifdef.

Take advantage of it right away in the livepatch code.

Note: on ARM we use tabs to conform to the style of the file.

Acked-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v1: First submission
v2: Added Jan's Ack
    Added extra set of paranetheses for __ALT_PTR (x86 & ARM)
    Used tabs instead of spaces on ARM header file.
---
 xen/arch/arm/alternative.c        | 4 ----
 xen/common/livepatch.c            | 4 ++--
 xen/include/asm-arm/alternative.h | 5 +++++
 xen/include/asm-x86/alternative.h | 4 ++++
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 8ee5a11..bf4101c 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -32,10 +32,6 @@
 #include <asm/insn.h>
 #include <asm/page.h>
 
-#define __ALT_PTR(a,f)      (u32 *)((void *)&(a)->f + (a)->f)
-#define ALT_ORIG_PTR(a)     __ALT_PTR(a, orig_offset)
-#define ALT_REPL_PTR(a)     __ALT_PTR(a, alt_offset)
-
 extern const struct alt_instr __alt_instructions[], __alt_instructions_end[];
 
 struct alt_region {
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 528c0c9..774a51d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -702,8 +702,8 @@ static int prepare_payload(struct payload *payload,
 
         for ( a = start; a < end; a++ )
         {
-            const void *instr = &a->instr_offset + a->instr_offset;
-            const void *replacement = &a->repl_offset + a->repl_offset;
+            const void *instr = ALT_ORIG_PTR(a);
+            const void *replacement = ALT_REPL_PTR(a);
 
             if ( (instr < region->start && instr >= region->end) ||
                  (replacement < region->start && replacement >= region->end) )
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 4287bac..f25d3a7 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -21,6 +21,11 @@ struct alt_instr {
 	u8  alt_len;		/* size of new instruction(s), <= orig_len */
 };
 
+/* Xen: helpers used by common code. */
+#define __ALT_PTR(a,f)		((u32 *)((void *)&(a)->f + (a)->f))
+#define ALT_ORIG_PTR(a)		__ALT_PTR(a, orig_offset)
+#define ALT_REPL_PTR(a)		__ALT_PTR(a, alt_offset)
+
 void __init apply_alternatives_all(void);
 int apply_alternatives(void *start, size_t length);
 
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index acaeded..de807c8 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -23,6 +23,10 @@ struct alt_instr {
     u8  replacementlen;     /* length of new instruction, <= instrlen */
 };
 
+#define __ALT_PTR(a,f)      ((u8 *)((void *)&(a)->f + (a)->f))
+#define ALT_ORIG_PTR(a)     __ALT_PTR(a, instr_offset)
+#define ALT_REPL_PTR(a)     __ALT_PTR(a, repl_offset)
+
 extern void add_nops(void *insns, unsigned int len);
 /* Similar to apply_alternatives except it can be run with IRQs enabled. */
 extern void apply_alternatives_nocheck(struct alt_instr *start,
-- 
2.4.11


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

[PATCH v2 04/20] alternatives: x86 rename and change parameters on ARM

[Konrad Rzeszutek Wilk]

On x86 we squash 'apply_alternatives' in to
'alternative_instructions' (who was its sole user)
and 'apply_alternatives_nocheck' to 'apply_alternatives'.

On ARM we change the parameters for 'apply_alternatives'
to be of 'const struct alt_instr *' instead of void pointer and
size length.

We also add 'const' and make the arguments be on the
proper offset.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>

v1: First submission.
v2: Squash apply_alternatives(old) in alternative_instructions.
    Add const on x86.
---
 xen/arch/arm/alternative.c        |  4 ++--
 xen/arch/x86/alternative.c        | 29 ++++++++++++-----------------
 xen/common/livepatch.c            |  2 +-
 xen/include/asm-arm/alternative.h |  2 +-
 xen/include/asm-x86/alternative.h |  7 +++----
 5 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index bf4101c..aba06db 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -200,11 +200,11 @@ void __init apply_alternatives_all(void)
     BUG_ON(ret);
 }
 
-int apply_alternatives(void *start, size_t length)
+int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end)
 {
     const struct alt_region region = {
         .begin = start,
-        .end = start + length,
+        .end = end,
     };
 
     return __apply_alternatives(&region);
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index fd8528e..6eaa10f 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -144,9 +144,10 @@ static void *init_or_livepatch text_poke(void *addr, const void *opcode, size_t
  * APs have less capabilities than the boot processor are not handled.
  * Tough. Make sure you disable such features by hand.
  */
-void init_or_livepatch apply_alternatives_nocheck(struct alt_instr *start, struct alt_instr *end)
+void init_or_livepatch apply_alternatives(const struct alt_instr *start,
+                                          const struct alt_instr *end)
 {
-    struct alt_instr *a;
+    const struct alt_instr *a;
     u8 *instr, *replacement;
     u8 insnbuf[MAX_PATCH_LEN];
 
@@ -187,24 +188,10 @@ void init_or_livepatch apply_alternatives_nocheck(struct alt_instr *start, struc
  * This routine is called with local interrupt disabled and used during
  * bootup.
  */
-void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
-{
-    unsigned long cr0 = read_cr0();
-
-    ASSERT(!local_irq_is_enabled());
-
-    /* Disable WP to allow application of alternatives to read-only pages. */
-    write_cr0(cr0 & ~X86_CR0_WP);
-
-    apply_alternatives_nocheck(start, end);
-
-    /* Reinstate WP. */
-    write_cr0(cr0);
-}
-
 void __init alternative_instructions(void)
 {
     nmi_callback_t *saved_nmi_callback;
+    unsigned long cr0 = read_cr0();
 
     arch_init_ideal_nops();
 
@@ -225,7 +212,15 @@ void __init alternative_instructions(void)
      * expect a machine check to cause undue problems during to code
      * patching.
      */
+    ASSERT(!local_irq_is_enabled());
+
+    /* Disable WP to allow application of alternatives to read-only pages. */
+    write_cr0(cr0 & ~X86_CR0_WP);
+
     apply_alternatives(__alt_instructions, __alt_instructions_end);
 
+    /* Reinstate WP. */
+    write_cr0(cr0);
+
     set_nmi_callback(saved_nmi_callback);
 }
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 774a51d..b771c7d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -713,7 +713,7 @@ static int prepare_payload(struct payload *payload,
                 return -EINVAL;
             }
         }
-        apply_alternatives_nocheck(start, end);
+        apply_alternatives(start, end);
     }
 
     sec = livepatch_elf_sec_by_name(elf, ".ex_table");
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index f25d3a7..9f88fd9 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -27,7 +27,7 @@ struct alt_instr {
 #define ALT_REPL_PTR(a)		__ALT_PTR(a, alt_offset)
 
 void __init apply_alternatives_all(void);
-int apply_alternatives(void *start, size_t length);
+int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end);
 
 #define ALTINSTR_ENTRY(feature)						      \
 	" .word 661b - .\n"				/* label           */ \
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index de807c8..db4f08e 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -28,10 +28,9 @@ struct alt_instr {
 #define ALT_REPL_PTR(a)     __ALT_PTR(a, repl_offset)
 
 extern void add_nops(void *insns, unsigned int len);
-/* Similar to apply_alternatives except it can be run with IRQs enabled. */
-extern void apply_alternatives_nocheck(struct alt_instr *start,
-                                       struct alt_instr *end);
-extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
+/* Similar to alternative_instructions except it can be run with IRQs enabled. */
+extern void apply_alternatives(const struct alt_instr *start,
+                               const struct alt_instr *end);
 extern void alternative_instructions(void);
 
 #define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
-- 
2.4.11


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

[PATCH v2 05/20] arm64/alternatives: Make it possible to patch outside of hypervisor.

[Konrad Rzeszutek Wilk]

With livepatching the alternatives that should be patched are
outside the Xen hypervisor _start -> _end. As such having
to use an alternative VA is not neccessary. In fact we
can use the ones that the caller provided us with.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>

v2: First version
---
 xen/arch/arm/alternative.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index aba06db..90a857a 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -94,24 +94,30 @@ static int __apply_alternatives(const struct alt_region *region)
 {
     const struct alt_instr *alt;
     const u32 *origptr, *replptr;
-    u32 *writeptr, *writemap;
+    u32 *writeptr, *writemap = NULL;
     mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
     unsigned int text_order = get_order_from_bytes(_end - _start);
 
-    printk(XENLOG_INFO "alternatives: Patching kernel code\n");
-
-    /*
-     * The text section is read-only. So re-map Xen to be able to patch
-     * the code.
-     */
-    writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
-                      VMAP_DEFAULT);
-    if ( !writemap )
+    if ( region->begin >= __alt_instructions &&
+         region->end <= __alt_instructions_end )
     {
-        printk(XENLOG_ERR "alternatives: Unable to map the text section (size %u)\n",
-               1 << text_order);
-        return -ENOMEM;
+        printk(XENLOG_INFO "alternatives: Patching kernel code\n");
+        /*
+        * The text section is read-only. So re-map Xen to be able to patch
+        * the code.
+        */
+        writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
+                          VMAP_DEFAULT);
+        if ( !writemap )
+        {
+            printk(XENLOG_ERR "alternatives: Unable to map the text section (size %u)\n",
+                   1 << text_order);
+            return -ENOMEM;
+        }
     }
+    else
+        printk(XENLOG_INFO "alternatives: Patching %p -> %p\n",
+               region->begin, region->end);
 
     for ( alt = region->begin; alt < region->end; alt++ )
     {
@@ -124,8 +130,11 @@ static int __apply_alternatives(const struct alt_region *region)
         BUG_ON(alt->alt_len != alt->orig_len);
 
         origptr = ALT_ORIG_PTR(alt);
-        writeptr = origptr - (u32 *)_start + writemap;
         replptr = ALT_REPL_PTR(alt);
+        if ( writemap )
+            writeptr = origptr - (u32 *)_start + writemap;
+        else
+            writeptr = (u32 *)origptr;
 
         nr_inst = alt->alt_len / sizeof(insn);
 
@@ -143,7 +152,8 @@ static int __apply_alternatives(const struct alt_region *region)
     /* Nuke the instruction cache */
     invalidate_icache();
 
-    vunmap(writemap);
+    if ( writemap )
+        vunmap(writemap);
 
     return 0;
 }
-- 
2.4.11


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

[PATCH v2 06/20] arm/alternative: Use _start instead of _stext

[Konrad Rzeszutek Wilk]

The code uses a mix of _start, _stext, and _etext. Lets unify
it and use the same 'style'.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>

v2: First submission
---
 xen/arch/arm/alternative.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 90a857a..ed84170 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -95,7 +95,7 @@ static int __apply_alternatives(const struct alt_region *region)
     const struct alt_instr *alt;
     const u32 *origptr, *replptr;
     u32 *writeptr, *writemap = NULL;
-    mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
+    mfn_t text_mfn = _mfn(virt_to_mfn(_start));
     unsigned int text_order = get_order_from_bytes(_end - _start);
 
     if ( region->begin >= __alt_instructions &&
-- 
2.4.11


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

[PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Konrad Rzeszutek Wilk]

x86 implements all of them by default - and we just
add two extra CONFIG_ variables to be declared in autoconf.h.

ARM 64 only has alternative while ARM 32 has none of them.

And while at it change the livepatch common code that
would benefit from this.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Doug Goldstein <cardoe@cardoe.com>

v2: First submission
---
 xen/arch/x86/Kconfig   | 8 ++++++++
 xen/common/livepatch.c | 5 ++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 265fd79..daef864 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -5,10 +5,12 @@ config X86
 	def_bool y
 	select ACPI
 	select ACPI_LEGACY_TABLES_LOOKUP
+	select ALTERNATIVE
 	select COMPAT
 	select CORE_PARKING
 	select HAS_CPUFREQ
 	select HAS_EHCI
+	select HAS_EX_TABLE
 	select HAS_GDBSX
 	select HAS_IOPORTS
 	select HAS_KEXEC
@@ -22,6 +24,12 @@ config X86
 	select NUMA
 	select VGA
 
+config ALTERNATIVE
+	bool
+
+config HAS_EX_TABLE
+	bool
+
 config ARCH_DEFCONFIG
 	string
 	default "arch/x86/configs/x86_64_defconfig"
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index b771c7d..7e36d97 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -3,6 +3,7 @@
  *
  */
 
+#include <xen/config.h>
 #include <xen/cpu.h>
 #include <xen/elf.h>
 #include <xen/err.h>
@@ -684,7 +685,7 @@ static int prepare_payload(struct payload *payload,
                                   sizeof(*region->frame[i].bugs);
     }
 
-#ifndef CONFIG_ARM
+#ifdef CONFIG_ALTERNATIVE
     sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
     if ( sec )
     {
@@ -715,7 +716,9 @@ static int prepare_payload(struct payload *payload,
         }
         apply_alternatives(start, end);
     }
+#endif
 
+#ifdef CONFIG_HAS_EX_TABLE
     sec = livepatch_elf_sec_by_name(elf, ".ex_table");
     if ( sec )
     {
-- 
2.4.11


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

[PATCH v2 08/20] x86: change modify_xen_mappings to return error

[Konrad Rzeszutek Wilk]

The implementation on x86 always returns zero, but
other platforms may return error values.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v2: First submission
---
 xen/arch/x86/livepatch.c | 4 +---
 xen/arch/x86/mm.c        | 5 +++--
 xen/include/xen/mm.h     | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 5b0c863..67dda07 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -234,9 +234,7 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
     else
         flag = PAGE_HYPERVISOR_RO;
 
-    modify_xen_mappings(start, start + pages * PAGE_SIZE, flag);
-
-    return 0;
+    return modify_xen_mappings(start, start + pages * PAGE_SIZE, flag);
 }
 
 void __init arch_livepatch_init(void)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ff8e904..94e684e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5974,7 +5974,7 @@ int populate_pt_range(unsigned long virt, unsigned long mfn,
  *
  * It is an error to call with present flags over an unpopulated range.
  */
-void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
+int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
 {
     bool_t locking = system_state > SYS_STATE_boot;
     l2_pgentry_t *pl2e;
@@ -6149,13 +6149,14 @@ void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
     flush_area(NULL, FLUSH_TLB_GLOBAL);
 
 #undef FLAGS_MASK
+    return 0;
 }
 
 #undef flush_area
 
 void destroy_xen_mappings(unsigned long s, unsigned long e)
 {
-    modify_xen_mappings(s, e, _PAGE_NONE);
+    (void)modify_xen_mappings(s, e, _PAGE_NONE);
 }
 
 void __set_fixmap(
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 58bc0b8..f257bbc 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -146,7 +146,7 @@ int map_pages_to_xen(
     unsigned long nr_mfns,
     unsigned int flags);
 /* Alter the permissions of a range of Xen virtual address space. */
-void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags);
+int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags);
 void destroy_xen_mappings(unsigned long v, unsigned long e);
 /*
  * Create only non-leaf page table entries for the
-- 
2.4.11


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

[PATCH v2 09/20] arm/mm: Introduce modify_xen_mappings

[Konrad Rzeszutek Wilk]

Which is only used by Livepatch code. The purpose behind
this call is to modify the page table entries flags.

Specifically the .ro and .nx flags. The current mechanism
puts cache attributes in the flags and the .ro and .nx are
locked down and assumed to be .ro=0, nx=1.

Livepatch needs .nx=0 and also .ro to be set to 1.

We introduce a new 'flags' where various bits determine
whether .ro and .nx bits are set or cleared. We can't use
an enum as the function prototype would diverge from x86.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
--
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
[Added as he wrote the x86 modify_xen_mappings version]

RFC: First submission.
v1: Add #defines to make it simpler to understand the bit-shifting.
v2: Add sanity checks on ro=0 and nx=0 never occuring (executable
    read/write code).
    Piggyback on "x86: change modify_xen_mappings to return error"
    to be able to return an error value.
---
 xen/arch/arm/mm.c          | 27 ++++++++++++++++++++++++---
 xen/include/asm-arm/page.h | 11 +++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 4e256c2..d622686 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -836,6 +836,7 @@ static int create_xen_table(lpae_t *entry)
 enum xenmap_operation {
     INSERT,
     REMOVE,
+    MODIFY,
     RESERVE
 };
 
@@ -881,14 +882,28 @@ static int create_xen_entries(enum xenmap_operation op,
                 pte.pt.table = 1;
                 write_pte(&third[third_table_offset(addr)], pte);
                 break;
+            case MODIFY:
             case REMOVE:
                 if ( !third[third_table_offset(addr)].pt.valid )
                 {
-                    printk("create_xen_entries: trying to remove a non-existing mapping addr=%lx\n",
-                           addr);
+                    printk("create_xen_entries: trying to %s a non-existing mapping addr=%lx\n",
+                           op == REMOVE ? "remove" : "modify", addr);
                     return -EINVAL;
                 }
-                pte.bits = 0;
+                if ( op == REMOVE )
+                    pte.bits = 0;
+                else
+                {
+                    pte = third[third_table_offset(addr)];
+                    pte.pt.ro = PTE_RO_MASK(ai);
+                    pte.pt.xn = PTE_NX_MASK(ai);
+                    if ( !pte.pt.ro && !pte.pt.xn )
+                    {
+                        printk("create_xen_entries: Incorrect combination for addr=%lx\n",
+                               addr);
+                        return -EINVAL;
+                    }
+                }
                 write_pte(&third[third_table_offset(addr)], pte);
                 break;
             default:
@@ -922,6 +937,12 @@ void destroy_xen_mappings(unsigned long v, unsigned long e)
     create_xen_entries(REMOVE, v, 0, (e - v) >> PAGE_SHIFT, 0);
 }
 
+int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
+{
+    ASSERT((flags & (PTE_NX | PTE_RO)) == flags);
+    return create_xen_entries(MODIFY, s, 0, (e - s) >> PAGE_SHIFT, flags);
+}
+
 enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
 static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
 {
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 05d9f82..2f66740 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -66,6 +66,17 @@
 #define PAGE_HYPERVISOR_WC      (DEV_WC)
 
 /*
+ * Defines for changing the PTE .ro and .nx bits. This is only to be
+ * used with modify_xen_mappings.
+ */
+#define _PTE_NX_BIT     0U
+#define _PTE_RO_BIT     1U
+#define PTE_NX          (1U << _PTE_NX_BIT)
+#define PTE_RO          (1U << _PTE_RO_BIT)
+#define PTE_NX_MASK(x)  (((x) >> _PTE_NX_BIT) & 0x1U)
+#define PTE_RO_MASK(x)  (((x) >> _PTE_RO_BIT) & 0x1U)
+
+/*
  * Stage 2 Memory Type.
  *
  * These are valid in the MemAttr[3:0] field of an LPAE stage 2 page
-- 
2.4.11


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

[PATCH v2 10/20] arm64/insn: introduce aarch64_insn_gen_{nop|branch_imm}() helper functions

[Konrad Rzeszutek Wilk]

This is copied from Linux 4.7, and the initial commit
that put this in is 5c5bf25d4f7a950382f94fc120a5818197b48fe9
"arm64: introduce aarch64_insn_gen_{nop|branch_imm}() helper functions"

This lays the groundwork for Livepatch to generate the
trampoline to jump to the new replacement function.
Also allows us to NOP the callsites.

This lays the groundwork for Livepatch to generate the
trampoline to jump to the new replacement function.
And also to NOP insns.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
--
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>

RFC: First submission
v1: The full copy of insn_gen_branch instead of just the code to make branch
---
 xen/arch/arm/arm64/insn.c        | 61 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/insn.h | 23 +++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/xen/arch/arm/arm64/insn.c b/xen/arch/arm/arm64/insn.c
index 12b4d96..c5f7e93 100644
--- a/xen/arch/arm/arm64/insn.c
+++ b/xen/arch/arm/arm64/insn.c
@@ -157,6 +157,67 @@ u32 __kprobes aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
 	return insn;
 }
 
+static inline long branch_imm_common(unsigned long pc, unsigned long addr,
+				     long range)
+{
+	long offset;
+
+	if ((pc & 0x3) || (addr & 0x3)) {
+		pr_err("%s: A64 instructions must be word aligned\n", __func__);
+		return range;
+	}
+
+	offset = ((long)addr - (long)pc);
+
+	if (offset < -range || offset >= range) {
+		pr_err("%s: offset out of range\n", __func__);
+		return range;
+	}
+
+	return offset;
+}
+
+u32 __kprobes aarch64_insn_gen_branch_imm(unsigned long pc, unsigned long addr,
+					  enum aarch64_insn_branch_type type)
+{
+	u32 insn;
+	long offset;
+
+	/*
+	 * B/BL support [-128M, 128M) offset
+	 * ARM64 virtual address arrangement guarantees all kernel and module
+	 * texts are within +/-128M.
+	 */
+	offset = branch_imm_common(pc, addr, SZ_128M);
+	if (offset >= SZ_128M)
+		return AARCH64_BREAK_FAULT;
+
+	switch (type) {
+	case AARCH64_INSN_BRANCH_LINK:
+		insn = aarch64_insn_get_bl_value();
+		break;
+	case AARCH64_INSN_BRANCH_NOLINK:
+		insn = aarch64_insn_get_b_value();
+		break;
+	default:
+		pr_err("%s: unknown branch encoding %d\n", __func__, type);
+		return AARCH64_BREAK_FAULT;
+	}
+
+	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_26, insn,
+					     offset >> 2);
+}
+
+u32 __kprobes aarch64_insn_gen_hint(enum aarch64_insn_hint_op op)
+{
+	return aarch64_insn_get_hint_value() | op;
+}
+
+u32 __kprobes aarch64_insn_gen_nop(void)
+{
+	return aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP);
+}
+
 /*
  * Decode the imm field of a branch, and return the byte offset as a
  * signed value (so it can be used when computing a new branch
diff --git a/xen/include/asm-arm/arm64/insn.h b/xen/include/asm-arm/arm64/insn.h
index 6ce37be..c8362e5 100644
--- a/xen/include/asm-arm/arm64/insn.h
+++ b/xen/include/asm-arm/arm64/insn.h
@@ -23,6 +23,15 @@
 #include <xen/types.h>
 #include <xen/stdbool.h>
 
+enum aarch64_insn_hint_op {
+	AARCH64_INSN_HINT_NOP	= 0x0 << 5,
+	AARCH64_INSN_HINT_YIELD	= 0x1 << 5,
+	AARCH64_INSN_HINT_WFE	= 0x2 << 5,
+	AARCH64_INSN_HINT_WFI	= 0x3 << 5,
+	AARCH64_INSN_HINT_SEV	= 0x4 << 5,
+	AARCH64_INSN_HINT_SEVL	= 0x5 << 5,
+};
+
 enum aarch64_insn_imm_type {
 	AARCH64_INSN_IMM_ADR,
 	AARCH64_INSN_IMM_26,
@@ -38,6 +47,14 @@ enum aarch64_insn_imm_type {
 	AARCH64_INSN_IMM_MAX
 };
 
+enum aarch64_insn_branch_type {
+	AARCH64_INSN_BRANCH_NOLINK,
+	AARCH64_INSN_BRANCH_LINK,
+	AARCH64_INSN_BRANCH_RETURN,
+	AARCH64_INSN_BRANCH_COMP_ZERO,
+	AARCH64_INSN_BRANCH_COMP_NONZERO,
+};
+
 #define	__AARCH64_INSN_FUNCS(abbr, mask, val)	\
 static always_inline bool_t aarch64_insn_is_##abbr(u32 code) \
 { return (code & (mask)) == (val); } \
@@ -51,6 +68,7 @@ __AARCH64_INSN_FUNCS(cbnz,	0x7F000000, 0x35000000)
 __AARCH64_INSN_FUNCS(tbz,	0x7F000000, 0x36000000)
 __AARCH64_INSN_FUNCS(tbnz,	0x7F000000, 0x37000000)
 __AARCH64_INSN_FUNCS(bcond,	0xFF000010, 0x54000000)
+__AARCH64_INSN_FUNCS(hint,	0xFFFFF01F, 0xD503201F)
 
 bool aarch64_insn_is_branch_imm(u32 insn);
 
@@ -61,6 +79,11 @@ u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
 s32 aarch64_get_branch_offset(u32 insn);
 u32 aarch64_set_branch_offset(u32 insn, s32 offset);
 
+u32 aarch64_insn_gen_branch_imm(unsigned long pc, unsigned long addr,
+				enum aarch64_insn_branch_type type);
+u32 aarch64_insn_gen_hint(enum aarch64_insn_hint_op op);
+u32 aarch64_insn_gen_nop(void);
+
 /* Wrapper for common code */
 static inline bool insn_is_branch_imm(u32 insn)
 {
-- 
2.4.11


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

[PATCH v2 11/20] arm/arm64: Update comment about VA layout.

[Konrad Rzeszutek Wilk]

It was missing 2MB.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>

v2: First submission. Spun of from 'livepatch: Initial ARM64 support."
---
 xen/include/asm-arm/config.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index a96f845..6772555 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -82,7 +82,7 @@
  *   8M -  10M   Early relocation address (used when relocating Xen)
  *
  * ARM32 layout:
- *   0  -   8M   <COMMON>
+ *   0  -  10M   <COMMON>
  *
  *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
  * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
@@ -93,7 +93,7 @@
  *
  * ARM64 layout:
  * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
- *   0  -   8M   <COMMON>
+ *   0  -  10M   <COMMON>
  *
  *   1G -   2G   VMAP: ioremap and early_ioremap
  *
-- 
2.4.11


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

[PATCH v2 12/20] x86, arm: Change arch_livepatch_quiesce() decleration.

[Konrad Rzeszutek Wilk]

On ARM we need an alternative VA region to poke in the
hypervisor .text data. And since this is setup during runtime
we may fail (it uses vmap so most likely error is ENOMEM).

As such this error needs to be bubbled up and also abort
the livepatching if it occurs.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v2: Initial submission. Spun of from "livepatch: Initial ARM64 support"
---
 xen/arch/arm/livepatch.c    |  3 ++-
 xen/arch/x86/livepatch.c    |  4 +++-
 xen/common/livepatch.c      | 16 ++++++++++++++--
 xen/include/xen/livepatch.h |  2 +-
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index aba1320..755f596 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -7,8 +7,9 @@
 #include <xen/livepatch_elf.h>
 #include <xen/livepatch.h>
 
-void arch_livepatch_quiesce(void)
+int arch_livepatch_quiesce(void)
 {
+    return -ENOSYS;
 }
 
 void arch_livepatch_revive(void)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 67dda07..5491c09 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -16,10 +16,12 @@
 #define PATCH_INSN_SIZE 5
 #define MAX_INSN_SIZE 15
 
-void arch_livepatch_quiesce(void)
+int arch_livepatch_quiesce(void)
 {
     /* Disable WP to allow changes to read-only pages. */
     write_cr0(read_cr0() & ~X86_CR0_WP);
+
+    return 0;
 }
 
 void arch_livepatch_revive(void)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 7e36d97..7d21326 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1088,6 +1088,7 @@ static int livepatch_list(xen_sysctl_livepatch_list_t *list)
 static int apply_payload(struct payload *data)
 {
     unsigned int i;
+    int rc;
 
     printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
             data->name, data->nfuncs);
@@ -1096,7 +1097,12 @@ static int apply_payload(struct payload *data)
     for ( i = 0; i < data->n_bss; i++ )
         memset(data->bss[i], 0, data->bss_size[i]);
 
-    arch_livepatch_quiesce();
+    rc = arch_livepatch_quiesce();
+    if ( rc )
+    {
+        printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
+        return rc;
+    }
 
     /*
      * Since we are running with IRQs disabled and the hooks may call common
@@ -1128,10 +1134,16 @@ static int apply_payload(struct payload *data)
 static int revert_payload(struct payload *data)
 {
     unsigned int i;
+    int rc;
 
     printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name);
 
-    arch_livepatch_quiesce();
+    rc = arch_livepatch_quiesce();
+    if ( rc )
+    {
+        printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
+        return rc;
+    }
 
     for ( i = 0; i < data->nfuncs; i++ )
         arch_livepatch_revert_jmp(&data->funcs[i]);
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 2e64686..6f30c0d 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -72,7 +72,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func);
  * These functions are called around the critical region patching live code,
  * for an architecture to take make appropratie global state adjustments.
  */
-void arch_livepatch_quiesce(void);
+int arch_livepatch_quiesce(void);
 void arch_livepatch_revive(void);
 
 void arch_livepatch_apply_jmp(struct livepatch_func *func);
-- 
2.4.11


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

[PATCH v2 13/20] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk]

As compared to x86 the va of the hypervisor .text
is locked down - we cannot modify the running pagetables
to have the .ro flag unset. We borrow the same idea that
alternative patching has - which is to vmap the entire
.text region and use the alternative virtual address
for patching.

Since we are doing vmap we may fail, hence the
arch_livepatch_quiesce was changed (see "x86,arm:
Change arch_livepatch_quiesce() decleration") to return
an error value which will be bubbled in payload->rc and
provided to the user (along with messages in the ring buffer).

The livepatch virtual address space (where the new functions
are) needs to be close to the hypervisor virtual address
so that the trampoline can reach it. As such we re-use
the BOOT_RELOC_VIRT_START which is not used after bootup
(alternatively we can also use the space after the _end to
FIXMAP_ADDR(0), but that may be too small).

The ELF relocation engine at the start was coded from
the "ELF for the ARM 64-bit Architecture (AArch64)"
(http://infocenter.arm.com/help/topic/com.arm.doc.ihi0056b/IHI0056B_aaelf64.pdf)
but after a while of trying to write the correct bit shifting
and masking from scratch I ended up borrowing from Linux, the
'reloc_insn_imm' (Linux v4.7 arch/arm64/kernel/module.c function.
See 257cb251925f854da435cbf79b140984413871ac "arm64: Loadable modules")

And while at it - we also utilize code from Linux to construct
the right branch instruction (see "arm64/insn: introduce
aarch64_insn_gen_{nop|branch_imm}() helper functions").

In the livepatch payload loading code we tweak the #ifdef to
only exclude ARM_32. The exceptions are not part of ARM 32/64 hence
they are still behind the #ifdef.

We also expand the MAINTAINERS file to include the arm64 and arm32
platform specific livepatch file.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc  Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

RFC: Wholy cow! It works!
v1: Finished the various TODOs and reviews outlined by Julien in RFC.
v2: Call check_for_livepatch_work in leave_hypervisor_tail not in
    reset_stack_and_jump
   - Move ARM 32 components in other patch
   - Blacklist platform options in Kconfig
   - Added R_AARCH64_LDST128_ABS_LO12_NC, R_AARCH64_TSTBR14, and
     R_AARCH64_ADR_PREL_PG_HI21_NC
   - Added do_reloc and reloc_data along with overflow checks from Linux.
   - Use apply_alternatives without any #ifdef
   - Use leave_hypervisor_tail to call check_for_livepatch_work
   - Add ASSERT that isns is not AARCH64_BREAK_FAULT
   - Spun out arch_livepatch_quiesce changes in seperate patch.
   - Spun out changes to config.h (document ones) to seperate patch.
   - Move the livepatch.h to include/xen/asm-arm.
   - Add LIVEPATCH_VMAP_END and LIVEPATCH_VMAP_START in config.h
   - In arch_livepatch_secure use switch for va_type.
   - Drop the #ifndef CONFIG_ARM for .ex_table (see
     "arm/x86: Add HAS_ALTERNATIVE and HAS_EX_TABLE")
   - Piggyback on "x86: change modify_xen_mappings to return error"
      so that arch_livepatch_secure can return errors.
   - Moves comment about branch predictor out of this patch.
---
 MAINTAINERS                     |   1 +
 xen/arch/arm/Makefile           |  13 +-
 xen/arch/arm/arm32/Makefile     |   1 +
 xen/arch/arm/arm32/livepatch.c  |  38 +++++
 xen/arch/arm/arm64/Makefile     |   1 +
 xen/arch/arm/arm64/livepatch.c  | 323 ++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/domain.c           |   6 +
 xen/arch/arm/livepatch.c        | 100 ++++++++++---
 xen/arch/arm/traps.c            |   6 +
 xen/common/Kconfig              |   2 +-
 xen/include/asm-arm/config.h    |   5 +
 xen/include/asm-arm/livepatch.h |  28 ++++
 xen/include/xen/elfstructs.h    |  34 ++++-
 xen/include/xen/types.h         |   6 +
 14 files changed, 539 insertions(+), 25 deletions(-)
 create mode 100644 xen/arch/arm/arm32/livepatch.c
 create mode 100644 xen/arch/arm/arm64/livepatch.c
 create mode 100644 xen/include/asm-arm/livepatch.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 97720a8..ae0b6bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -269,6 +269,7 @@ S:  Supported
 F:  docs/misc/livepatch.markdown
 F:  tools/misc/xen-livepatch.c
 F:  xen/arch/*/livepatch*
+F:  xen/arch/*/*/livepatch*
 F:  xen/common/livepatch*
 F:  xen/include/xen/livepatch*
 
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 0a96713..9f75c5c 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -58,6 +58,15 @@ ALL_OBJS := $(TARGET_SUBARCH)/head.o $(ALL_OBJS)
 
 DEPS += $(TARGET_SUBARCH)/.head.o.d
 
+ifdef CONFIG_LIVEPATCH
+all_symbols = --all-symbols
+ifdef CONFIG_FAST_SYMBOL_LOOKUP
+all_symbols = --all-symbols --sort-by-name
+endif
+else
+all_symbols =
+endif
+
 $(TARGET): $(TARGET)-syms $(TARGET).axf
 	$(OBJCOPY) -O binary -S $< $@
 ifeq ($(CONFIG_ARM_64),y)
@@ -93,12 +102,12 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
 	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
-		| $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0.S
+		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
 	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
-		| $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1.S
+		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
 	    $(@D)/.$(@F).1.o -o $@
diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index b20db64..4395693 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -4,6 +4,7 @@ obj-$(EARLY_PRINTK) += debug.o
 obj-y += domctl.o
 obj-y += domain.o
 obj-y += entry.o
+obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-y += proc-v7.o proc-caxx.o
 obj-y += smpboot.o
 obj-y += traps.o
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
new file mode 100644
index 0000000..c33b68d
--- /dev/null
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -0,0 +1,38 @@
+/*
+ *  Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ */
+
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/livepatch_elf.h>
+#include <xen/livepatch.h>
+
+void arch_livepatch_apply_jmp(struct livepatch_func *func)
+{
+}
+
+void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+{
+}
+
+int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
+{
+    return -EOPNOTSUPP;
+}
+
+int arch_livepatch_perform_rela(struct livepatch_elf *elf,
+                                const struct livepatch_elf_sec *base,
+                                const struct livepatch_elf_sec *rela)
+{
+    return -ENOSYS;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index c1fa43f..149b6b3 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -6,6 +6,7 @@ obj-y += domctl.o
 obj-y += domain.o
 obj-y += entry.o
 obj-y += insn.o
+obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-y += smpboot.o
 obj-y += traps.o
 obj-y += vfp.o
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
new file mode 100644
index 0000000..0809a42
--- /dev/null
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -0,0 +1,323 @@
+/*
+ *  Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ */
+
+#include <xen/bitops.h>
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/livepatch_elf.h>
+#include <xen/livepatch.h>
+#include <xen/mm.h>
+#include <xen/vmap.h>
+
+#include <asm/bitops.h>
+#include <asm/byteorder.h>
+#include <asm/insn.h>
+#include <asm/livepatch.h>
+
+void arch_livepatch_apply_jmp(struct livepatch_func *func)
+{
+    uint32_t insn;
+    uint32_t *old_ptr;
+    uint32_t *new_ptr;
+
+    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
+    BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
+
+    ASSERT(vmap_of_xen_text);
+
+    /* Save old one. */
+    old_ptr = func->old_addr;
+    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
+
+    if ( func->new_addr )
+        insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
+                                           (unsigned long)func->new_addr,
+                                           AARCH64_INSN_BRANCH_NOLINK);
+    else
+        insn = aarch64_insn_gen_nop();
+
+    ASSERT(insn != AARCH64_BREAK_FAULT);
+    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+
+    /* PATCH! */
+    *(new_ptr) = cpu_to_le32(insn);
+
+    clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr));
+}
+
+void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+{
+    uint32_t *new_ptr;
+    uint32_t insn;
+
+    memcpy(&insn, func->opaque, PATCH_INSN_SIZE);
+
+    new_ptr = (uint32_t *)func->old_addr - (u32 *)_start + vmap_of_xen_text;
+
+    /* PATCH! */
+    *(new_ptr) = cpu_to_le32(insn);
+
+    clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr));
+}
+
+int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
+{
+    const Elf_Ehdr *hdr = elf->hdr;
+
+    if ( hdr->e_machine != EM_AARCH64 ||
+         hdr->e_ident[EI_CLASS] != ELFCLASS64 )
+    {
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF Machine type!\n",
+                elf->name);
+        return -EOPNOTSUPP;
+    }
+
+    return 0;
+}
+
+enum aarch64_reloc_op {
+    RELOC_OP_NONE,
+    RELOC_OP_ABS,
+    RELOC_OP_PREL,
+    RELOC_OP_PAGE,
+};
+
+static u64 do_reloc(enum aarch64_reloc_op reloc_op, void *place, u64 val)
+{
+    switch (reloc_op) {
+    case RELOC_OP_ABS:
+        return val;
+
+    case RELOC_OP_PREL:
+        return val - (u64)place;
+
+    case RELOC_OP_PAGE:
+        return (val & ~0xfff) - ((u64)place & ~0xfff);
+
+    case RELOC_OP_NONE:
+        return 0;
+
+    }
+
+    dprintk(XENLOG_DEBUG, LIVEPATCH "do_reloc: unknown relocation operation %d\n", reloc_op);
+    return 0;
+}
+
+static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
+{
+    s64 sval = do_reloc(op, place, val);
+
+    switch (len) {
+    case 16:
+        *(s16 *)place = sval;
+        if (sval < S16_MIN || sval > U16_MAX)
+	        return -EOVERFLOW;
+        break;
+
+    case 32:
+        *(s32 *)place = sval;
+        if (sval < S32_MIN || sval > U32_MAX)
+	        return -EOVERFLOW;
+        break;
+
+    case 64:
+        *(s64 *)place = sval;
+        break;
+
+    default:
+        dprintk(XENLOG_DEBUG, LIVEPATCH "Invalid length (%d) for data relocation\n", len);
+        return 0;
+    }
+
+    return 0;
+}
+
+enum aarch64_insn_movw_imm_type {
+    AARCH64_INSN_IMM_MOVNZ,
+    AARCH64_INSN_IMM_MOVKZ,
+};
+
+static int reloc_insn_imm(enum aarch64_reloc_op op, void *dest, u64 val,
+                          int lsb, int len, enum aarch64_insn_imm_type imm_type)
+{
+    u64 imm, imm_mask;
+    s64 sval;
+    u32 insn = *(u32 *)dest;
+
+    /* Calculate the relocation value. */
+    sval = do_reloc(op, dest, val);
+    sval >>= lsb;
+
+    /* Extract the value bits and shift them to bit 0. */
+    imm_mask = (BIT(lsb + len) - 1) >> lsb;
+    imm = sval & imm_mask;
+
+    /* Update the instruction's immediate field. */
+    insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
+    *(u32 *)dest = insn;
+
+    /*
+     * Extract the upper value bits (including the sign bit) and
+     * shift them to bit 0.
+     */
+    sval = (s64)(sval & ~(imm_mask >> 1)) >> (len - 1);
+
+    /*
+     * Overflow has occurred if the upper bits are not all equal to
+     * the sign bit of the value.
+     */
+    if ((u64)(sval + 1) >= 2)
+        return -EOVERFLOW;
+    return 0;
+}
+
+int arch_livepatch_perform_rela(struct livepatch_elf *elf,
+                                const struct livepatch_elf_sec *base,
+                                const struct livepatch_elf_sec *rela)
+{
+    const Elf_RelA *r;
+    unsigned int symndx, i;
+    uint64_t val;
+    void *dest;
+    bool_t overflow_check;
+
+    for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
+    {
+        int ovf = 0;
+
+        r = rela->data + i * rela->sec->sh_entsize;
+
+        symndx = ELF64_R_SYM(r->r_info);
+
+        if ( symndx > elf->nsym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+
+        dest = base->load_addr + r->r_offset; /* P */
+        val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
+
+        overflow_check = 1;
+
+        /* ARM64 operations at minimum are always 32-bit. */
+        if ( r->r_offset >= base->sec->sh_size ||
+            (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
+            goto bad_offset;
+
+        switch ( ELF64_R_TYPE(r->r_info) ) {
+        /* Data */
+        case R_AARCH64_ABS64:
+            if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
+                goto bad_offset;
+            overflow_check = false;
+            ovf = reloc_data(RELOC_OP_ABS, dest, val, 64);
+            break;
+
+        case R_AARCH64_ABS32:
+            ovf = reloc_data(RELOC_OP_ABS, dest, val, 32);
+            break;
+
+        case R_AARCH64_PREL64:
+            if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
+                goto bad_offset;
+            overflow_check = false;
+            ovf = reloc_data(RELOC_OP_PREL, dest, val, 64);
+            break;
+
+        case R_AARCH64_PREL32:
+            ovf = reloc_data(RELOC_OP_PREL, dest, val, 32);
+            break;
+
+        /* Instructions. */
+        case R_AARCH64_ADR_PREL_LO21:
+            ovf = reloc_insn_imm(RELOC_OP_PREL, dest, val, 0, 21,
+                                 AARCH64_INSN_IMM_ADR);
+            break;
+
+        case R_AARCH64_ADR_PREL_PG_HI21_NC:
+            overflow_check = false;
+        case R_AARCH64_ADR_PREL_PG_HI21:
+            ovf = reloc_insn_imm(RELOC_OP_PAGE, dest, val, 12, 21,
+                                 AARCH64_INSN_IMM_ADR);
+            break;
+
+        case R_AARCH64_LDST8_ABS_LO12_NC:
+        case R_AARCH64_ADD_ABS_LO12_NC:
+            overflow_check = false;
+            ovf = reloc_insn_imm(RELOC_OP_ABS, dest, val, 0, 12,
+                                 AARCH64_INSN_IMM_12);
+            break;
+
+        case R_AARCH64_LDST16_ABS_LO12_NC:
+            overflow_check = false;
+            ovf = reloc_insn_imm(RELOC_OP_ABS, dest, val, 1, 11,
+                                 AARCH64_INSN_IMM_12);
+            break;
+
+        case R_AARCH64_LDST32_ABS_LO12_NC:
+            overflow_check = false;
+            ovf = reloc_insn_imm(RELOC_OP_ABS, dest, val, 2, 10,
+                                 AARCH64_INSN_IMM_12);
+            break;
+
+        case R_AARCH64_LDST64_ABS_LO12_NC:
+            overflow_check = false;
+            ovf = reloc_insn_imm(RELOC_OP_ABS, dest, val, 3, 9,
+                                 AARCH64_INSN_IMM_12);
+            break;
+
+        case R_AARCH64_LDST128_ABS_LO12_NC:
+            overflow_check = false;
+            ovf = reloc_insn_imm(RELOC_OP_ABS, dest, val, 4, 8,
+                                 AARCH64_INSN_IMM_12);
+            break;
+
+        case R_AARCH64_TSTBR14:
+            ovf = reloc_insn_imm(RELOC_OP_PREL, dest, val, 2, 19,
+                                 AARCH64_INSN_IMM_14);
+            break;
+
+        case R_AARCH64_CONDBR19:
+            ovf = reloc_insn_imm(RELOC_OP_PREL, dest, val, 2, 19,
+                                 AARCH64_INSN_IMM_19);
+            break;
+
+        case R_AARCH64_JUMP26:
+        case R_AARCH64_CALL26:
+            ovf = reloc_insn_imm(RELOC_OP_PREL, dest, val, 2, 26,
+                                 AARCH64_INSN_IMM_26);
+            break;
+
+        default:
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation %lu\n",
+                    elf->name, ELF64_R_TYPE(r->r_info));
+            return -EOPNOTSUPP;
+        }
+
+        if ( overflow_check && ovf == -EOVERFLOW )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n",
+                    elf->name, i, rela->name, base->name);
+            return ovf;
+        }
+    }
+    return 0;
+
+ bad_offset:
+    dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation offset is past %s section!\n",
+            elf->name, base->name);
+    return -EINVAL;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 20bb2ba..607ee59 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -13,6 +13,7 @@
 #include <xen/hypercall.h>
 #include <xen/init.h>
 #include <xen/lib.h>
+#include <xen/livepatch.h>
 #include <xen/sched.h>
 #include <xen/softirq.h>
 #include <xen/wait.h>
@@ -55,6 +56,11 @@ void idle_loop(void)
 
         do_tasklet();
         do_softirq();
+        /*
+         * We MUST be last (or before dsb, wfi). Otherwise after we get the
+         * softirq we would execute dsb,wfi (and sleep) and not patch.
+         */
+        check_for_livepatch_work();
     }
 }
 
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 755f596..f49e347 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -6,44 +6,80 @@
 #include <xen/lib.h>
 #include <xen/livepatch_elf.h>
 #include <xen/livepatch.h>
+#include <xen/vmap.h>
+
+#include <asm/livepatch.h>
+#include <asm/mm.h>
+
+void *vmap_of_xen_text;
 
 int arch_livepatch_quiesce(void)
 {
-    return -ENOSYS;
+    mfn_t text_mfn;
+    unsigned int text_order;
+
+    if ( vmap_of_xen_text )
+        return -EINVAL;
+
+    text_mfn = _mfn(virt_to_mfn(_start));
+    text_order = get_order_from_bytes(_end - _start);
+
+    /*
+     * The text section is read-only. So re-map Xen to be able to patch
+     * the code.
+     */
+    vmap_of_xen_text = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
+                              VMAP_DEFAULT);
+
+    if ( !vmap_of_xen_text )
+    {
+        printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! (order=%u)\n",
+               text_order);
+        return -ENOMEM;
+    }
+    return 0;
 }
 
 void arch_livepatch_revive(void)
 {
+    /*
+     * Nuke the instruction cache. Data cache has been cleaned before in
+     * arch_livepatch_apply_jmp.
+     */
+    invalidate_icache();
+
+    if ( vmap_of_xen_text )
+        vunmap(vmap_of_xen_text);
+
+    vmap_of_xen_text = NULL;
 }
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
 {
-    return -ENOSYS;
-}
+    /* If NOPing only do the insn size. */
+    if ( !func->new_addr && func->new_size != PATCH_INSN_SIZE )
+        return -EOPNOTSUPP;
 
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
-{
-}
+    if ( func->old_size < PATCH_INSN_SIZE )
+        return -EINVAL;
 
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
-{
+    return 0;
 }
 
 void arch_livepatch_post_action(void)
 {
+    /* arch_livepatch_revive has nuked the instruction cache. */
 }
 
 void arch_livepatch_mask(void)
 {
+    /* Mask System Error (SError) */
+    local_abort_disable();
 }
 
 void arch_livepatch_unmask(void)
 {
-}
-
-int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
-{
-    return -ENOSYS;
+    local_abort_enable();
 }
 
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
@@ -53,20 +89,42 @@ int arch_livepatch_perform_rel(struct livepatch_elf *elf,
     return -ENOSYS;
 }
 
-int arch_livepatch_perform_rela(struct livepatch_elf *elf,
-                                const struct livepatch_elf_sec *base,
-                                const struct livepatch_elf_sec *rela)
-{
-    return -ENOSYS;
-}
-
 int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
 {
-    return -ENOSYS;
+    unsigned long start = (unsigned long)va;
+    unsigned int flags = 0;
+
+    ASSERT(va);
+    ASSERT(pages);
+
+    switch ( type ) {
+    case LIVEPATCH_VA_RX:
+        flags = PTE_RO; /* R set, NX clear */
+        break;
+
+    case LIVEPATCH_VA_RW:
+        flags = PTE_NX; /* R clear, NX set */
+        break;
+
+    case LIVEPATCH_VA_RO:
+        flags = PTE_NX | PTE_RO; /* R set, NX set */
+        break;
+
+    default:
+        return -EINVAL;
+    }
+
+    return modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
 }
 
 void __init arch_livepatch_init(void)
 {
+    void *start, *end;
+
+    start = (void *)LIVEPATCH_VMAP_START;
+    end = (void *)LIVEPATCH_VMAP_END;
+
+    vm_init_type(VMAP_XEN, start, end);
 }
 
 /*
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 683bcb2..010bb16 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -24,6 +24,7 @@
 #include <xen/symbols.h>
 #include <xen/irq.h>
 #include <xen/lib.h>
+#include <xen/livepatch.h>
 #include <xen/mm.h>
 #include <xen/errno.h>
 #include <xen/hypercall.h>
@@ -2688,6 +2689,11 @@ asmlinkage void leave_hypervisor_tail(void)
         }
         local_irq_enable();
         do_softirq();
+        /*
+         * Must be the last one - as the IPI will trigger us to come here
+         * and we want to patch the hypervisor with almost no stack.
+         */
+        check_for_livepatch_work();
     }
 }
 
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index befa30e..03e4b50 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -225,7 +225,7 @@ config CRYPTO
 config LIVEPATCH
 	bool "Live patching support (TECH PREVIEW)"
 	default n
-	depends on X86 && HAS_BUILD_ID = "y"
+	depends on !ARM_32 && HAS_BUILD_ID = "y"
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
 	  binary patches without rebooting. This is primarily used to binarily
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 6772555..ba61f65 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -80,6 +80,7 @@
  *   4M -   6M   Fixmap: special-purpose 4K mapping slots
  *   6M -   8M   Early boot mapping of FDT
  *   8M -  10M   Early relocation address (used when relocating Xen)
+ *               and later for livepatch vmap (if compiled in)
  *
  * ARM32 layout:
  *   0  -  10M   <COMMON>
@@ -113,6 +114,10 @@
 #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
 #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
 #define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00800000)
+#ifdef CONFIG_LIVEPATCH
+#define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00800000)
+#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
+#endif
 
 #define HYPERVISOR_VIRT_START  XEN_VIRT_START
 
diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h
new file mode 100644
index 0000000..8c8d625
--- /dev/null
+++ b/xen/include/asm-arm/livepatch.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#ifndef __XEN_ARM_LIVEPATCH_H__
+#define __XEN_ARM_LIVEPATCH_H__
+
+/* On ARM32,64 instructions are always 4 bytes long. */
+#define PATCH_INSN_SIZE 4
+
+/*
+ * The va of the hypervisor .text region. We need this as the
+ * normal va are write protected.
+ */
+extern void *vmap_of_xen_text;
+
+#endif /* __XEN_ARM_LIVEPATCH_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index 5f2082e..3746268 100644
--- a/xen/include/xen/elfstructs.h
+++ b/xen/include/xen/elfstructs.h
@@ -177,6 +177,7 @@ typedef struct {
 #define EM_IA_64	50		/* Intel Merced */
 #define EM_X86_64	62		/* AMD x86-64 architecture */
 #define EM_VAX		75		/* DEC VAX */
+#define EM_AARCH64	183		/* ARM 64-bit */
 
 /* Version */
 #define EV_NONE		0		/* Invalid */
@@ -353,12 +354,43 @@ typedef struct {
 #define	ELF64_R_TYPE(info)	((info) & 0xFFFFFFFF)
 #define ELF64_R_INFO(s,t) 	(((s) << 32) + (u_int32_t)(t))
 
-/* x86-64 relocation types. We list only the ones Live Patch implements. */
+/*
+ * Relocation types for x86_64 and ARM 64. We list only the ones Live Patch
+ * implements.
+ */
 #define R_X86_64_NONE		0	/* No reloc */
 #define R_X86_64_64	    	1	/* Direct 64 bit  */
 #define R_X86_64_PC32		2	/* PC relative 32 bit signed */
 #define R_X86_64_PLT32		4	/* 32 bit PLT address */
 
+/*
+ * S - address of symbol.
+ * A - addend for relocation (r_addend)
+ * P - address of the dest being relocated (derieved from r_offset)
+ * NC -  No check for overflow.
+ *
+ * The defines also use _PREL for PC-relative address, and _NC is No Check.
+ */
+#define R_AARCH64_ABS64			257 /* Direct 64 bit. S+A, NC*/
+#define R_AARCH64_ABS32			258 /* Direct 32 bit. S+A */
+#define R_AARCH64_PREL64		260 /* S+A-P, NC */
+#define R_AARCH64_PREL32		261 /* S+A-P */
+
+#define R_AARCH64_ADR_PREL_LO21		274 /* ADR imm, [20:0]. S+A-P */
+#define R_AARCH64_ADR_PREL_PG_HI21	275 /* ADRP imm, [32:12]. Page(S+A) - Page(P).*/
+#define R_AARCH64_ADR_PREL_PG_HI21_NC	276
+#define R_AARCH64_ADD_ABS_LO12_NC	277 /* ADD imm. [11:0]. S+A, NC */
+
+#define R_AARCH64_TSTBR14		279
+#define R_AARCH64_CONDBR19		280 /* Bits 20:2, S+A-P */
+#define R_AARCH64_JUMP26		282 /* Bits 27:2, S+A-P */
+#define R_AARCH64_CALL26		283 /* Bits 27:2, S+A-P */
+#define R_AARCH64_LDST16_ABS_LO12_NC	284 /* LD/ST to bits 11:1, S+A, NC */
+#define R_AARCH64_LDST32_ABS_LO12_NC	285 /* LD/ST to bits 11:2, S+A, NC */
+#define R_AARCH64_LDST64_ABS_LO12_NC	286 /* LD/ST to bits 11:3, S+A, NC */
+#define R_AARCH64_LDST8_ABS_LO12_NC	278 /* LD/ST to bits 11:0, S+A, NC */
+#define R_AARCH64_LDST128_ABS_LO12_NC	299
+
 /* Program Header */
 typedef struct {
 	Elf32_Word	p_type;		/* segment type */
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 7bdc83b..707251c 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -14,6 +14,12 @@
 #define NULL ((void*)0)
 #endif
 
+#define U16_MAX         ((u16)~0U)
+#define S16_MAX         ((s16)(U16_MAX>>1))
+#define S16_MIN         ((s16)(-S16_MAX - 1))
+#define U32_MAX         ((u32)~0U)
+#define S32_MAX         ((s32)(U32_MAX>>1))
+#define S32_MIN         ((s32)(-S32_MAX - 1))
 #define INT_MAX         ((int)(~0U>>1))
 #define INT_MIN         (-INT_MAX - 1)
 #define UINT_MAX        (~0U)
-- 
2.4.11


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

[PATCH v2 14/20] livepatch: ARM 32|64: Ignore mapping symbols: $[d, a, x, t]

[Konrad Rzeszutek Wilk]

Those symbols are used to help final linkers to replace insn.
The ARM ELF specification mandates that they are present
to denote the start of certain CPU features. There are two
variants of it - short and long format.

Either way - we can ignore these symbols.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v1: First submission
v2: Update the order of symbols, fix title
    Add {} in after the first if - per Jan's recommendation.
---
 xen/arch/arm/livepatch.c    | 32 ++++++++++++++++++++++++++++++++
 xen/arch/x86/livepatch.c    |  7 +++++++
 xen/common/livepatch.c      |  2 +-
 xen/include/xen/livepatch.h |  2 ++
 4 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index f49e347..c290602 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -82,6 +82,38 @@ void arch_livepatch_unmask(void)
     local_abort_enable();
 }
 
+int arch_is_payload_symbol(const struct livepatch_elf *elf,
+                           const struct livepatch_elf_sym *sym)
+{
+    /*
+     * - Mapping symbols - denote the "start of a sequence of bytes of the
+     *   appropiate type" to mark certain features - such as start of region
+     *   containing data ($d); ARM ($a), A64 ($x), or Thumb instructions ($t).
+     *
+     * The format is either short: '$x' or long: '$x.<any>'. We do not
+     * need this and more importantly - each payload will contain this
+     * resulting in symbol collisions.
+     */
+    if ( *sym->name == '$' && sym->name[1] != '\0' )
+    {
+        char p = sym->name[1];
+        size_t len = strlen(sym->name);
+
+        if ( (len >= 3 && ( sym->name[2] == '.' )) || (len == 2) )
+        {
+            if ( p == 'd' ||
+#ifdef CONFIG_ARM_32
+                 p == 'a' || p == 't'
+#else
+                 p == 'x'
+#endif
+               )
+                return 0;
+        }
+    }
+    return 1;
+}
+
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
                                const struct livepatch_elf_sec *rela)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 5491c09..f45eccf 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -130,6 +130,13 @@ int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
     return 0;
 }
 
+int arch_is_payload_symbol(const struct livepatch_elf *elf,
+                           const struct livepatch_elf_sym *sym)
+{
+    /* No special checks on x86. */
+    return 1;
+}
+
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
                                const struct livepatch_elf_sec *rela)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 7d21326..d5f069e 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -779,7 +779,7 @@ static bool_t is_payload_symbol(const struct livepatch_elf *elf,
          !strncmp(sym->name, ".L", 2) )
         return 0;
 
-    return 1;
+    return arch_is_payload_symbol(elf, sym);
 }
 
 static int build_symbol_table(struct payload *payload,
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 6f30c0d..d94155f 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -46,6 +46,8 @@ bool_t is_patch(const void *addr);
 /* Arch hooks. */
 int arch_verify_insn_length(unsigned long len);
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
+int arch_is_payload_symbol(const struct livepatch_elf *elf,
+                           const struct livepatch_elf_sym *sym);
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
                                const struct livepatch_elf_sec *rela);
-- 
2.4.11


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

[PATCH v2 15/20] livepatch: Move test-cases to common

[Konrad Rzeszutek Wilk]

So they can be shared with ARM64 (but not yet, so they
are only built on x86).

No functional change.

We also need to tweak the MAINTAINERS and .gitignore file

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v1: First submission
v2: Move to test/livepatch per Jan's recommendation
---
 .gitignore                                                     | 8 ++++----
 MAINTAINERS                                                    | 1 +
 xen/Makefile                                                   | 3 ++-
 xen/arch/arm/Makefile                                          | 3 ---
 xen/arch/x86/Makefile                                          | 5 -----
 xen/test/Makefile                                              | 9 +++++++++
 xen/{arch/x86/test => test/livepatch}/Makefile                 | 0
 xen/{arch/x86/test => test/livepatch}/xen_bye_world.c          | 0
 xen/{arch/x86/test => test/livepatch}/xen_bye_world_func.c     | 0
 xen/{arch/x86/test => test/livepatch}/xen_hello_world.c        | 0
 xen/{arch/x86/test => test/livepatch}/xen_hello_world_func.c   | 0
 xen/{arch/x86/test => test/livepatch}/xen_replace_world.c      | 0
 xen/{arch/x86/test => test/livepatch}/xen_replace_world_func.c | 0
 13 files changed, 16 insertions(+), 13 deletions(-)
 create mode 100644 xen/test/Makefile
 rename xen/{arch/x86/test => test/livepatch}/Makefile (100%)
 rename xen/{arch/x86/test => test/livepatch}/xen_bye_world.c (100%)
 rename xen/{arch/x86/test => test/livepatch}/xen_bye_world_func.c (100%)
 rename xen/{arch/x86/test => test/livepatch}/xen_hello_world.c (100%)
 rename xen/{arch/x86/test => test/livepatch}/xen_hello_world_func.c (100%)
 rename xen/{arch/x86/test => test/livepatch}/xen_replace_world.c (100%)
 rename xen/{arch/x86/test => test/livepatch}/xen_replace_world_func.c (100%)

diff --git a/.gitignore b/.gitignore
index 44cc7bf..4fded28 100644
--- a/.gitignore
+++ b/.gitignore
@@ -255,10 +255,6 @@ xen/arch/x86/efi.lds
 xen/arch/x86/efi/check.efi
 xen/arch/x86/efi/disabled
 xen/arch/x86/efi/mkreloc
-xen/arch/x86/test/config.h
-xen/arch/x86/test/xen_hello_world.livepatch
-xen/arch/x86/test/xen_bye_world.livepatch
-xen/arch/x86/test/xen_replace_world.livepatch
 xen/arch/*/efi/boot.c
 xen/arch/*/efi/compat.c
 xen/arch/*/efi/efi.h
@@ -275,6 +271,10 @@ xen/include/public/public
 xen/include/xen/*.new
 xen/include/xen/acm_policy.h
 xen/include/xen/compile.h
+xen/test/livepatch/config.h
+xen/test/livepatch/xen_bye_world.livepatch
+xen/test/livepatch/xen_hello_world.livepatch
+xen/test/livepatch/xen_replace_world.livepatch
 xen/tools/kconfig/.tmp_gtkcheck
 xen/tools/kconfig/.tmp_qtcheck
 xen/tools/symbols
diff --git a/MAINTAINERS b/MAINTAINERS
index ae0b6bc..160d950 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -271,6 +271,7 @@ F:  tools/misc/xen-livepatch.c
 F:  xen/arch/*/livepatch*
 F:  xen/arch/*/*/livepatch*
 F:  xen/common/livepatch*
+F:  xen/test/livepatch/*
 F:  xen/include/xen/livepatch*
 
 MACHINE CHECK (MCA) & RAS
diff --git a/xen/Makefile b/xen/Makefile
index d68c84d..94ced98 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -80,7 +80,7 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
 
 .PHONY: _tests
 _tests:
-	$(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) tests
+	$(MAKE) -f $(BASEDIR)/Rules.mk -C test tests
 
 .PHONY: _uninstall
 _uninstall: D=$(DESTDIR)
@@ -114,6 +114,7 @@ _clean: delete-unfresh-files
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C xsm clean
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C crypto clean
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) clean
+	$(MAKE) -f $(BASEDIR)/Rules.mk -C test clean
 	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) clean
 	find . \( -name "*.o" -o -name ".*.d" \) -exec rm -f {} \;
 	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 9f75c5c..9dc0797 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -73,9 +73,6 @@ ifeq ($(CONFIG_ARM_64),y)
 	ln -sf $(notdir $@)  ../../$(notdir $@).efi
 endif
 
-.PHONY: tests
-tests:
-
 $(TARGET).axf: $(TARGET)-syms
 	# XXX: VE model loads by VMA so instead of
 	# making a proper ELF we link with LMA == VMA and adjust crudely
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 7209560..b813887 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -92,10 +92,6 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
 	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \
 	`$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
 
-.PHONY: tests
-tests:
-	$(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
-
 ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
 
 ifeq ($(lto),y)
@@ -219,4 +215,3 @@ clean::
 	rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.o efi/.*.d efi/*.efi efi/disabled efi/mkreloc
 	rm -f boot/reloc.S boot/reloc.lnk boot/reloc.bin
 	rm -f note.o
-	$(MAKE) -f $(BASEDIR)/Rules.mk -C test clean
diff --git a/xen/test/Makefile b/xen/test/Makefile
new file mode 100644
index 0000000..8c53040
--- /dev/null
+++ b/xen/test/Makefile
@@ -0,0 +1,9 @@
+.PHONY: tests
+tests:
+ifeq ($(XEN_TARGET_ARCH),x86_64)
+	$(MAKE) -f $(BASEDIR)/Rules.mk -C livepatch livepatch
+endif
+
+.PHONY: clean
+clean::
+	$(MAKE) -f $(BASEDIR)/Rules.mk -C livepatch clean
diff --git a/xen/arch/x86/test/Makefile b/xen/test/livepatch/Makefile
similarity index 100%
rename from xen/arch/x86/test/Makefile
rename to xen/test/livepatch/Makefile
diff --git a/xen/arch/x86/test/xen_bye_world.c b/xen/test/livepatch/xen_bye_world.c
similarity index 100%
rename from xen/arch/x86/test/xen_bye_world.c
rename to xen/test/livepatch/xen_bye_world.c
diff --git a/xen/arch/x86/test/xen_bye_world_func.c b/xen/test/livepatch/xen_bye_world_func.c
similarity index 100%
rename from xen/arch/x86/test/xen_bye_world_func.c
rename to xen/test/livepatch/xen_bye_world_func.c
diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/test/livepatch/xen_hello_world.c
similarity index 100%
rename from xen/arch/x86/test/xen_hello_world.c
rename to xen/test/livepatch/xen_hello_world.c
diff --git a/xen/arch/x86/test/xen_hello_world_func.c b/xen/test/livepatch/xen_hello_world_func.c
similarity index 100%
rename from xen/arch/x86/test/xen_hello_world_func.c
rename to xen/test/livepatch/xen_hello_world_func.c
diff --git a/xen/arch/x86/test/xen_replace_world.c b/xen/test/livepatch/xen_replace_world.c
similarity index 100%
rename from xen/arch/x86/test/xen_replace_world.c
rename to xen/test/livepatch/xen_replace_world.c
diff --git a/xen/arch/x86/test/xen_replace_world_func.c b/xen/test/livepatch/xen_replace_world_func.c
similarity index 100%
rename from xen/arch/x86/test/xen_replace_world_func.c
rename to xen/test/livepatch/xen_replace_world_func.c
-- 
2.4.11


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

[PATCH v2 16/20] livepatch: tests: Make them compile under ARM64

[Konrad Rzeszutek Wilk]

We need to two things:
1) Wrap the platform-specific objcopy parameters in defines
   The input and output parameters for $(OBJCOPY) are different
   based on the platforms. As such provide them in the
   OBJCOPY_MAGIC define and use that.

2) The alternative is a bit different and there are no
   exceptions under ARM (but there are under ARM 64).
   Also use one of the first config options for the CPU
   field feature.

We are not yet attempting to build them under ARM32 so
that is still ifdefed out.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

v1: First submission
v2: Corrected description by Julien
    Add #ifeq instead of #else for ARM case.
---
 xen/test/Makefile                         |  2 +-
 xen/test/livepatch/Makefile               | 11 +++++++++--
 xen/test/livepatch/xen_hello_world_func.c |  8 +++++++-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/xen/test/Makefile b/xen/test/Makefile
index 8c53040..95c1755 100644
--- a/xen/test/Makefile
+++ b/xen/test/Makefile
@@ -1,6 +1,6 @@
 .PHONY: tests
 tests:
-ifeq ($(XEN_TARGET_ARCH),x86_64)
+ifneq $(XEN_TARGET_ARCH),arm32)
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C livepatch livepatch
 endif
 
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 23dff1d..ce09e1d 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -1,5 +1,12 @@
 include $(XEN_ROOT)/Config.mk
 
+ifeq ($(XEN_TARGET_ARCH),x86_64)
+OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
+endif
+ifeq ($(XEN_TARGET_ARCH),arm64)
+OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64
+endif
+
 CODE_ADDR=$(shell nm --defined $(1) | grep $(2) | awk '{print "0x"$$1}')
 CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}')
 
@@ -54,7 +61,7 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
 .PHONY: note.o
 note.o:
 	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin
-	$(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \
+	$(OBJCOPY) $(OBJCOPY_MAGIC) \
 		   --rename-section=.data=.livepatch.depends -S $@.bin $@
 	rm -f $@.bin
 
@@ -65,7 +72,7 @@ note.o:
 .PHONY: hello_world_note.o
 hello_world_note.o: $(LIVEPATCH)
 	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@.bin
-	$(OBJCOPY)  -I binary -O elf64-x86-64 -B i386:x86-64 \
+	$(OBJCOPY) $(OBJCOPY_MAGIC) \
 		   --rename-section=.data=.livepatch.depends -S $@.bin $@
 	rm -f $@.bin
 
diff --git a/xen/test/livepatch/xen_hello_world_func.c b/xen/test/livepatch/xen_hello_world_func.c
index 03d6b84..a357ebf 100644
--- a/xen/test/livepatch/xen_hello_world_func.c
+++ b/xen/test/livepatch/xen_hello_world_func.c
@@ -6,14 +6,17 @@
 #include <xen/types.h>
 
 #include <asm/alternative.h>
+#ifdef CONFIG_X86
 #include <asm/nops.h>
 #include <asm/uaccess.h>
 
 static unsigned long *non_canonical_addr = (unsigned long *)0xdead000000000000ULL;
+#endif
 
 /* Our replacement function for xen_extra_version. */
 const char *xen_hello_world(void)
 {
+#ifdef CONFIG_X86
     unsigned long tmp;
     int rc;
 
@@ -24,7 +27,10 @@ const char *xen_hello_world(void)
      */
     rc = __get_user(tmp, non_canonical_addr);
     BUG_ON(rc != -EFAULT);
-
+#endif
+#ifdef CONFIG_ARM_64
+     asm(ALTERNATIVE("nop", "nop", ARM64_WORKAROUND_CLEAN_CACHE));
+#endif
     return "Hello World";
 }
 
-- 
2.4.11


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

[PATCH v2 17/20] xen/arm32: Add an helper to invalidate all instruction caches

[Konrad Rzeszutek Wilk]

This is exactly like commit fb9d877a9c0f3d4d15db8f6e0c5506ea641862c6
"xen/arm64: Add an helper to invalidate all instruction caches"
except it is on ARM32 side.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>

v2: First submission
---
 xen/include/asm-arm/arm32/page.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index bccdbfc..26184ec 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -29,6 +29,12 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
  * inline asm operand) */
 #define __clean_and_invalidate_dcache_one(R) STORE_CP32(R, DCCIMVAC)
 
+/* Invalidate all instruction caches in Inner Shareable domain to PoU */
+static inline void invalidate_icache(void)
+{
+    asm volatile (CMD_CP32(ICIALLUIS));
+}
+
 /*
  * Flush all hypervisor mappings from the TLB and branch predictor of
  * the local processor.
-- 
2.4.11


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

[PATCH v2 18/20] xen/arm32/livepatch: Add BPICALLIS to helper to invalidate all instruction caches

[Konrad Rzeszutek Wilk]

When we are flushing the cache we are most likley also want
to flush the branch predictor too. Hence add this.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>

v2: First submission
---
 xen/arch/arm/livepatch.c         | 3 +++
 xen/include/asm-arm/arm32/page.h | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index c290602..cdb8d65 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -45,6 +45,9 @@ void arch_livepatch_revive(void)
     /*
      * Nuke the instruction cache. Data cache has been cleaned before in
      * arch_livepatch_apply_jmp.
+     *
+     * Need to flush the branch predictor for ARMv7 as it may be
+     * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b).
      */
     invalidate_icache();
 
diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index 26184ec..6caf596 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -32,7 +32,10 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
 /* Invalidate all instruction caches in Inner Shareable domain to PoU */
 static inline void invalidate_icache(void)
 {
-    asm volatile (CMD_CP32(ICIALLUIS));
+    asm volatile (
+        CMD_CP32(ICIALLUIS)     /* Flush I-cache. */
+        CMD_CP32(BPIALLIS)      /* Flush branch predictor. */
+        : : : "memory");
 }
 
 /*
-- 
2.4.11


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

[PATCH v2 19/20] livepatch/elf: Adjust section aligment to word

[Konrad Rzeszutek Wilk]

On most architectures it does not matter what the aligment is.

On ARM32 it is paramount that the aligment is word-size (4)
otherwise we get a Data Abort when trying to perform ELF
relocations. That is due to ARM 32 only being able to write to
word-aligned addresses.

The default section alignments for .bug.frame, .livepatch.depends,
,.rodata.str1, and .strtab are 1 unless we:

 - Provide our own linker script file.
 - Piggyback on Xen's (but that means lots of #ifdefery)
 - Throw an error and refuse the payload

Or just fix it up.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v2: First submission
---
 xen/common/livepatch_elf.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index cda9b27..2392bdf 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -71,7 +71,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
         delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize;
 
         sec[i].sec = data + delta;
-
+        /*
+         * Some architectures REQUIRE section alignment to be word-size.
+         */
+        if ( sec[i].sec->sh_addralign % sizeof(uint32_t) )
+        {
+            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Adjusting aligment for section [%u]\n",
+                    elf->name, i);
+            ((Elf_Shdr *)sec[i].sec)->sh_addralign = 4;
+        }
         delta = sec[i].sec->sh_offset;
         /*
          * N.B. elf_resolve_section_names, elf_get_sym skip this check as
-- 
2.4.11


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

[PATCH v2 20/20] livepatch: ARM32 support.

[Konrad Rzeszutek Wilk]

The patch piggybacks on: livepatch: Initial ARM64 support, which
brings up all of the neccessary livepatch infrastructure pieces in.

This patch adds three major pieces:

 1) ELF relocations. ARM32 uses SHT_REL instead of SHT_RELA which
    means the adddendum had to be extracted from within the
    instruction. Which required parsing BL/BLX, B/BL<cond>,
    MOVT, and MOVW instructions.

    The code was written from scratch using the ARM ELF manual
    (and the ARM Architecture Reference Manual)

 2) Inserting an trampoline. We use the B (branch to address)
    which uses an offset that is based on the PC value: PC + imm32.
    Because we insert the branch at the start of the old function
    we have to account for the instruction already being fetched
    and subtract -4 from the delta (new_addr - old_addr).

 3) Allows the test-cases to be built under ARM 32.
    The "livepatch: tests: Make them compile under ARM64"
    put in the right infrastructure for it and we piggyback on it.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>

v2: First submission.
---
 xen/arch/arm/arm32/livepatch.c | 252 ++++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/arm64/livepatch.c |   7 ++
 xen/arch/arm/livepatch.c       |   7 --
 xen/common/Kconfig             |   2 +-
 xen/include/xen/elfstructs.h   |  24 +++-
 xen/test/Makefile              |   2 -
 xen/test/livepatch/Makefile    |   3 +
 7 files changed, 284 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index c33b68d..63e450b 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -3,28 +3,276 @@
  */
 
 #include <xen/errno.h>
+#include <xen/kernel.h>
 #include <xen/lib.h>
 #include <xen/livepatch_elf.h>
 #include <xen/livepatch.h>
 
+#include <asm/page.h>
+#include <asm/livepatch.h>
+
 void arch_livepatch_apply_jmp(struct livepatch_func *func)
 {
+    uint32_t insn;
+    uint32_t *old_ptr;
+    uint32_t *new_ptr;
+
+    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
+    BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
+
+    ASSERT(vmap_of_xen_text);
+
+    /* Save old one. */
+    old_ptr = func->old_addr;
+    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
+
+    if ( func->new_addr )
+    {
+        s32 delta;
+
+        /*
+         * The -4 is to account for the b <offset> instruction placed at
+         * the start of the func->old_addr.
+         */
+        delta = (s32)(func->new_addr - func->old_addr - 4);
+        /* CPU shifts by two (left) when decoding, so we shift right by two. */
+        delta = delta >> 2;
+        /* No thumb code and lets not modify the cond. */
+        delta &= 0x00FFFFFE;
+
+        insn = 0xea000000 | (uint32_t)delta;
+    }
+    else
+        insn = 0xe1a00000; /* mov r0, r0 */
+
+    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+
+    /* PATCH! */
+    *(new_ptr) = insn;
+
+    clean_and_invalidate_dcache_va_range(func->old_addr, sizeof(*new_ptr));
 }
 
 void arch_livepatch_revert_jmp(const struct livepatch_func *func)
 {
+    uint32_t *new_ptr;
+    uint32_t insn;
+
+    memcpy(&insn, func->opaque, PATCH_INSN_SIZE);
+
+    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+
+    /* PATCH! */
+    *(new_ptr) = insn;
+
+    clean_and_invalidate_dcache_va_range(func->old_addr, sizeof(*new_ptr));
 }
 
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
 {
-    return -EOPNOTSUPP;
+    const Elf_Ehdr *hdr = elf->hdr;
+
+    if ( hdr->e_machine != EM_ARM ||
+         hdr->e_ident[EI_CLASS] != ELFCLASS32 )
+    {
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF Machine type!\n",
+                elf->name);
+        return -EOPNOTSUPP;
+    }
+
+    if ( (hdr->e_flags & EF_ARM_EABI_MASK) != EF_ARM_EABI_VER5 )
+    {
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF EABI(%x)!\n",
+                elf->name, hdr->e_flags);
+        return -EOPNOTSUPP;
+    }
+
+    return 0;
+}
+
+static s32 get_addend(unsigned char type, void *dest)
+{
+    s32 addend = 0;
+
+    switch ( type ) {
+    case R_ARM_NONE:
+        /* ignore */
+        break;
+
+    case R_ARM_ABS32:
+        addend = *(u32 *)dest;
+        break;
+
+    case R_ARM_REL32:
+        addend = *(u32 *)dest;
+        break;
+
+    case R_ARM_MOVW_ABS_NC:
+    case R_ARM_MOVT_ABS:
+        addend =  (*(u32 *)dest & 0x00000FFF);
+        addend |= (*(u32 *)dest & 0x000F0000) >> 4;
+        /* Addend is to sign-extend ([19:16],[11:0]). */
+        addend = (s16)addend;
+        break;
+
+    case R_ARM_CALL:
+    case R_ARM_JUMP24:
+        /* Addend = sign_extend (insn[23:0]) << 2 */
+        addend = ((*(u32 *)dest & 0xFFFFFF) ^ 0x800000) - 0x800000;
+        addend = addend << 2;
+        break;
+    }
+
+    return addend;
+}
+
+static int perform_rel(unsigned char type, void *dest, uint32_t val, s32 addend)
+{
+
+    switch ( type ) {
+    case R_ARM_NONE:
+        /* ignore */
+        break;
+
+    case R_ARM_ABS32: /* (S + A) | T */
+        *(u32 *)dest = (val + addend);
+        break;
+
+    case R_ARM_REL32: /* ((S + A) | T) – P */
+        *(u32 *)dest = (val + addend) - (uint32_t)dest;
+        break;
+
+    case R_ARM_MOVW_ABS_NC: /* S + A */
+    case R_ARM_MOVT_ABS: /* S + A */
+        /* Clear addend if needed . */
+        if ( addend )
+            *(u32 *)dest &= 0xFFF0F000;
+
+        if ( type == R_ARM_MOVT_ABS )
+        {
+            /*
+             * Almost the same as MOVW except it uses the 16 bit
+             * high value. Putting it in insn requires shifting right by
+             * 16-bit (as we only have 16-bit for imm.
+             */
+            val &= 0xFFFF0000; /* ResultMask */
+            val = val >> 16;
+        }
+        else
+        {
+            /* MOVW loads 16 bits into the bottom half of a register. */
+            val &= 0xFFFF;
+        }
+        /* [11:0] = Result_Mask(X) & 0xFFF,[19:16] = Result_Mask(X) >> 12 */
+        *(u32 *)dest |= val & 0xFFF;
+        *(u32 *)dest |= (val >> 12) << 16;
+        break;
+
+    case R_ARM_CALL:
+    case R_ARM_JUMP24: /* (S + A) - P */
+        /* Clear the old addend. */
+        if ( addend )
+            *(u32 *)dest &= 0xFF000000;
+
+        val += addend - (uint32_t)dest;
+
+#define NEGATIVE_32M ((s32)(-MB(32) - 1))
+        if ( (s32)val < NEGATIVE_32M || (s32)val > (s32)MB(32) )
+            return -EOVERFLOW;
+
+        /* CPU always shifts insn by two, so complement it. */
+        val = val >> 2;
+        val &= 0x00FFFFFE;
+        *(u32 *)dest |= (uint32_t)val;
+        break;
+
+    default:
+         return -EOPNOTSUPP;
+    }
+
+    return 0;
+}
+
+int arch_livepatch_perform(struct livepatch_elf *elf,
+                           const struct livepatch_elf_sec *base,
+                           const struct livepatch_elf_sec *rela,
+                           bool use_rela)
+{
+    const Elf_RelA *r_a;
+    const Elf_Rel *r;
+    unsigned int symndx, i;
+    uint32_t val;
+    void *dest;
+    int rc = 0;
+
+    for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
+    {
+        unsigned char type;
+        s32 addend = 0;
+
+        if ( use_rela )
+        {
+            r_a = rela->data + i * rela->sec->sh_entsize;
+            symndx = ELF32_R_SYM(r_a->r_info);
+            type = ELF32_R_TYPE(r_a->r_info);
+            dest = base->load_addr + r_a->r_offset; /* P */
+            addend = r_a->r_addend;
+        }
+        else
+        {
+            r = rela->data + i * rela->sec->sh_entsize;
+            symndx = ELF32_R_SYM(r->r_info);
+            type = ELF32_R_TYPE(r->r_info);
+            dest = base->load_addr + r->r_offset; /* P */
+        }
+
+        if ( symndx > elf->nsym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants symbol@%u which is past end!\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+
+        if ( !use_rela )
+            addend = get_addend(type, dest);
+
+        val = elf->sym[symndx].sym->st_value; /* S */
+
+        rc = perform_rel(type, dest, val, addend);
+        switch ( rc ) {
+        case -EOVERFLOW:
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n",
+                    elf->name, i, rela->name, base->name);
+            break;
+
+        case -EOPNOTSUPP:
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation #%x\n",
+                    elf->name, type);
+            break;
+
+        default:
+            break;
+        }
+
+        if ( rc )
+            break;
+    }
+
+    return rc;
+}
+
+int arch_livepatch_perform_rel(struct livepatch_elf *elf,
+                               const struct livepatch_elf_sec *base,
+                               const struct livepatch_elf_sec *rela)
+{
+    return arch_livepatch_perform(elf, base, rela, false);
 }
 
 int arch_livepatch_perform_rela(struct livepatch_elf *elf,
                                 const struct livepatch_elf_sec *base,
                                 const struct livepatch_elf_sec *rela)
 {
-    return -ENOSYS;
+    return arch_livepatch_perform(elf, base, rela, true);
 }
 
 /*
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 0809a42..660fdf9 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -172,6 +172,13 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, void *dest, u64 val,
     return 0;
 }
 
+int arch_livepatch_perform_rel(struct livepatch_elf *elf,
+                               const struct livepatch_elf_sec *base,
+                               const struct livepatch_elf_sec *rela)
+{
+    return -ENOSYS;
+}
+
 int arch_livepatch_perform_rela(struct livepatch_elf *elf,
                                 const struct livepatch_elf_sec *base,
                                 const struct livepatch_elf_sec *rela)
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index cdb8d65..00fe41f 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -117,13 +117,6 @@ int arch_is_payload_symbol(const struct livepatch_elf *elf,
     return 1;
 }
 
-int arch_livepatch_perform_rel(struct livepatch_elf *elf,
-                               const struct livepatch_elf_sec *base,
-                               const struct livepatch_elf_sec *rela)
-{
-    return -ENOSYS;
-}
-
 int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
 {
     unsigned long start = (unsigned long)va;
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 03e4b50..b1361d7 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -225,7 +225,7 @@ config CRYPTO
 config LIVEPATCH
 	bool "Live patching support (TECH PREVIEW)"
 	default n
-	depends on !ARM_32 && HAS_BUILD_ID = "y"
+	depends on HAS_BUILD_ID = "y"
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
 	  binary patches without rebooting. This is primarily used to binarily
diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index 3746268..43cec03 100644
--- a/xen/include/xen/elfstructs.h
+++ b/xen/include/xen/elfstructs.h
@@ -103,6 +103,15 @@ typedef uint64_t	Elf64_Xword;
                       (ehdr).e_ident[EI_MAG2] == ELFMAG2 && \
                       (ehdr).e_ident[EI_MAG3] == ELFMAG3)
 
+/* e_flags */
+#define EF_ARM_EABI_MASK	0xff000000
+#define EF_ARM_EABI_UNKNOWN	0x00000000
+#define EF_ARM_EABI_VER1	0x01000000
+#define EF_ARM_EABI_VER2	0x02000000
+#define EF_ARM_EABI_VER3	0x03000000
+#define EF_ARM_EABI_VER4	0x04000000
+#define EF_ARM_EABI_VER5	0x05000000
+
 /* ELF Header */
 typedef struct elfhdr {
 	unsigned char	e_ident[EI_NIDENT]; /* ELF Identification */
@@ -364,9 +373,22 @@ typedef struct {
 #define R_X86_64_PLT32		4	/* 32 bit PLT address */
 
 /*
+ * ARM32 relocation types. See
+ * http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf
  * S - address of symbol.
- * A - addend for relocation (r_addend)
+ * A - addend for relocation (r_addend or need to extract from insn)
  * P - address of the dest being relocated (derieved from r_offset)
+ */
+#define R_ARM_NONE              0
+#define R_ARM_ABS32             2	/* Direct 32-bit. S+A */
+#define R_ARM_REL32             3	/* PC relative. S+A */
+#define R_ARM_CALL              28	/* SignExtend([23:0]) << 2. S+A-P */
+#define R_ARM_JUMP24            29	/* Same as R_ARM_CALL */
+#define R_ARM_MOVW_ABS_NC       43	/* SignExtend([19:16],[11:0])&0xFFFF, S+A */
+#define R_ARM_MOVT_ABS          44	/* SignExtend([19:16],[11:0))&0xFFFF0000 */
+					/*  >> 16, S+A. */
+
+/*
  * NC -  No check for overflow.
  *
  * The defines also use _PREL for PC-relative address, and _NC is No Check.
diff --git a/xen/test/Makefile b/xen/test/Makefile
index 95c1755..d91b319 100644
--- a/xen/test/Makefile
+++ b/xen/test/Makefile
@@ -1,8 +1,6 @@
 .PHONY: tests
 tests:
-ifneq $(XEN_TARGET_ARCH),arm32)
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C livepatch livepatch
-endif
 
 .PHONY: clean
 clean::
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index ce09e1d..bfd63c2 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -6,6 +6,9 @@ endif
 ifeq ($(XEN_TARGET_ARCH),arm64)
 OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64
 endif
+ifeq ($(XEN_TARGET_ARCH),arm32)
+OBJCOPY_MAGIC := -I binary -O elf32-littlearm -B arm
+endif
 
 CODE_ADDR=$(shell nm --defined $(1) | grep $(2) | awk '{print "0x"$$1}')
 CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}')
-- 
2.4.11


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

Re: [PATCH v2 08/20] x86: change modify_xen_mappings to return error

[Andrew Cooper]

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:

> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 58bc0b8..f257bbc 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -146,7 +146,7 @@ int map_pages_to_xen(
>       unsigned long nr_mfns,
>       unsigned int flags);
>   /* Alter the permissions of a range of Xen virtual address space. */
> -void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags);
> +int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags);
>   void destroy_xen_mappings(unsigned long v, unsigned long e);

destroy_xen_mappings() should also be changed to match, as its common.

With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 04/20] alternatives: x86 rename and change parameters on ARM

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 545 bytes --]

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:

> On x86 we squash 'apply_alternatives' in to
> 'alternative_instructions' (who was its sole user)
> and 'apply_alternatives_nocheck' to 'apply_alternatives'.
>
> On ARM we change the parameters for 'apply_alternatives'
> to be of 'const struct alt_instr *' instead of void pointer and
> size length.
>
> We also add 'const' and make the arguments be on the
> proper offset.
>
> Signed-off-by: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[-- Attachment #1.2: Type: text/html, Size: 1069 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Andrew Cooper]

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:

> x86 implements all of them by default - and we just
> add two extra CONFIG_ variables to be declared in autoconf.h.
>
> ARM 64 only has alternative while ARM 32 has none of them.
>
> And while at it change the livepatch common code that
> would benefit from this.
>
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Surely livepatch should select alternatives ?

~Andrew

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

Re: [PATCH v2 12/20] x86, arm: Change arch_livepatch_quiesce() decleration.

[Andrew Cooper]

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:

> On ARM we need an alternative VA region to poke in the
> hypervisor .text data. And since this is setup during runtime
> we may fail (it uses vmap so most likely error is ENOMEM).
>
> As such this error needs to be bubbled up and also abort
> the livepatching if it occurs.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Julien Grall]

Hi Andrew,

On 25/08/2016 09:58, Andrew Cooper wrote:
> On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
>
>> x86 implements all of them by default - and we just
>> add two extra CONFIG_ variables to be declared in autoconf.h.
>>
>> ARM 64 only has alternative while ARM 32 has none of them.
>>
>> And while at it change the livepatch common code that
>> would benefit from this.
>>
>> Suggested-by: Julien Grall <julien.grall@arm.com>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> Surely livepatch should select alternatives ?

Do you mean for ARM? If so, alternatives is not supported by ARM32 so it 
should not be selected.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 14/20] livepatch: ARM 32|64: Ignore mapping symbols: $[d, a, x, t]

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 450 bytes --]

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> Those symbols are used to help final linkers to replace insn.
> The ARM ELF specification mandates that they are present
> to denote the start of certain CPU features. There are two
> variants of it - short and long format.
>
> Either way - we can ignore these symbols.
>
> Signed-off-by: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>

x86 bits Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[-- Attachment #1.2: Type: text/html, Size: 1022 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Andrew Cooper]

On 25/08/16 15:02, Julien Grall wrote:
> Hi Andrew,
>
> On 25/08/2016 09:58, Andrew Cooper wrote:
>> On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
>>
>>> x86 implements all of them by default - and we just
>>> add two extra CONFIG_ variables to be declared in autoconf.h.
>>>
>>> ARM 64 only has alternative while ARM 32 has none of them.
>>>
>>> And while at it change the livepatch common code that
>>> would benefit from this.
>>>
>>> Suggested-by: Julien Grall <julien.grall@arm.com>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> Surely livepatch should select alternatives ?
>
> Do you mean for ARM? If so, alternatives is not supported by ARM32 so 
> it should not be selected.

Perhaps we need a "selects ALTERNATIVES if X86" then, but the x86 
implementation of livepatching does strictly need alternatives support, 
and should be represented appropriately.

~Andrew

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

Re: [PATCH v2 01/20] livepatch: Bubble up sanity checks on Elf relocs

[Jan Beulich]

>>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
> The checks for SHT_REL[,A] ELF sanity checks does not need to
> be in the platform specific file and can be bubbled up
> in the platform agnostic file.
> 
> This makes the ARM 32/64 implementation easier as the
> duplicate checks don't have to be in the platform specific files.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

x86 part:
Acked-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Jan Beulich]

>>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
> @@ -22,6 +24,12 @@ config X86
>  	select NUMA
>  	select VGA
>  
> +config ALTERNATIVE
> +	bool
> +
> +config HAS_EX_TABLE
> +	bool

These need to move out of arch/x86/, and the first one's name is
too generic (and should probably also start with HAS_).

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -3,6 +3,7 @@
>   *
>   */
>  
> +#include <xen/config.h>
>  #include <xen/cpu.h>
>  #include <xen/elf.h>
>  #include <xen/err.h>

No new explicit inclusions of xen/config.h please.

Jan


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

Re: [PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Jan Beulich]

>>> On 25.08.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
> On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> 
>> x86 implements all of them by default - and we just
>> add two extra CONFIG_ variables to be declared in autoconf.h.
>>
>> ARM 64 only has alternative while ARM 32 has none of them.
>>
>> And while at it change the livepatch common code that
>> would benefit from this.
>>
>> Suggested-by: Julien Grall <julien.grall@arm.com>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Surely livepatch should select alternatives ?

DYM depend on? It clearly shouldn't select them, as whether
they're present is determined by arch code.

Jan


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

Re: [PATCH v2 13/20] livepatch: Initial ARM64 support.

[Jan Beulich]

>>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -14,6 +14,12 @@
>  #define NULL ((void*)0)
>  #endif
>  
> +#define U16_MAX         ((u16)~0U)
> +#define S16_MAX         ((s16)(U16_MAX>>1))
> +#define S16_MIN         ((s16)(-S16_MAX - 1))
> +#define U32_MAX         ((u32)~0U)
> +#define S32_MAX         ((s32)(U32_MAX>>1))
> +#define S32_MIN         ((s32)(-S32_MAX - 1))

These are rather strange constants: Fixed width types necessarily
always have the same boundaries of representable values. Otoh
the C standard has such constants too - maybe if we really want
them we should rather use their names?

Jan


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

Re: [PATCH v2 15/20] livepatch: Move test-cases to common

[Jan Beulich]

>>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
> So they can be shared with ARM64 (but not yet, so they
> are only built on x86).
> 
> No functional change.
> 
> We also need to tweak the MAINTAINERS and .gitignore file
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

for whichever parts it's relevant
Acked-by: Jan Beulich <jbeulich@suse.com>
with one adjustment request:

> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -271,6 +271,7 @@ F:  tools/misc/xen-livepatch.c
>  F:  xen/arch/*/livepatch*
>  F:  xen/arch/*/*/livepatch*
>  F:  xen/common/livepatch*
> +F:  xen/test/livepatch/*
>  F:  xen/include/xen/livepatch*

Please keep this sorted.

Jan


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

Re: [PATCH v2 19/20] livepatch/elf: Adjust section aligment to word

[Jan Beulich]

>>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
> On most architectures it does not matter what the aligment is.
> 
> On ARM32 it is paramount that the aligment is word-size (4)
> otherwise we get a Data Abort when trying to perform ELF
> relocations. That is due to ARM 32 only being able to write to
> word-aligned addresses.

That's not exactly true, afaik: ARM can write to byte- and
half-word-aligned addresses, but only bytes/half-words.

> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -71,7 +71,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
>          delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize;
>  
>          sec[i].sec = data + delta;
> -
> +        /*
> +         * Some architectures REQUIRE section alignment to be word-size.
> +         */

This is a single line comment.

> +        if ( sec[i].sec->sh_addralign % sizeof(uint32_t) )

Hmm, word size for ARM64 and x86-64 ought to be 8 bytes. Also you
don't cover sec[i].sec->sh_addralign being zero (in fact any non-power-
of-2 value would seem bogus to me). And then - why does this need to
be done to all sections?

> +        {
> +            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Adjusting aligment for section [%u]\n",
> +                    elf->name, i);
> +            ((Elf_Shdr *)sec[i].sec)->sh_addralign = 4;

And of course you know how I like such casting away of constness.

Jan


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

Re: [PATCH v2] Livepatch for ARM 64 and 32.

[Julien Grall]

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> Hey!

Hi Konrad,

> Since v1 (and RFC): [https://lists.xen.org/archives/html/xen-devel/2016-08/msg01835.html]
>  - Acted on most all comments.
>  - Added ARM32 support.
>
> The patches are based on: [PATCH v4] Livepatch fixes and features for v4.8.
> (https://lists.xen.org/archives/html/xen-devel/2016-08/msg02705.html)
>
> And the git tree is:
>  git://xenbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v4
>
> There are two outstanding questions that should be addressed at some point:
>  -  #16 "livepatch: tests: Make them compile under ARM64"
>     We chatted about adding a specific CPU bit (LIVEPATCH) so that alternative
>     code always gets exercised. This being for both x86 and ARM. But this has
>     a side effect that it gets exposed to the toolstack and can also be
>     exposed to the guests. I think it is better if we do not do that
>     so left it to be enabled based on the most common errata (or feature
>     on x86).

There is no common errata on ARM. I spoke with Andrew which mentioned 
that on x86 they have architectural xen feature bits which are not 
exposed to the toolstack/guest.

Furthermore, I think it would be unwise to let the toolstack aware of 
some errata and possibly setting/clearing them. So adding a specific 
feature bit is fine by me.

>  -  #13 "livepatch: Initial ARM64 support."
>     Need to look in erratum #843419 on some Cortex-A53 and figuring
>     out how to avoid payloads having R_AARCH64_ADR_PREL_PG_HI21 relocations.

I will not considered this has a blocker for this series. Having 
livepatching on all the other boards for Xen 4.8 would still be awesome :).

Regards,

-- 
Julien Grall

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

Re: [PATCH v2] Livepatch for ARM 64 and 32.

[Konrad Rzeszutek Wilk]

On Wed, Aug 31, 2016 at 03:49:55PM +0100, Julien Grall wrote:
> On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> > Hey!
> 
> Hi Konrad,
> 
> > Since v1 (and RFC): [https://lists.xen.org/archives/html/xen-devel/2016-08/msg01835.html]
> >  - Acted on most all comments.
> >  - Added ARM32 support.
> > 
> > The patches are based on: [PATCH v4] Livepatch fixes and features for v4.8.
> > (https://lists.xen.org/archives/html/xen-devel/2016-08/msg02705.html)
> > 
> > And the git tree is:
> >  git://xenbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v4
> > 
> > There are two outstanding questions that should be addressed at some point:
> >  -  #16 "livepatch: tests: Make them compile under ARM64"
> >     We chatted about adding a specific CPU bit (LIVEPATCH) so that alternative
> >     code always gets exercised. This being for both x86 and ARM. But this has
> >     a side effect that it gets exposed to the toolstack and can also be
> >     exposed to the guests. I think it is better if we do not do that
> >     so left it to be enabled based on the most common errata (or feature
> >     on x86).
> 
> There is no common errata on ARM. I spoke with Andrew which mentioned that
> on x86 they have architectural xen feature bits which are not exposed to the
> toolstack/guest.
> 
> Furthermore, I think it would be unwise to let the toolstack aware of some
> errata and possibly setting/clearing them. So adding a specific feature bit
> is fine by me.

So you are thinking of exposing this 'xen feature bits' that Andrew mention
to be on ARM as well.

Andrew, is this the 'Hypervisor' one?

> 
> >  -  #13 "livepatch: Initial ARM64 support."
> >     Need to look in erratum #843419 on some Cortex-A53 and figuring
> >     out how to avoid payloads having R_AARCH64_ADR_PREL_PG_HI21 relocations.
> 
> I will not considered this has a blocker for this series. Having
> livepatching on all the other boards for Xen 4.8 would still be awesome :).
> 
> Regards,
> 
> -- 
> Julien Grall

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

Re: [PATCH v2] Livepatch for ARM 64 and 32.

[Julien Grall]

On 31/08/16 16:06, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 31, 2016 at 03:49:55PM +0100, Julien Grall wrote:
>> On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
>>> Hey!
>>
>> Hi Konrad,
>>
>>> Since v1 (and RFC): [https://lists.xen.org/archives/html/xen-devel/2016-08/msg01835.html]
>>>  - Acted on most all comments.
>>>  - Added ARM32 support.
>>>
>>> The patches are based on: [PATCH v4] Livepatch fixes and features for v4.8.
>>> (https://lists.xen.org/archives/html/xen-devel/2016-08/msg02705.html)
>>>
>>> And the git tree is:
>>>  git://xenbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v4
>>>
>>> There are two outstanding questions that should be addressed at some point:
>>>  -  #16 "livepatch: tests: Make them compile under ARM64"
>>>     We chatted about adding a specific CPU bit (LIVEPATCH) so that alternative
>>>     code always gets exercised. This being for both x86 and ARM. But this has
>>>     a side effect that it gets exposed to the toolstack and can also be
>>>     exposed to the guests. I think it is better if we do not do that
>>>     so left it to be enabled based on the most common errata (or feature
>>>     on x86).
>>
>> There is no common errata on ARM. I spoke with Andrew which mentioned that
>> on x86 they have architectural xen feature bits which are not exposed to the
>> toolstack/guest.
>>
>> Furthermore, I think it would be unwise to let the toolstack aware of some
>> errata and possibly setting/clearing them. So adding a specific feature bit
>> is fine by me.
>
> So you are thinking of exposing this 'xen feature bits' that Andrew mention
> to be on ARM as well.

I only chat with Andrew about it and I have not looked at the code, so I 
can't tell how it is on x86. I was just thinking to add LIVEPATCH bit in 
cpufeature.h for now and be worry later when cpufeature will be exposed 
to the toolstack for ARM.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2] Livepatch for ARM 64 and 32.

[Andrew Cooper]

On 31/08/16 16:09, Julien Grall wrote:
>
>
> On 31/08/16 16:06, Konrad Rzeszutek Wilk wrote:
>> On Wed, Aug 31, 2016 at 03:49:55PM +0100, Julien Grall wrote:
>>> On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
>>>> Hey!
>>>
>>> Hi Konrad,
>>>
>>>> Since v1 (and RFC):
>>>> [https://lists.xen.org/archives/html/xen-devel/2016-08/msg01835.html]
>>>>  - Acted on most all comments.
>>>>  - Added ARM32 support.
>>>>
>>>> The patches are based on: [PATCH v4] Livepatch fixes and features
>>>> for v4.8.
>>>> (https://lists.xen.org/archives/html/xen-devel/2016-08/msg02705.html)
>>>>
>>>> And the git tree is:
>>>>  git://xenbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v4
>>>>
>>>> There are two outstanding questions that should be addressed at
>>>> some point:
>>>>  -  #16 "livepatch: tests: Make them compile under ARM64"
>>>>     We chatted about adding a specific CPU bit (LIVEPATCH) so that
>>>> alternative
>>>>     code always gets exercised. This being for both x86 and ARM.
>>>> But this has
>>>>     a side effect that it gets exposed to the toolstack and can
>>>> also be
>>>>     exposed to the guests. I think it is better if we do not do that
>>>>     so left it to be enabled based on the most common errata (or
>>>> feature
>>>>     on x86).
>>>
>>> There is no common errata on ARM. I spoke with Andrew which
>>> mentioned that
>>> on x86 they have architectural xen feature bits which are not
>>> exposed to the
>>> toolstack/guest.
>>>
>>> Furthermore, I think it would be unwise to let the toolstack aware
>>> of some
>>> errata and possibly setting/clearing them. So adding a specific
>>> feature bit
>>> is fine by me.
>>
>> So you are thinking of exposing this 'xen feature bits' that Andrew
>> mention
>> to be on ARM as well.
>
> I only chat with Andrew about it and I have not looked at the code, so
> I can't tell how it is on x86. I was just thinking to add LIVEPATCH
> bit in cpufeature.h for now and be worry later when cpufeature will be
> exposed to the toolstack for ARM.

Sorry - I have forgotten what exactly the purpose of this bit needs to
be, and therefore which bit of x86 I point you at.

~Andrew

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

Re: [PATCH v2] Livepatch for ARM 64 and 32.

[Julien Grall]

On 31/08/16 16:24, Andrew Cooper wrote:
> On 31/08/16 16:09, Julien Grall wrote:
>>
>>
>> On 31/08/16 16:06, Konrad Rzeszutek Wilk wrote:
>>> On Wed, Aug 31, 2016 at 03:49:55PM +0100, Julien Grall wrote:
>>>> On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
>>>>> Hey!
>>>>
>>>> Hi Konrad,
>>>>
>>>>> Since v1 (and RFC):
>>>>> [https://lists.xen.org/archives/html/xen-devel/2016-08/msg01835.html]
>>>>>  - Acted on most all comments.
>>>>>  - Added ARM32 support.
>>>>>
>>>>> The patches are based on: [PATCH v4] Livepatch fixes and features
>>>>> for v4.8.
>>>>> (https://lists.xen.org/archives/html/xen-devel/2016-08/msg02705.html)
>>>>>
>>>>> And the git tree is:
>>>>>  git://xenbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v4
>>>>>
>>>>> There are two outstanding questions that should be addressed at
>>>>> some point:
>>>>>  -  #16 "livepatch: tests: Make them compile under ARM64"
>>>>>     We chatted about adding a specific CPU bit (LIVEPATCH) so that
>>>>> alternative
>>>>>     code always gets exercised. This being for both x86 and ARM.
>>>>> But this has
>>>>>     a side effect that it gets exposed to the toolstack and can
>>>>> also be
>>>>>     exposed to the guests. I think it is better if we do not do that
>>>>>     so left it to be enabled based on the most common errata (or
>>>>> feature
>>>>>     on x86).
>>>>
>>>> There is no common errata on ARM. I spoke with Andrew which
>>>> mentioned that
>>>> on x86 they have architectural xen feature bits which are not
>>>> exposed to the
>>>> toolstack/guest.
>>>>
>>>> Furthermore, I think it would be unwise to let the toolstack aware
>>>> of some
>>>> errata and possibly setting/clearing them. So adding a specific
>>>> feature bit
>>>> is fine by me.
>>>
>>> So you are thinking of exposing this 'xen feature bits' that Andrew
>>> mention
>>> to be on ARM as well.
>>
>> I only chat with Andrew about it and I have not looked at the code, so
>> I can't tell how it is on x86. I was just thinking to add LIVEPATCH
>> bit in cpufeature.h for now and be worry later when cpufeature will be
>> exposed to the toolstack for ARM.
>
> Sorry - I have forgotten what exactly the purpose of this bit needs to
> be, and therefore which bit of x86 I point you at.

The purpose of this bit is to be able to test the alternative with 
livepatching on any platform.

All the current feature bits are for errata and none of them are common 
enough to ensure a proper testing.

So I suggested to Konrad to introduce a feature bit LIVEPATCH that would 
be enabled unconditionally or when debug is enabled. So if you load the 
hello world payload, the alternative code will effectively be exercised.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 03/20] x86/arm64: Expose the ALT_[ORIG|REPL]_PTR macros to header files.

[Julien Grall]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> That way common code can use the same macro to access
> the most common attributes without much #ifdef.
>
> Take advantage of it right away in the livepatch code.
>
> Note: on ARM we use tabs to conform to the style of the file.
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 04/20] alternatives: x86 rename and change parameters on ARM

[Julien Grall]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> On x86 we squash 'apply_alternatives' in to
> 'alternative_instructions' (who was its sole user)
> and 'apply_alternatives_nocheck' to 'apply_alternatives'.
>
> On ARM we change the parameters for 'apply_alternatives'
> to be of 'const struct alt_instr *' instead of void pointer and
> size length.
>
> We also add 'const' and make the arguments be on the
> proper offset.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

For the ARM bits:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2] Livepatch for ARM 64 and 32.

[Jan Beulich]

>>> On 31.08.16 at 17:24, <andrew.cooper3@citrix.com> wrote:
> On 31/08/16 16:09, Julien Grall wrote:
>> On 31/08/16 16:06, Konrad Rzeszutek Wilk wrote:
>>> So you are thinking of exposing this 'xen feature bits' that Andrew
>>> mention
>>> to be on ARM as well.
>>
>> I only chat with Andrew about it and I have not looked at the code, so
>> I can't tell how it is on x86. I was just thinking to add LIVEPATCH
>> bit in cpufeature.h for now and be worry later when cpufeature will be
>> exposed to the toolstack for ARM.
> 
> Sorry - I have forgotten what exactly the purpose of this bit needs to
> be, and therefore which bit of x86 I point you at.

I'm pretty sure talk here is about X86_FEATURE_ALWAYS.

Jan


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

Re: [PATCH v2 05/20] arm64/alternatives: Make it possible to patch outside of hypervisor.

[Julien Grall]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> With livepatching the alternatives that should be patched are
> outside the Xen hypervisor _start -> _end. As such having
> to use an alternative VA is not neccessary. In fact we

s/neccessary/necessary/

> can use the ones that the caller provided us with.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
>
> v2: First version
> ---
>  xen/arch/arm/alternative.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index aba06db..90a857a 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -94,24 +94,30 @@ static int __apply_alternatives(const struct alt_region *region)
>  {
>      const struct alt_instr *alt;
>      const u32 *origptr, *replptr;
> -    u32 *writeptr, *writemap;
> +    u32 *writeptr, *writemap = NULL;
>      mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
>      unsigned int text_order = get_order_from_bytes(_end - _start);
>
> -    printk(XENLOG_INFO "alternatives: Patching kernel code\n");
> -
> -    /*
> -     * The text section is read-only. So re-map Xen to be able to patch
> -     * the code.
> -     */
> -    writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
> -                      VMAP_DEFAULT);
> -    if ( !writemap )
> +    if ( region->begin >= __alt_instructions &&
> +         region->end <= __alt_instructions_end )

I think this is quite fragile because __alt_instructions is part of the 
init section. So after runtime, those regions could be reallocated for 
other purposes.

I thought a bit more about the issue, each alt_instr contains a relative 
offset. So it would be possible to re-map the region in 
__apply_alternatives_multi_stop and passed the re-map the region to
__apply_alternatives.

Furthermore, I think it is safe to mandate apply_alternatives to work 
only read-write region.

So we could simplify the hacky code I wrote to workaround the Write 
permissions implies execute-never.

Let me try to write a patch to see if my solution works.

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 09/20] arm/mm: Introduce modify_xen_mappings

[Julien Grall]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> Which is only used by Livepatch code. The purpose behind
> this call is to modify the page table entries flags.
>
> Specifically the .ro and .nx flags. The current mechanism
> puts cache attributes in the flags and the .ro and .nx are
> locked down and assumed to be .ro=0, nx=1.
>
> Livepatch needs .nx=0 and also .ro to be set to 1.
>
> We introduce a new 'flags' where various bits determine
> whether .ro and .nx bits are set or cleared. We can't use
> an enum as the function prototype would diverge from x86.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

with one minor request (see below).

> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 05d9f82..2f66740 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -66,6 +66,17 @@
>  #define PAGE_HYPERVISOR_WC      (DEV_WC)
>
>  /*
> + * Defines for changing the PTE .ro and .nx bits. This is only to be

I would say "hypervisor PTE" because the stage-2 page tables have 
different permission (read and write have separate bits).

> + * used with modify_xen_mappings.
> + */
> +#define _PTE_NX_BIT     0U
> +#define _PTE_RO_BIT     1U
> +#define PTE_NX          (1U << _PTE_NX_BIT)
> +#define PTE_RO          (1U << _PTE_RO_BIT)
> +#define PTE_NX_MASK(x)  (((x) >> _PTE_NX_BIT) & 0x1U)
> +#define PTE_RO_MASK(x)  (((x) >> _PTE_RO_BIT) & 0x1U)
> +
> +/*
>   * Stage 2 Memory Type.
>   *
>   * These are valid in the MemAttr[3:0] field of an LPAE stage 2 page
>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 10/20] arm64/insn: introduce aarch64_insn_gen_{nop|branch_imm}() helper functions

[Julien Grall]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> This is copied from Linux 4.7, and the initial commit
> that put this in is 5c5bf25d4f7a950382f94fc120a5818197b48fe9
> "arm64: introduce aarch64_insn_gen_{nop|branch_imm}() helper functions"
>
> This lays the groundwork for Livepatch to generate the
> trampoline to jump to the new replacement function.
> Also allows us to NOP the callsites.
>
> This lays the groundwork for Livepatch to generate the
> trampoline to jump to the new replacement function.
> And also to NOP insns.

NIT: Both paragraphs seem to stay the same things. Did you mean to keep 
only one?

>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

The rest of the patch looks good:

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 11/20] arm/arm64: Update comment about VA layout.

[Julien Grall]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> It was missing 2MB.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Regards,

> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
>
> v2: First submission. Spun of from 'livepatch: Initial ARM64 support."
> ---
>  xen/include/asm-arm/config.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index a96f845..6772555 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -82,7 +82,7 @@
>   *   8M -  10M   Early relocation address (used when relocating Xen)
>   *
>   * ARM32 layout:
> - *   0  -   8M   <COMMON>
> + *   0  -  10M   <COMMON>
>   *
>   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
> @@ -93,7 +93,7 @@
>   *
>   * ARM64 layout:
>   * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
> - *   0  -   8M   <COMMON>
> + *   0  -  10M   <COMMON>
>   *
>   *   1G -   2G   VMAP: ioremap and early_ioremap
>   *
>

-- 
Julien Grall

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

Re: [PATCH v2 12/20] x86, arm: Change arch_livepatch_quiesce() decleration.

[Julien Grall]

Hi Konrad,

NIT: title: s/decleration/declaration/

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> On ARM we need an alternative VA region to poke in the
> hypervisor .text data. And since this is setup during runtime
> we may fail (it uses vmap so most likely error is ENOMEM).
>
> As such this error needs to be bubbled up and also abort
> the livepatching if it occurs.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 13/20] livepatch: Initial ARM64 support.

[Julien Grall]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> As compared to x86 the va of the hypervisor .text
> is locked down - we cannot modify the running pagetables
> to have the .ro flag unset. We borrow the same idea that
> alternative patching has - which is to vmap the entire
> .text region and use the alternative virtual address
> for patching.
>
> Since we are doing vmap we may fail, hence the
> arch_livepatch_quiesce was changed (see "x86,arm:
> Change arch_livepatch_quiesce() decleration") to return

s/decleration/declaration/

> an error value which will be bubbled in payload->rc and
> provided to the user (along with messages in the ring buffer).
>
> The livepatch virtual address space (where the new functions
> are) needs to be close to the hypervisor virtual address
> so that the trampoline can reach it. As such we re-use
> the BOOT_RELOC_VIRT_START which is not used after bootup
> (alternatively we can also use the space after the _end to
> FIXMAP_ADDR(0), but that may be too small).
>
> The ELF relocation engine at the start was coded from
> the "ELF for the ARM 64-bit Architecture (AArch64)"
> (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0056b/IHI0056B_aaelf64.pdf)
> but after a while of trying to write the correct bit shifting
> and masking from scratch I ended up borrowing from Linux, the
> 'reloc_insn_imm' (Linux v4.7 arch/arm64/kernel/module.c function.
> See 257cb251925f854da435cbf79b140984413871ac "arm64: Loadable modules")
>
> And while at it - we also utilize code from Linux to construct
> the right branch instruction (see "arm64/insn: introduce
> aarch64_insn_gen_{nop|branch_imm}() helper functions").
>
> In the livepatch payload loading code we tweak the #ifdef to
> only exclude ARM_32. The exceptions are not part of ARM 32/64 hence
> they are still behind the #ifdef.
>
> We also expand the MAINTAINERS file to include the arm64 and arm32
> platform specific livepatch file.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

[...]

> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> new file mode 100644
> index 0000000..0809a42
> --- /dev/null
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -0,0 +1,323 @@
> +/*
> + *  Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + */
> +
> +#include <xen/bitops.h>
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/livepatch_elf.h>
> +#include <xen/livepatch.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/bitops.h>
> +#include <asm/byteorder.h>
> +#include <asm/insn.h>
> +#include <asm/livepatch.h>
> +
> +void arch_livepatch_apply_jmp(struct livepatch_func *func)
> +{
> +    uint32_t insn;
> +    uint32_t *old_ptr;
> +    uint32_t *new_ptr;
> +
> +    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> +    BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
> +
> +    ASSERT(vmap_of_xen_text);
> +
> +    /* Save old one. */
> +    old_ptr = func->old_addr;
> +    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);

I don't see any value to use a temporary variable (old_ptr) to hold 
func->old_addr here.

> +
> +    if ( func->new_addr )
> +        insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
> +                                           (unsigned long)func->new_addr,
> +                                           AARCH64_INSN_BRANCH_NOLINK);
> +    else
> +        insn = aarch64_insn_gen_nop();
> +
> +    ASSERT(insn != AARCH64_BREAK_FAULT);

Could you document in the code what prevents aarch64_insn_gen_branch_imm 
to not generate a break fault instruction?

> +    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> +
> +    /* PATCH! */
> +    *(new_ptr) = cpu_to_le32(insn);
> +
> +    clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr));
> +}
> +
> +void arch_livepatch_revert_jmp(const struct livepatch_func *func)
> +{
> +    uint32_t *new_ptr;
> +    uint32_t insn;
> +
> +    memcpy(&insn, func->opaque, PATCH_INSN_SIZE);
> +
> +    new_ptr = (uint32_t *)func->old_addr - (u32 *)_start + vmap_of_xen_text;

The computation here looks wrong. You are mixing (u32 *) and (void *).

> +
> +    /* PATCH! */
> +    *(new_ptr) = cpu_to_le32(insn);
> +
> +    clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr));
> +}
> +
> +int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
> +{
> +    const Elf_Ehdr *hdr = elf->hdr;
> +
> +    if ( hdr->e_machine != EM_AARCH64 ||
> +         hdr->e_ident[EI_CLASS] != ELFCLASS64 )
> +    {
> +        dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF Machine type!\n",
> +                elf->name);
> +        return -EOPNOTSUPP;
> +    }
> +
> +    return 0;
> +}
> +
> +enum aarch64_reloc_op {
> +    RELOC_OP_NONE,
> +    RELOC_OP_ABS,
> +    RELOC_OP_PREL,
> +    RELOC_OP_PAGE,
> +};
> +
> +static u64 do_reloc(enum aarch64_reloc_op reloc_op, void *place, u64 val)
> +{
> +    switch (reloc_op) {

This seems to be a left-over of the Linux coding style.

> +    case RELOC_OP_ABS:
> +        return val;
> +
> +    case RELOC_OP_PREL:
> +        return val - (u64)place;
> +
> +    case RELOC_OP_PAGE:
> +        return (val & ~0xfff) - ((u64)place & ~0xfff);
> +
> +    case RELOC_OP_NONE:
> +        return 0;
> +
> +    }
> +
> +    dprintk(XENLOG_DEBUG, LIVEPATCH "do_reloc: unknown relocation operation %d\n", reloc_op);

NIT: Please add a blank line here.

> +    return 0;
> +}
> +
> +static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
> +{
> +    s64 sval = do_reloc(op, place, val);
> +
> +    switch (len) {

Same question about coding style here.

> +    case 16:
> +        *(s16 *)place = sval;
> +        if (sval < S16_MIN || sval > U16_MAX)

Ditto

> +	        return -EOVERFLOW;
> +        break;
> +
> +    case 32:
> +        *(s32 *)place = sval;
> +        if (sval < S32_MIN || sval > U32_MAX)

Ditto

> +	        return -EOVERFLOW;
> +        break;
> +
> +    case 64:
> +        *(s64 *)place = sval;
> +        break;
> +
> +    default:
> +        dprintk(XENLOG_DEBUG, LIVEPATCH "Invalid length (%d) for data relocation\n", len);
> +        return 0;
> +    }
> +
> +    return 0;
> +}
> +
> +enum aarch64_insn_movw_imm_type {
> +    AARCH64_INSN_IMM_MOVNZ,
> +    AARCH64_INSN_IMM_MOVKZ,
> +};
> +
> +static int reloc_insn_imm(enum aarch64_reloc_op op, void *dest, u64 val,
> +                          int lsb, int len, enum aarch64_insn_imm_type imm_type)
> +{
> +    u64 imm, imm_mask;
> +    s64 sval;
> +    u32 insn = *(u32 *)dest;
> +
> +    /* Calculate the relocation value. */
> +    sval = do_reloc(op, dest, val);
> +    sval >>= lsb;
> +
> +    /* Extract the value bits and shift them to bit 0. */
> +    imm_mask = (BIT(lsb + len) - 1) >> lsb;
> +    imm = sval & imm_mask;
> +
> +    /* Update the instruction's immediate field. */
> +    insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
> +    *(u32 *)dest = insn;
> +
> +    /*
> +     * Extract the upper value bits (including the sign bit) and
> +     * shift them to bit 0.
> +     */
> +    sval = (s64)(sval & ~(imm_mask >> 1)) >> (len - 1);
> +
> +    /*
> +     * Overflow has occurred if the upper bits are not all equal to
> +     * the sign bit of the value.
> +     */
> +    if ((u64)(sval + 1) >= 2)

Ditto

> +        return -EOVERFLOW;
> +    return 0;
> +}
> +
> +int arch_livepatch_perform_rela(struct livepatch_elf *elf,
> +                                const struct livepatch_elf_sec *base,
> +                                const struct livepatch_elf_sec *rela)
> +{
> +    const Elf_RelA *r;
> +    unsigned int symndx, i;
> +    uint64_t val;
> +    void *dest;
> +    bool_t overflow_check;
> +
> +    for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
> +    {
> +        int ovf = 0;
> +
> +        r = rela->data + i * rela->sec->sh_entsize;
> +
> +        symndx = ELF64_R_SYM(r->r_info);
> +
> +        if ( symndx > elf->nsym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
> +
> +        dest = base->load_addr + r->r_offset; /* P */
> +        val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
> +
> +        overflow_check = 1;

Please use true here.

> +
> +        /* ARM64 operations at minimum are always 32-bit. */
> +        if ( r->r_offset >= base->sec->sh_size ||
> +            (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
> +            goto bad_offset;
> +
> +        switch ( ELF64_R_TYPE(r->r_info) ) {

This seems to be a left-over of the Linux coding style.

> +        /* Data */
> +        case R_AARCH64_ABS64:
> +            if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
> +                goto bad_offset;
> +            overflow_check = false;
> +            ovf = reloc_data(RELOC_OP_ABS, dest, val, 64);
> +            break;
> +
> +        case R_AARCH64_ABS32:
> +            ovf = reloc_data(RELOC_OP_ABS, dest, val, 32);
> +            break;

I have noticed that not all the relocations are implemented (e.g 
R_AARCH64_ABS16, R_AARCH64_MOVW_*...). I don't think there is anything 
preventing the compiler to use them. So is there any particular reasons 
to not include them?

[...]

> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index 755f596..f49e347 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -6,44 +6,80 @@
>  #include <xen/lib.h>
>  #include <xen/livepatch_elf.h>
>  #include <xen/livepatch.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/livepatch.h>
> +#include <asm/mm.h>
> +
> +void *vmap_of_xen_text;
>
>  int arch_livepatch_quiesce(void)
>  {
> -    return -ENOSYS;
> +    mfn_t text_mfn;
> +    unsigned int text_order;
> +
> +    if ( vmap_of_xen_text )
> +        return -EINVAL;
> +
> +    text_mfn = _mfn(virt_to_mfn(_start));
> +    text_order = get_order_from_bytes(_end - _start);
> +
> +    /*
> +     * The text section is read-only. So re-map Xen to be able to patch
> +     * the code.
> +     */
> +    vmap_of_xen_text = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,

NIT: Could you use 1U here?

> +                              VMAP_DEFAULT);
> +
> +    if ( !vmap_of_xen_text )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! (order=%u)\n",
> +               text_order);
> +        return -ENOMEM;
> +    }

NIT: Missing blank line here.

> +    return 0;
>  }

[...]

>  int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
>  {
> -    return -ENOSYS;
> +    unsigned long start = (unsigned long)va;
> +    unsigned int flags = 0;
> +
> +    ASSERT(va);
> +    ASSERT(pages);
> +
> +    switch ( type ) {

NIT: The brace should be on a newline.

> +    case LIVEPATCH_VA_RX:
> +        flags = PTE_RO; /* R set, NX clear */
> +        break;
> +
> +    case LIVEPATCH_VA_RW:
> +        flags = PTE_NX; /* R clear, NX set */
> +        break;
> +
> +    case LIVEPATCH_VA_RO:
> +        flags = PTE_NX | PTE_RO; /* R set, NX set */
> +        break;
> +
> +    default:
> +        return -EINVAL;
> +    }
> +
> +    return modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
>  }

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 14/20] livepatch: ARM 32|64: Ignore mapping symbols: $[d, a, x, t]

[Julien Grall]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> Those symbols are used to help final linkers to replace insn.
> The ARM ELF specification mandates that they are present
> to denote the start of certain CPU features. There are two
> variants of it - short and long format.
>
> Either way - we can ignore these symbols.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>
> v1: First submission
> v2: Update the order of symbols, fix title
>     Add {} in after the first if - per Jan's recommendation.
> ---
>  xen/arch/arm/livepatch.c    | 32 ++++++++++++++++++++++++++++++++
>  xen/arch/x86/livepatch.c    |  7 +++++++
>  xen/common/livepatch.c      |  2 +-
>  xen/include/xen/livepatch.h |  2 ++
>  4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index f49e347..c290602 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -82,6 +82,38 @@ void arch_livepatch_unmask(void)
>      local_abort_enable();
>  }
>
> +int arch_is_payload_symbol(const struct livepatch_elf *elf,
> +                           const struct livepatch_elf_sym *sym)

I think this function should return bool (or bool_t) as the return will 
be used by is_payload_symbol as bool.

> +{
> +    /*
> +     * - Mapping symbols - denote the "start of a sequence of bytes of the
> +     *   appropiate type" to mark certain features - such as start of region

s/appropiate/appropriate/

> +     *   containing data ($d); ARM ($a), A64 ($x), or Thumb instructions ($t).
> +     *
> +     * The format is either short: '$x' or long: '$x.<any>'. We do not
> +     * need this and more importantly - each payload will contain this
> +     * resulting in symbol collisions.
> +     */
> +    if ( *sym->name == '$' && sym->name[1] != '\0' )
> +    {
> +        char p = sym->name[1];
> +        size_t len = strlen(sym->name);
> +
> +        if ( (len >= 3 && ( sym->name[2] == '.' )) || (len == 2) )
> +        {
> +            if ( p == 'd' ||
> +#ifdef CONFIG_ARM_32
> +                 p == 'a' || p == 't'

Note that Xen is not using Thumb instructions which have variable 
length, so we shouldn't expect to see $t. symbols.

> +#else
> +                 p == 'x'
> +#endif
> +               )
> +                return 0;

Please use false here.

> +        }
> +    }
> +    return 1;

Please use true here.

> +}
> +

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 18/20] xen/arm32/livepatch: Add BPICALLIS to helper to invalidate all instruction caches

[Julien Grall]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> When we are flushing the cache we are most likley also want
> to flush the branch predictor too. Hence add this.

I think it makes more sense to fold this patch into #18 because 
ICIALLUIS may flush the branch predictor if the instruction cache is 
separate (see Table B4-35 in ARM DDI 0406C.c).

Therefore we need to be consistent and also flush the branch predictor 
for unified cache.

>
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
>
> v2: First submission
> ---
>  xen/arch/arm/livepatch.c         | 3 +++
>  xen/include/asm-arm/arm32/page.h | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index c290602..cdb8d65 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -45,6 +45,9 @@ void arch_livepatch_revive(void)
>      /*
>       * Nuke the instruction cache. Data cache has been cleaned before in
>       * arch_livepatch_apply_jmp.
> +     *
> +     * Need to flush the branch predictor for ARMv7 as it may be
> +     * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b).

I don't think this comment belongs to livepatch.c. It describes the 
behavior internal behavior of invalidate_icache.

>       */
>      invalidate_icache();
>
> diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
> index 26184ec..6caf596 100644
> --- a/xen/include/asm-arm/arm32/page.h
> +++ b/xen/include/asm-arm/arm32/page.h
> @@ -32,7 +32,10 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
>  /* Invalidate all instruction caches in Inner Shareable domain to PoU */
>  static inline void invalidate_icache(void)
>  {
> -    asm volatile (CMD_CP32(ICIALLUIS));
> +    asm volatile (
> +        CMD_CP32(ICIALLUIS)     /* Flush I-cache. */
> +        CMD_CP32(BPIALLIS)      /* Flush branch predictor. */
> +        : : : "memory");
>  }
>
>  /*
>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 19/20] livepatch/elf: Adjust section aligment to word

[Julien Grall]

Hi,

On 25/08/16 16:11, Jan Beulich wrote:
>>>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
>> On most architectures it does not matter what the aligment is.
>>
>> On ARM32 it is paramount that the aligment is word-size (4)
>> otherwise we get a Data Abort when trying to perform ELF
>> relocations. That is due to ARM 32 only being able to write to
>> word-aligned addresses.

Well, it is the same on ARM64. However we decided to disable the 
alignment check (SCTLR.A not set) because some of the primitives 
imported from Linux (e.g memcpy) rely on the hardware handling misalignment.

In any case, I would not recommend to use unaligned access on ARM as 
they may have a big performance impact.

>
> That's not exactly true, afaik: ARM can write to byte- and
> half-word-aligned addresses, but only bytes/half-words.

That's correct.

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 18/20] xen/arm32/livepatch: Add BPICALLIS to helper to invalidate all instruction caches

[Konrad Rzeszutek Wilk]

On Thu, Sep 01, 2016 at 04:13:30PM +0100, Julien Grall wrote:
> Hi Konrad,
> 
> On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> > When we are flushing the cache we are most likley also want
> > to flush the branch predictor too. Hence add this.
> 
> I think it makes more sense to fold this patch into #18 because ICIALLUIS

This is #18 :-)

> may flush the branch predictor if the instruction cache is separate (see
> Table B4-35 in ARM DDI 0406C.c).
> 
> Therefore we need to be consistent and also flush the branch predictor for
> unified cache.
> 
> > 
> > Suggested-by: Julien Grall <julien.grall@arm.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > ---
> > Cc: Julien Grall <julien.grall@arm.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > v2: First submission
> > ---
> >  xen/arch/arm/livepatch.c         | 3 +++
> >  xen/include/asm-arm/arm32/page.h | 5 ++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> > index c290602..cdb8d65 100644
> > --- a/xen/arch/arm/livepatch.c
> > +++ b/xen/arch/arm/livepatch.c
> > @@ -45,6 +45,9 @@ void arch_livepatch_revive(void)
> >      /*
> >       * Nuke the instruction cache. Data cache has been cleaned before in
> >       * arch_livepatch_apply_jmp.
> > +     *
> > +     * Need to flush the branch predictor for ARMv7 as it may be
> > +     * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b).
> 
> I don't think this comment belongs to livepatch.c. It describes the behavior
> internal behavior of invalidate_icache.

<nods>
> 
> >       */
> >      invalidate_icache();
> > 
> > diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
> > index 26184ec..6caf596 100644
> > --- a/xen/include/asm-arm/arm32/page.h
> > +++ b/xen/include/asm-arm/arm32/page.h
> > @@ -32,7 +32,10 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
> >  /* Invalidate all instruction caches in Inner Shareable domain to PoU */
> >  static inline void invalidate_icache(void)
> >  {
> > -    asm volatile (CMD_CP32(ICIALLUIS));
> > +    asm volatile (
> > +        CMD_CP32(ICIALLUIS)     /* Flush I-cache. */
> > +        CMD_CP32(BPIALLIS)      /* Flush branch predictor. */
> > +        : : : "memory");
> >  }
> > 
> >  /*
> > 
> 
> Regards,
> 
> -- 
> Julien Grall

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

Re: [PATCH v2 01/20] livepatch: Bubble up sanity checks on Elf relocs

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2016 at 09:37:16AM -0400, Konrad Rzeszutek Wilk wrote:
> The checks for SHT_REL[,A] ELF sanity checks does not need to
> be in the platform specific file and can be bubbled up
> in the platform agnostic file.
> 
> This makes the ARM 32/64 implementation easier as the
> duplicate checks don't have to be in the platform specific files.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

Ross, could you review the patch please?


> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> v1: First submission
> v2: Mirror checks for SHT_REL case.
> ---
>  xen/arch/x86/livepatch.c   | 12 ------------
>  xen/common/livepatch_elf.c | 17 +++++++++++++++++
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 952e897..5b0c863 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -146,18 +146,6 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>      uint64_t val;
>      uint8_t *dest;
>  
> -    /* Nothing to do. */
> -    if ( !rela->sec->sh_size )
> -        return 0;
> -
> -    if ( rela->sec->sh_entsize < sizeof(Elf_RelA) ||
> -         rela->sec->sh_size % rela->sec->sh_entsize )
> -    {
> -        dprintk(XENLOG_ERR, LIVEPATCH "%s: Section relative header is corrupted!\n",
> -                elf->name);
> -        return -EINVAL;
> -    }
> -
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
>          r = rela->data + i * rela->sec->sh_entsize;
> diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
> index 789e8fc..cda9b27 100644
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -335,6 +335,7 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf)
>      struct livepatch_elf_sec *r, *base;
>      unsigned int i;
>      int rc = 0;
> +    size_t sz;
>  
>      ASSERT(elf->sym);
>  
> @@ -365,6 +366,22 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf)
>          }
>  
>          if ( r->sec->sh_type == SHT_RELA )
> +            sz = sizeof(Elf_RelA);
> +        else
> +            sz = sizeof(Elf_Rel);
> +
> +        if ( !r->sec->sh_size )
> +            continue;
> +
> +        if ( r->sec->sh_entsize < sz || r->sec->sh_size % r->sec->sh_entsize )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section relative header is corrupted!\n",
> +                    elf->name);
> +            rc = -EINVAL;
> +            break;
> +        }
> +
> +        if ( r->sec->sh_type == SHT_RELA )
>              rc = arch_livepatch_perform_rela(elf, base, r);
>          else /* SHT_REL */
>              rc = arch_livepatch_perform_rel(elf, base, r);
> -- 
> 2.4.11
> 

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

Re: [PATCH v2 15/20] livepatch: Move test-cases to common

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2016 at 09:05:27AM -0600, Jan Beulich wrote:
> >>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
> > So they can be shared with ARM64 (but not yet, so they
> > are only built on x86).
> > 
> > No functional change.
> > 
> > We also need to tweak the MAINTAINERS and .gitignore file
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> for whichever parts it's relevant
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thank you!
> with one adjustment request:
> 
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -271,6 +271,7 @@ F:  tools/misc/xen-livepatch.c
> >  F:  xen/arch/*/livepatch*
> >  F:  xen/arch/*/*/livepatch*
> >  F:  xen/common/livepatch*
> > +F:  xen/test/livepatch/*
> >  F:  xen/include/xen/livepatch*
> 
> Please keep this sorted.

A side effect of that is that * vs / gets moved too.

diff --git a/MAINTAINERS b/MAINTAINERS
index 7d42883..507bb40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -268,10 +268,11 @@ M:  Ross Lagerwall <ross.lagerwall@citrix.com>
 S:  Supported
 F:  docs/misc/livepatch.markdown
 F:  tools/misc/xen-livepatch.c
-F:  xen/arch/*/livepatch*
 F:  xen/arch/*/*/livepatch*
+F:  xen/arch/*/livepatch*
 F:  xen/common/livepatch*
 F:  xen/include/xen/livepatch*
+F:  xen/test/livepatch/*

You OK with that or would you prefer to leave it as is?

> 
> Jan
> 

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

Re: [PATCH v2 15/20] livepatch: Move test-cases to common

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2016 at 09:37:30AM -0400, Konrad Rzeszutek Wilk wrote:
> So they can be shared with ARM64 (but not yet, so they
> are only built on x86).
> 
> No functional change.
> 
> We also need to tweak the MAINTAINERS and .gitignore file
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Ross, could you review the patch please?

Thanks!
> 
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> v1: First submission
> v2: Move to test/livepatch per Jan's recommendation
> ---
>  .gitignore                                                     | 8 ++++----
>  MAINTAINERS                                                    | 1 +
>  xen/Makefile                                                   | 3 ++-
>  xen/arch/arm/Makefile                                          | 3 ---
>  xen/arch/x86/Makefile                                          | 5 -----
>  xen/test/Makefile                                              | 9 +++++++++
>  xen/{arch/x86/test => test/livepatch}/Makefile                 | 0
>  xen/{arch/x86/test => test/livepatch}/xen_bye_world.c          | 0
>  xen/{arch/x86/test => test/livepatch}/xen_bye_world_func.c     | 0
>  xen/{arch/x86/test => test/livepatch}/xen_hello_world.c        | 0
>  xen/{arch/x86/test => test/livepatch}/xen_hello_world_func.c   | 0
>  xen/{arch/x86/test => test/livepatch}/xen_replace_world.c      | 0
>  xen/{arch/x86/test => test/livepatch}/xen_replace_world_func.c | 0
>  13 files changed, 16 insertions(+), 13 deletions(-)
>  create mode 100644 xen/test/Makefile
>  rename xen/{arch/x86/test => test/livepatch}/Makefile (100%)
>  rename xen/{arch/x86/test => test/livepatch}/xen_bye_world.c (100%)
>  rename xen/{arch/x86/test => test/livepatch}/xen_bye_world_func.c (100%)
>  rename xen/{arch/x86/test => test/livepatch}/xen_hello_world.c (100%)
>  rename xen/{arch/x86/test => test/livepatch}/xen_hello_world_func.c (100%)
>  rename xen/{arch/x86/test => test/livepatch}/xen_replace_world.c (100%)
>  rename xen/{arch/x86/test => test/livepatch}/xen_replace_world_func.c (100%)
> 
> diff --git a/.gitignore b/.gitignore
> index 44cc7bf..4fded28 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -255,10 +255,6 @@ xen/arch/x86/efi.lds
>  xen/arch/x86/efi/check.efi
>  xen/arch/x86/efi/disabled
>  xen/arch/x86/efi/mkreloc
> -xen/arch/x86/test/config.h
> -xen/arch/x86/test/xen_hello_world.livepatch
> -xen/arch/x86/test/xen_bye_world.livepatch
> -xen/arch/x86/test/xen_replace_world.livepatch
>  xen/arch/*/efi/boot.c
>  xen/arch/*/efi/compat.c
>  xen/arch/*/efi/efi.h
> @@ -275,6 +271,10 @@ xen/include/public/public
>  xen/include/xen/*.new
>  xen/include/xen/acm_policy.h
>  xen/include/xen/compile.h
> +xen/test/livepatch/config.h
> +xen/test/livepatch/xen_bye_world.livepatch
> +xen/test/livepatch/xen_hello_world.livepatch
> +xen/test/livepatch/xen_replace_world.livepatch
>  xen/tools/kconfig/.tmp_gtkcheck
>  xen/tools/kconfig/.tmp_qtcheck
>  xen/tools/symbols
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae0b6bc..160d950 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -271,6 +271,7 @@ F:  tools/misc/xen-livepatch.c
>  F:  xen/arch/*/livepatch*
>  F:  xen/arch/*/*/livepatch*
>  F:  xen/common/livepatch*
> +F:  xen/test/livepatch/*
>  F:  xen/include/xen/livepatch*
>  
>  MACHINE CHECK (MCA) & RAS
> diff --git a/xen/Makefile b/xen/Makefile
> index d68c84d..94ced98 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -80,7 +80,7 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
>  
>  .PHONY: _tests
>  _tests:
> -	$(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) tests
> +	$(MAKE) -f $(BASEDIR)/Rules.mk -C test tests
>  
>  .PHONY: _uninstall
>  _uninstall: D=$(DESTDIR)
> @@ -114,6 +114,7 @@ _clean: delete-unfresh-files
>  	$(MAKE) -f $(BASEDIR)/Rules.mk -C xsm clean
>  	$(MAKE) -f $(BASEDIR)/Rules.mk -C crypto clean
>  	$(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) clean
> +	$(MAKE) -f $(BASEDIR)/Rules.mk -C test clean
>  	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) clean
>  	find . \( -name "*.o" -o -name ".*.d" \) -exec rm -f {} \;
>  	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 9f75c5c..9dc0797 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -73,9 +73,6 @@ ifeq ($(CONFIG_ARM_64),y)
>  	ln -sf $(notdir $@)  ../../$(notdir $@).efi
>  endif
>  
> -.PHONY: tests
> -tests:
> -
>  $(TARGET).axf: $(TARGET)-syms
>  	# XXX: VE model loads by VMA so instead of
>  	# making a proper ELF we link with LMA == VMA and adjust crudely
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 7209560..b813887 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -92,10 +92,6 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
>  	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \
>  	`$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
>  
> -.PHONY: tests
> -tests:
> -	$(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
> -
>  ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
>  
>  ifeq ($(lto),y)
> @@ -219,4 +215,3 @@ clean::
>  	rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.o efi/.*.d efi/*.efi efi/disabled efi/mkreloc
>  	rm -f boot/reloc.S boot/reloc.lnk boot/reloc.bin
>  	rm -f note.o
> -	$(MAKE) -f $(BASEDIR)/Rules.mk -C test clean
> diff --git a/xen/test/Makefile b/xen/test/Makefile
> new file mode 100644
> index 0000000..8c53040
> --- /dev/null
> +++ b/xen/test/Makefile
> @@ -0,0 +1,9 @@
> +.PHONY: tests
> +tests:
> +ifeq ($(XEN_TARGET_ARCH),x86_64)
> +	$(MAKE) -f $(BASEDIR)/Rules.mk -C livepatch livepatch
> +endif
> +
> +.PHONY: clean
> +clean::
> +	$(MAKE) -f $(BASEDIR)/Rules.mk -C livepatch clean
> diff --git a/xen/arch/x86/test/Makefile b/xen/test/livepatch/Makefile
> similarity index 100%
> rename from xen/arch/x86/test/Makefile
> rename to xen/test/livepatch/Makefile
> diff --git a/xen/arch/x86/test/xen_bye_world.c b/xen/test/livepatch/xen_bye_world.c
> similarity index 100%
> rename from xen/arch/x86/test/xen_bye_world.c
> rename to xen/test/livepatch/xen_bye_world.c
> diff --git a/xen/arch/x86/test/xen_bye_world_func.c b/xen/test/livepatch/xen_bye_world_func.c
> similarity index 100%
> rename from xen/arch/x86/test/xen_bye_world_func.c
> rename to xen/test/livepatch/xen_bye_world_func.c
> diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/test/livepatch/xen_hello_world.c
> similarity index 100%
> rename from xen/arch/x86/test/xen_hello_world.c
> rename to xen/test/livepatch/xen_hello_world.c
> diff --git a/xen/arch/x86/test/xen_hello_world_func.c b/xen/test/livepatch/xen_hello_world_func.c
> similarity index 100%
> rename from xen/arch/x86/test/xen_hello_world_func.c
> rename to xen/test/livepatch/xen_hello_world_func.c
> diff --git a/xen/arch/x86/test/xen_replace_world.c b/xen/test/livepatch/xen_replace_world.c
> similarity index 100%
> rename from xen/arch/x86/test/xen_replace_world.c
> rename to xen/test/livepatch/xen_replace_world.c
> diff --git a/xen/arch/x86/test/xen_replace_world_func.c b/xen/test/livepatch/xen_replace_world_func.c
> similarity index 100%
> rename from xen/arch/x86/test/xen_replace_world_func.c
> rename to xen/test/livepatch/xen_replace_world_func.c
> -- 
> 2.4.11
> 

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

Re: [PATCH v2 14/20] livepatch: ARM 32|64: Ignore mapping symbols: $[d, a, x, t]

[Konrad Rzeszutek Wilk]

.snip..
> > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> > index f49e347..c290602 100644
> > --- a/xen/arch/arm/livepatch.c
> > +++ b/xen/arch/arm/livepatch.c
> > @@ -82,6 +82,38 @@ void arch_livepatch_unmask(void)
> >      local_abort_enable();
> >  }
> > 
> > +int arch_is_payload_symbol(const struct livepatch_elf *elf,
> > +                           const struct livepatch_elf_sym *sym)
> 
> I think this function should return bool (or bool_t) as the return will be
> used by is_payload_symbol as bool.

I also took the liberty of changing the name. It is now:

bool_t arch_livepatch_symbol_ok(const struct livepatch_elf *elf,
                                const struct livepatch_elf_sym *sym)

As that sounds much more understanding I think.

> > +#ifdef CONFIG_ARM_32
> > +                 p == 'a' || p == 't'
> 
> Note that Xen is not using Thumb instructions which have variable length, so
> we shouldn't expect to see $t. symbols.

/me nods.

Let me spin off a seperate patch that will check for arch specific symbols.
And if found within the payload will abort loading of it.

Something like this (compile tested on x86 for right now):

From e513d9d2e689870ab5b381a7a6d7d3ad24a517e5 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 6 Sep 2016 14:52:21 -0400
Subject: [PATCH] livepatch/arm/x86: Check payload for for unwelcomed symbols.

Certain platforms, such as ARM [32|64] add extra mapping symbols
such as $x (for ARM64 instructions), or more interesting to
this patch: $t (for Thumb instructions). These symbols are suppose
to help the final linker to make any adjustments (such as
add an veneer). But more importantly - we do not compile Xen
with any Thumb instructions (which are variable length) - and
if we find these mapping symbols we should disallow such payload.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3: New submission.
---
 xen/arch/arm/livepatch.c    | 15 ++++++++++++++-
 xen/arch/x86/livepatch.c    |  7 +++++++
 xen/common/livepatch_elf.c  |  9 +++++++++
 xen/include/xen/livepatch.h |  2 ++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 95a538b..6a9502b 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -114,7 +114,20 @@ bool_t arch_livepatch_symbol_ok(const struct livepatch_elf *elf,
     }
     return true;
 }
-
+int arch_livepatch_symbol_check(const struct livepatch_elf *elf,
+                                const struct livepatch_elf_sym *sym)
+{
+#ifdef CONFIG_ARM_32
+    /*
+     * Xen does not use Thumb instructions - and we should not see any of
+     * them. If we do, abort.
+     */
+    if ( *sym->name == '$' && sym->name[1] == 't' )
+        return -EINVAL;
+#else
+    return 0;
+#endif
+}
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
                                const struct livepatch_elf_sec *rela)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index b3581ff..781dab9 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -137,6 +137,13 @@ bool_t arch_livepatch_symbol_ok(const struct livepatch_elf *elf,
     return true;
 }
 
+int arch_livepatch_symbol_check(const struct livepatch_elf *elf,
+                                const struct livepatch_elf_sym *sym)
+{
+    /* No special checks on x86. */
+    return 0;
+}
+
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
                                const struct livepatch_elf_sec *rela)
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index ee6fef4..26c8df7 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -244,6 +244,7 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data)
     for ( i = 1; i < nsym; i++ )
     {
         const Elf_Sym *s = symtab_sec->data + symtab_sec->sec->sh_entsize * i;
+        int rc;
 
         delta = s->st_name;
         /* Boundary check within the .strtab. */
@@ -256,6 +257,14 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data)
 
         sym[i].sym = s;
         sym[i].name = strtab_sec->data + delta;
+        /* On ARM we should NEVER see $t* symbols. */
+        rc = arch_livepatch_symbol_check(elf, sym);
+        if ( rc )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Symbol '%s' should not be in payload!\n",
+                    elf->name, sym[i].name);
+            return rc;
+        }
     }
     elf->nsym = nsym;
 
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index c47f43d..6a0ad7f 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -48,6 +48,8 @@ int arch_verify_insn_length(unsigned long len);
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
 bool_t arch_livepatch_symbol_ok(const struct livepatch_elf *elf,
                                 const struct livepatch_elf_sym *sym);
+int arch_livepatch_symbol_check(const struct livepatch_elf *elf,
+                                const struct livepatch_elf_sym *sym);
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
                                const struct livepatch_elf_sec *rela);
-- 
2.4.11


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

Re: [PATCH v2 18/20] xen/arm32/livepatch: Add BPICALLIS to helper to invalidate all instruction caches

[Konrad Rzeszutek Wilk]

On Thu, Sep 01, 2016 at 04:13:30PM +0100, Julien Grall wrote:
> Hi Konrad,
> 
> On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> > When we are flushing the cache we are most likley also want
> > to flush the branch predictor too. Hence add this.
> 
> I think it makes more sense to fold this patch into #18 because ICIALLUIS

I squashed it in " xen/arm32: Add an helper to invalidate all instruction caches"

> may flush the branch predictor if the instruction cache is separate (see
> Table B4-35 in ARM DDI 0406C.c).
> 
> Therefore we need to be consistent and also flush the branch predictor for
> unified cache.
> 
> > 
> > Suggested-by: Julien Grall <julien.grall@arm.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > ---
> > Cc: Julien Grall <julien.grall@arm.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > v2: First submission
> > ---
> >  xen/arch/arm/livepatch.c         | 3 +++
> >  xen/include/asm-arm/arm32/page.h | 5 ++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> > index c290602..cdb8d65 100644
> > --- a/xen/arch/arm/livepatch.c
> > +++ b/xen/arch/arm/livepatch.c
> > @@ -45,6 +45,9 @@ void arch_livepatch_revive(void)
> >      /*
> >       * Nuke the instruction cache. Data cache has been cleaned before in
> >       * arch_livepatch_apply_jmp.
> > +     *
> > +     * Need to flush the branch predictor for ARMv7 as it may be
> > +     * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b).
> 
> I don't think this comment belongs to livepatch.c. It describes the behavior
> internal behavior of invalidate_icache.

Moved it to be right above invalidate_icache in xen/include/asm-arm/arm32/page.h.

Thanks!

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

Re: [PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2016 at 08:54:51AM -0600, Jan Beulich wrote:
> >>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
> > @@ -22,6 +24,12 @@ config X86
> >  	select NUMA
> >  	select VGA
> >  
> > +config ALTERNATIVE
> > +	bool
> > +
> > +config HAS_EX_TABLE
> > +	bool
> 
> These need to move out of arch/x86/, and the first one's name is
> too generic (and should probably also start with HAS_).

<nods> Done.

ARM64 is going to look a bit funny as it has ALTERNATIVE
sprinkled and now it will also have an HAS_ALTERNATIVE.

> 
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -3,6 +3,7 @@
> >   *
> >   */
> >  
> > +#include <xen/config.h>
> >  #include <xen/cpu.h>
> >  #include <xen/elf.h>
> >  #include <xen/err.h>
> 
> No new explicit inclusions of xen/config.h please.

/me scratches his head.

I need the 'generated/autoconf.h' for its generated names.

Oh, I can just do '#include <xen/kconfig.h>"!


> 
> Jan
> 

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

Re: [PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2016 at 08:56:07AM -0600, Jan Beulich wrote:
> >>> On 25.08.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
> > On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> > 
> >> x86 implements all of them by default - and we just
> >> add two extra CONFIG_ variables to be declared in autoconf.h.
> >>
> >> ARM 64 only has alternative while ARM 32 has none of them.
> >>
> >> And while at it change the livepatch common code that
> >> would benefit from this.
> >>
> >> Suggested-by: Julien Grall <julien.grall@arm.com>
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > Surely livepatch should select alternatives ?
> 
> DYM depend on? It clearly shouldn't select them, as whether
> they're present is determined by arch code.

And I am not sure if there is a need for Kconfig LIVEPATCH
entry to depend on it?

Especially as the common/livepatch.c has #ifef CONFIG_ALTERNATIVE
(and also CONFIG_HAS_EX_TABLE) - added by this patch - to deal with
architectures that do not have support them.

Ssuch as ARM 32 or ARM64 without errata support built-in.

I spun out an seperate patch that would thrown an -EOPNOTSUPP
if these sections are part of the payload and the hypervisor
was built without the support, like this:

(This is to be on top of the patch discussed - or if folks
prefer I can squash it in).

From 3a4953dd0f7d2411e5d638a82bfe57c0e16a22b3 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 6 Sep 2016 16:28:23 -0400
Subject: [PATCH] livepatch: Reject payloads with .alternative or .ex_table if
 support is not built-in.

ARM 64 can be built without alternative support (without the
errata support) - and it would sad if the payload loaded
had .alternative section but we did not support parsing it.

As this could lead to sad results instead of ignoring this
lets error out during loading.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3: New submission.
---
 xen/common/livepatch.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 73d4edb..342a5ec 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -685,10 +685,10 @@ static int prepare_payload(struct payload *payload,
                                   sizeof(*region->frame[i].bugs);
     }
 
-#ifdef CONFIG_HAS_ALTERNATIVE
     sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
     if ( sec )
     {
+#ifdef CONFIG_HAS_ALTERNATIVE
         struct alt_instr *a, *start, *end;
 
         if ( sec->sec->sh_size % sizeof(*a) )
@@ -715,13 +715,17 @@ static int prepare_payload(struct payload *payload,
             }
         }
         apply_alternatives(start, end);
-    }
+#else
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: We don't support alternative patching!\n",
+                elf->name);
+        return -EOPNOTSUPP;
 #endif
+    }
 
-#ifdef CONFIG_HAS_EX_TABLE
     sec = livepatch_elf_sec_by_name(elf, ".ex_table");
     if ( sec )
     {
+#ifdef CONFIG_HAS_EX_TABLE
         struct exception_table_entry *s, *e;
 
         if ( !sec->sec->sh_size ||
@@ -740,8 +744,12 @@ static int prepare_payload(struct payload *payload,
 
         region->ex = s;
         region->ex_end = e;
-    }
+#else
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: We don't support .ex_table!\n",
+                elf->name);
+        return -EOPNOTSUPP;
 #endif
+    }
 
     return 0;
 }
-- 
2.4.11


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

Re: [PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Konrad Rzeszutek Wilk]

On Tue, Sep 06, 2016 at 04:36:40PM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Aug 25, 2016 at 08:56:07AM -0600, Jan Beulich wrote:
> > >>> On 25.08.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
> > > On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> > > 
> > >> x86 implements all of them by default - and we just
> > >> add two extra CONFIG_ variables to be declared in autoconf.h.
> > >>
> > >> ARM 64 only has alternative while ARM 32 has none of them.
> > >>
> > >> And while at it change the livepatch common code that
> > >> would benefit from this.
> > >>
> > >> Suggested-by: Julien Grall <julien.grall@arm.com>
> > >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > 
> > > Surely livepatch should select alternatives ?
> > 
> > DYM depend on? It clearly shouldn't select them, as whether
> > they're present is determined by arch code.
> 
> And I am not sure if there is a need for Kconfig LIVEPATCH
> entry to depend on it?
> 
> Especially as the common/livepatch.c has #ifef CONFIG_ALTERNATIVE
> (and also CONFIG_HAS_EX_TABLE) - added by this patch - to deal with
> architectures that do not have support them.
> 
> Ssuch as ARM 32 or ARM64 without errata support built-in.

Err, ARM64 is always built with alternative, hence:

> 
> I spun out an seperate patch that would thrown an -EOPNOTSUPP
> if these sections are part of the payload and the hypervisor
> was built without the support, like this:
> 
> (This is to be on top of the patch discussed - or if folks
> prefer I can squash it in).
> 
> From 3a4953dd0f7d2411e5d638a82bfe57c0e16a22b3 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Tue, 6 Sep 2016 16:28:23 -0400
> Subject: [PATCH] livepatch: Reject payloads with .alternative or .ex_table if
>  support is not built-in.
> 
> ARM 64 can be built without alternative support (without the
> errata support) - and it would sad if the payload loaded
> had .alternative section but we did not support parsing it.

This would never happen.

So please ignore the patch below.
> 
> As this could lead to sad results instead of ignoring this
> lets error out during loading.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> v3: New submission.
> ---
>  xen/common/livepatch.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 73d4edb..342a5ec 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -685,10 +685,10 @@ static int prepare_payload(struct payload *payload,
>                                    sizeof(*region->frame[i].bugs);
>      }
>  
> -#ifdef CONFIG_HAS_ALTERNATIVE
>      sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
>      if ( sec )
>      {
> +#ifdef CONFIG_HAS_ALTERNATIVE
>          struct alt_instr *a, *start, *end;
>  
>          if ( sec->sec->sh_size % sizeof(*a) )
> @@ -715,13 +715,17 @@ static int prepare_payload(struct payload *payload,
>              }
>          }
>          apply_alternatives(start, end);
> -    }
> +#else
> +        dprintk(XENLOG_ERR, LIVEPATCH "%s: We don't support alternative patching!\n",
> +                elf->name);
> +        return -EOPNOTSUPP;
>  #endif
> +    }
>  
> -#ifdef CONFIG_HAS_EX_TABLE
>      sec = livepatch_elf_sec_by_name(elf, ".ex_table");
>      if ( sec )
>      {
> +#ifdef CONFIG_HAS_EX_TABLE
>          struct exception_table_entry *s, *e;
>  
>          if ( !sec->sec->sh_size ||
> @@ -740,8 +744,12 @@ static int prepare_payload(struct payload *payload,
>  
>          region->ex = s;
>          region->ex_end = e;
> -    }
> +#else
> +        dprintk(XENLOG_ERR, LIVEPATCH "%s: We don't support .ex_table!\n",
> +                elf->name);
> +        return -EOPNOTSUPP;
>  #endif
> +    }
>  
>      return 0;
>  }
> -- 
> 2.4.11
> 

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

Re: [PATCH v2 19/20] livepatch/elf: Adjust section aligment to word

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2016 at 09:11:15AM -0600, Jan Beulich wrote:
> >>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
> > On most architectures it does not matter what the aligment is.
> > 
> > On ARM32 it is paramount that the aligment is word-size (4)
> > otherwise we get a Data Abort when trying to perform ELF
> > relocations. That is due to ARM 32 only being able to write to
> > word-aligned addresses.
> 
> That's not exactly true, afaik: ARM can write to byte- and
> half-word-aligned addresses, but only bytes/half-words.
> 
> > --- a/xen/common/livepatch_elf.c
> > +++ b/xen/common/livepatch_elf.c
> > @@ -71,7 +71,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
> >          delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize;
> >  
> >          sec[i].sec = data + delta;
> > -
> > +        /*
> > +         * Some architectures REQUIRE section alignment to be word-size.
> > +         */
> 
> This is a single line comment.
> 
> > +        if ( sec[i].sec->sh_addralign % sizeof(uint32_t) )
> 
> Hmm, word size for ARM64 and x86-64 ought to be 8 bytes. Also you
> don't cover sec[i].sec->sh_addralign being zero (in fact any non-power-
> of-2 value would seem bogus to me). And then - why does this need to
> be done to all sections?

The issue I hit was relocations being done on .bug_frame section on ARM 32.

We had an .rel.bug_frame which would modify the .bug_frame and it would try
to use a uint32_t operation (R_ARM_REL32 if I recall correctly).
Since .bug_frame sh_addralign was 1, it ended up sadly with an
Data Abort exception.

The other sections that had .sh_addralign of 1 such as: livepatch.depends,
,.rodata.str1, and .strtab were OK - there was no problem with them having
byte alignment.

P.S.
On x86 and ARM64 the .bug_frame sections also have byte size alignment.

> 
> > +        {
> > +            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Adjusting aligment for section [%u]\n",
> > +                    elf->name, i);
> > +            ((Elf_Shdr *)sec[i].sec)->sh_addralign = 4;
> 
> And of course you know how I like such casting away of constness.

Heh.

Initially I was hoping the linker would have an --section-alignment like the ld PE one
which would make this a simple $LD_FLAGS change.

But without that the only other recourse I have (especially for the test-cases) is
to cobble up an livepatch.lds which I would need to sync with the xen.lds occassionaly.
That would guarantee that the livepatch'es would have the right section alignment and
then this patch would just return -EINVAL if the alignment was not to the power of 2.

Or something along this patch where we adjust it (and yes need to cast away the
constness).

Preferences?
> 
> Jan
> 

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

Re: [PATCH v2 13/20] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk]

> > +void arch_livepatch_apply_jmp(struct livepatch_func *func)
> > +{
> > +    uint32_t insn;
> > +    uint32_t *old_ptr;
> > +    uint32_t *new_ptr;
> > +
> > +    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> > +    BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
> > +
> > +    ASSERT(vmap_of_xen_text);
> > +
> > +    /* Save old one. */
> > +    old_ptr = func->old_addr;
> > +    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> 
> I don't see any value to use a temporary variable (old_ptr) to hold
> func->old_addr here.
> 
> > +
> > +    if ( func->new_addr )
> > +        insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
> > +                                           (unsigned long)func->new_addr,
> > +                                           AARCH64_INSN_BRANCH_NOLINK);
> > +    else
> > +        insn = aarch64_insn_gen_nop();
> > +
> > +    ASSERT(insn != AARCH64_BREAK_FAULT);
> 
> Could you document in the code what prevents aarch64_insn_gen_branch_imm to
> not generate a break fault instruction?

In the excitment of getting ARM32 implementation working - forgot
to write the code that would have done the check for all the platforms
(32MB for ARM, 128MB for ARM64, and 2GB on x86).

It will be an followup patch, like so (compile tested):

From 508157d81eaacab7ff621a84d7d885fb1bf689cc Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 6 Sep 2016 20:14:18 -0400
Subject: [PATCH] livepatch: ARM/x86: Check range of old_addr between new_addr

If the distance is too great we are in trouble - as our relocation
distance can surely be clipped, or still have a valid width - but
cause an overflow of distance.

On various architectures the maximum displacement for a branch/jump
varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86 for 32-bit
relocations is +/- 2G.

[Note: On x86 we could use the 64-bit jmpq instruction which
would provide much bigger displacement to do a jump, but we would
still have issues with the new function not being able to reach
any of the old functions (as all the relocations would assume 32-bit
displacement)].

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>

v3: New submission.
---
 xen/arch/arm/arm64/livepatch.c  |  1 +
 xen/common/livepatch.c          |  4 ++++
 xen/include/asm-arm/livepatch.h |  8 ++++++++
 xen/include/asm-x86/livepatch.h | 23 +++++++++++++++++++++++
 xen/include/xen/livepatch.h     | 18 +++++++++++++++++-
 5 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/asm-x86/livepatch.h

diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index e4d2926..27a68ab 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -35,6 +35,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
     else
         insn = aarch64_insn_gen_nop();
 
+    /* Verified in arch_livepatch_verify_distance. */
     ASSERT(insn != AARCH64_BREAK_FAULT);
     new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
 
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 86232b8..1ba6ca9 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -587,6 +587,10 @@ static int prepare_payload(struct payload *payload,
         rc = resolve_old_address(f, elf);
         if ( rc )
             return rc;
+
+        rc = arch_livepatch_verify_distance(f);
+        if ( rc )
+            return rc;
     }
 
     sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h
index 8c8d625..b1806d0 100644
--- a/xen/include/asm-arm/livepatch.h
+++ b/xen/include/asm-arm/livepatch.h
@@ -6,6 +6,8 @@
 #ifndef __XEN_ARM_LIVEPATCH_H__
 #define __XEN_ARM_LIVEPATCH_H__
 
+#include <xen/sizes.h> /* For SZ_* macros. */
+
 /* On ARM32,64 instructions are always 4 bytes long. */
 #define PATCH_INSN_SIZE 4
 
@@ -15,6 +17,12 @@
  */
 extern void *vmap_of_xen_text;
 
+#ifdef CONFIG_ARM_32
+#define LIVEPATCH_ARCH_RANGE SZ_32M
+#else
+#define LIVEPATCH_ARCH_RANGE SZ_128M
+#endif
+
 #endif /* __XEN_ARM_LIVEPATCH_H__ */
 
 /*
diff --git a/xen/include/asm-x86/livepatch.h b/xen/include/asm-x86/livepatch.h
new file mode 100644
index 0000000..4b77601
--- /dev/null
+++ b/xen/include/asm-x86/livepatch.h
@@ -0,0 +1,23 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#ifndef __XEN_X86_LIVEPATCH_H__
+#define __XEN_X86_LIVEPATCH_H__
+
+#include <xen/sizes.h> /* For SZ_* macros. */
+
+#define LIVEPATCH_ARCH_RANGE SZ_2G
+
+#endif /* __XEN_X86_LIVEPATCH_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 243e240..55553806 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -12,6 +12,7 @@ struct livepatch_elf_sym;
 struct xen_sysctl_livepatch_op;
 
 #include <xen/elfstructs.h>
+#include <xen/errno.h> /* For -ENOSYS or -EOVERFLOW */
 #ifdef CONFIG_LIVEPATCH
 
 /*
@@ -66,7 +67,23 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type types
 void arch_livepatch_init(void);
 
 #include <public/sysctl.h> /* For struct livepatch_func. */
+#include <asm/livepatch.h> /* For LIVEPATCH_ARCH_RANGE. */
 int arch_livepatch_verify_func(const struct livepatch_func *func);
+
+static inline int arch_livepatch_verify_distance(const struct livepatch_func *func)
+{
+    long offset;
+
+    if ( !func->new_addr ) /* Ignore NOPs. */
+        return 0;
+
+    offset = ((long)func->old_addr - (long)func->new_addr);
+    if ( offset < -LIVEPATCH_ARCH_RANGE || offset >= LIVEPATCH_ARCH_RANGE )
+        return -EOVERFLOW;
+
+    return 0;
+}
+
 /*
  * These functions are called around the critical region patching live code,
  * for an architecture to take make appropratie global state adjustments.
@@ -91,7 +108,6 @@ void arch_livepatch_unmask(void);
 #define init_or_livepatch_data        __initdata
 #define init_or_livepatch             __init
 
-#include <xen/errno.h> /* For -ENOSYS */
 static inline int livepatch_op(struct xen_sysctl_livepatch_op *op)
 {
     return -ENOSYS;
-- 
2.4.11

...snip..
> > +        case R_AARCH64_ABS32:
> > +            ovf = reloc_data(RELOC_OP_ABS, dest, val, 32);
> > +            break;
> 
> I have noticed that not all the relocations are implemented (e.g
> R_AARCH64_ABS16, R_AARCH64_MOVW_*...). I don't think there is anything
> preventing the compiler to use them. So is there any particular reasons to
> not include them?

I followed the same principle Ross did on x86 - only implement the ones that
the compiler has generated. And that is what I initially had in the v1, but expanded
it per request.

I can include more, but at what point should I stop?

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

Re: [PATCH v2 13/20] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2016 at 09:02:48AM -0600, Jan Beulich wrote:
> >>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/include/xen/types.h
> > +++ b/xen/include/xen/types.h
> > @@ -14,6 +14,12 @@
> >  #define NULL ((void*)0)
> >  #endif
> >  
> > +#define U16_MAX         ((u16)~0U)
> > +#define S16_MAX         ((s16)(U16_MAX>>1))
> > +#define S16_MIN         ((s16)(-S16_MAX - 1))
> > +#define U32_MAX         ((u32)~0U)
> > +#define S32_MAX         ((s32)(U32_MAX>>1))
> > +#define S32_MIN         ((s32)(-S32_MAX - 1))
> 
> These are rather strange constants: Fixed width types necessarily
> always have the same boundaries of representable values. Otoh
> the C standard has such constants too - maybe if we really want
> them we should rather use their names?

I got these from the Linux kernel - which may have added these
before the C99 standard.

But I agree that using the C ones should make this easier, so I pulled
them in from /usr/include/stdint.h
> 
> Jan
> 

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

Re: [PATCH v2 13/20] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk]

> ...snip..
> > > +        case R_AARCH64_ABS32:
> > > +            ovf = reloc_data(RELOC_OP_ABS, dest, val, 32);
> > > +            break;
> > 
> > I have noticed that not all the relocations are implemented (e.g
> > R_AARCH64_ABS16, R_AARCH64_MOVW_*...). I don't think there is anything
> > preventing the compiler to use them. So is there any particular reasons to
> > not include them?
> 
> I followed the same principle Ross did on x86 - only implement the ones that
> the compiler has generated. And that is what I initially had in the v1, but expanded
> it per request.
> 
> I can include more, but at what point should I stop?


 xen/arch/arm/arm64/livepatch.c       |  140 +++++++
 xen/include/xen/elfstructs.h         |   23 ++

lines later and I added them all in.

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

Re: [PATCH v2] Livepatch for ARM 64 and 32.

[Konrad Rzeszutek Wilk]

On Wed, Aug 31, 2016 at 09:54:30AM -0600, Jan Beulich wrote:
> >>> On 31.08.16 at 17:24, <andrew.cooper3@citrix.com> wrote:
> > On 31/08/16 16:09, Julien Grall wrote:
> >> On 31/08/16 16:06, Konrad Rzeszutek Wilk wrote:
> >>> So you are thinking of exposing this 'xen feature bits' that Andrew
> >>> mention
> >>> to be on ARM as well.
> >>
> >> I only chat with Andrew about it and I have not looked at the code, so
> >> I can't tell how it is on x86. I was just thinking to add LIVEPATCH
> >> bit in cpufeature.h for now and be worry later when cpufeature will be
> >> exposed to the toolstack for ARM.
> > 
> > Sorry - I have forgotten what exactly the purpose of this bit needs to
> > be, and therefore which bit of x86 I point you at.
> 
> I'm pretty sure talk here is about X86_FEATURE_ALWAYS.

Which is X86_FEATURE_LM, which is what we have in the test-case - and that
'lm' part is exposed to the toolstack.

Anyhow I believe Julien was thinking something like this:
(Would be nice if we could do arch-independent NOPs)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index a076982..549f908 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -8,6 +8,7 @@
 #include <xen/livepatch.h>
 #include <xen/vmap.h>
 
+#include <asm/cpufeature.h>
 #include <asm/livepatch.h>
 #include <asm/mm.h>
 
@@ -167,6 +168,8 @@ void __init arch_livepatch_init(void)
     end = (void *)LIVEPATCH_VMAP_END;
 
     vm_init_type(VMAP_XEN, start, end);
+
+    cpus_set_cap(LIVEPATCH_FEATURE);
 }
 
 /*
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 9f88fd9..074ff63 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -165,6 +165,8 @@ static inline int apply_alternatives(void *start, size_t lenght)
     return 0;
 }
 
+#define ALTERNATIVE(oldinstr, newinstr, ...) ""
+
 #endif
 
 #endif /* __ASM_ALTERNATIVE_H */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 66e930f..19e768b 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -40,7 +40,12 @@
 #define ARM32_WORKAROUND_766422 2
 #define ARM64_WORKAROUND_834220 3
 
+#ifdef CONFIG_LIVEPATCH
+#define LIVEPATCH_FEATURE   4
+#define ARM_NCAPS           5
+#else
 #define ARM_NCAPS           4
+#endif
 
 #ifndef __ASSEMBLY__
 
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index bcdf5d6..404dd91 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -34,6 +34,7 @@ XEN_CPUFEATURE(MFENCE_RDTSC,    (FSCAPINTS+0)*32+ 9) /* MFENCE synchronizes RDTS
 
 /* An alias of a feature we know is always going to be present. */
 #define X86_FEATURE_ALWAYS      X86_FEATURE_LM
+#define LIVEPATCH_FEATURE       X86_FEATURE_ALWAYS
 
 #if !defined(__ASSEMBLY__) && !defined(X86_FEATURES_ONLY)
 #include <xen/bitops.h>
diff --git a/xen/test/livepatch/xen_hello_world_func.c b/xen/test/livepatch/xen_hello_world_func.c
index 6f53ab4..6b41a55 100644
--- a/xen/test/livepatch/xen_hello_world_func.c
+++ b/xen/test/livepatch/xen_hello_world_func.c
@@ -20,7 +20,6 @@ const char *xen_hello_world(void)
     unsigned long tmp;
     int rc;
 
-    alternative(ASM_NOP8, ASM_NOP1, X86_FEATURE_LM);
     /*
      * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore
      * exceptions will be caught and processed properly.
@@ -28,8 +27,11 @@ const char *xen_hello_world(void)
     rc = __get_user(tmp, non_canonical_addr);
     BUG_ON(rc != -EFAULT);
 #endif
-#ifdef CONFIG_ARM_64
-    asm(ALTERNATIVE("nop", "nop", ARM64_WORKAROUND_CLEAN_CACHE));
+#ifdef CONFIG_ARM
+    asm(ALTERNATIVE("nop", "nop", LIVEPATCH_FEATURE));
+#endif
+#ifdef CONFIG_X86
+    asm(ALTERNATIVE(ASM_NOP8, ASM_NOP1, LIVEPATCH_FEATURE);
 #endif
     return "Hello World";
 }

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

Re: [PATCH v2 07/20] arm/x86: Add ALTERNATIVE and HAS_EX_TABLE

[Jan Beulich]

>>> On 06.09.16 at 22:16, <konrad.wilk@oracle.com> wrote:
> On Thu, Aug 25, 2016 at 08:54:51AM -0600, Jan Beulich wrote:
>> >>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
>> > --- a/xen/common/livepatch.c
>> > +++ b/xen/common/livepatch.c
>> > @@ -3,6 +3,7 @@
>> >   *
>> >   */
>> >  
>> > +#include <xen/config.h>
>> >  #include <xen/cpu.h>
>> >  #include <xen/elf.h>
>> >  #include <xen/err.h>
>> 
>> No new explicit inclusions of xen/config.h please.
> 
> /me scratches his head.
> 
> I need the 'generated/autoconf.h' for its generated names.
> 
> Oh, I can just do '#include <xen/kconfig.h>"!

I don't understand. The make rules force the inclusion of xen/config.h
by every translation unit (see xen/Rules.mk).

Jan


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

Re: [PATCH v2 19/20] livepatch/elf: Adjust section aligment to word

[Jan Beulich]

>>> On 06.09.16 at 23:18, <konrad.wilk@oracle.com> wrote:
> On Thu, Aug 25, 2016 at 09:11:15AM -0600, Jan Beulich wrote:
>> >>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
>> > --- a/xen/common/livepatch_elf.c
>> > +++ b/xen/common/livepatch_elf.c
>> > @@ -71,7 +71,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
>> >          delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize;
>> >  
>> >          sec[i].sec = data + delta;
>> > -
>> > +        /*
>> > +         * Some architectures REQUIRE section alignment to be word-size.
>> > +         */
>> 
>> This is a single line comment.
>> 
>> > +        if ( sec[i].sec->sh_addralign % sizeof(uint32_t) )
>> 
>> Hmm, word size for ARM64 and x86-64 ought to be 8 bytes. Also you
>> don't cover sec[i].sec->sh_addralign being zero (in fact any non-power-
>> of-2 value would seem bogus to me). And then - why does this need to
>> be done to all sections?
> 
> The issue I hit was relocations being done on .bug_frame section on ARM 32.
> 
> We had an .rel.bug_frame which would modify the .bug_frame and it would try
> to use a uint32_t operation (R_ARM_REL32 if I recall correctly).
> Since .bug_frame sh_addralign was 1, it ended up sadly with an
> Data Abort exception.

So then it's rather the insufficient alignment of .bug_frame which
needs fixing? As done recently to a few other sections, alignment
should always be properly expressed in the .o files already.
Artificially aligning sections in the linker script (for the base binary)
or in the loader (for patches) should never be a requirement.

>> > +        {
>> > +            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Adjusting aligment for 
> section [%u]\n",
>> > +                    elf->name, i);
>> > +            ((Elf_Shdr *)sec[i].sec)->sh_addralign = 4;
>> 
>> And of course you know how I like such casting away of constness.
> 
> Heh.
> 
> Initially I was hoping the linker would have an --section-alignment like the ld PE one
> which would make this a simple $LD_FLAGS change.
> 
> But without that the only other recourse I have (especially for the test-cases) is
> to cobble up an livepatch.lds which I would need to sync with the xen.lds occassionaly.
> That would guarantee that the livepatch'es would have the right section alignment and
> then this patch would just return -EINVAL if the alignment was not to the power of 2.
> 
> Or something along this patch where we adjust it (and yes need to cast away the
> constness).
> 
> Preferences?

See above - this should be fixed without linker scripts or alike.

Jan


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

Re: [PATCH v2 15/20] livepatch: Move test-cases to common

[Jan Beulich]

>>> On 06.09.16 at 19:16, <konrad.wilk@oracle.com> wrote:
> On Thu, Aug 25, 2016 at 09:05:27AM -0600, Jan Beulich wrote:
>> >>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote:
>> > --- a/MAINTAINERS
>> > +++ b/MAINTAINERS
>> > @@ -271,6 +271,7 @@ F:  tools/misc/xen-livepatch.c
>> >  F:  xen/arch/*/livepatch*
>> >  F:  xen/arch/*/*/livepatch*
>> >  F:  xen/common/livepatch*
>> > +F:  xen/test/livepatch/*
>> >  F:  xen/include/xen/livepatch*
>> 
>> Please keep this sorted.
> 
> A side effect of that is that * vs / gets moved too.
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7d42883..507bb40 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -268,10 +268,11 @@ M:  Ross Lagerwall <ross.lagerwall@citrix.com>
>  S:  Supported
>  F:  docs/misc/livepatch.markdown
>  F:  tools/misc/xen-livepatch.c
> -F:  xen/arch/*/livepatch*
>  F:  xen/arch/*/*/livepatch*
> +F:  xen/arch/*/livepatch*
>  F:  xen/common/livepatch*
>  F:  xen/include/xen/livepatch*
> +F:  xen/test/livepatch/*
> 
> You OK with that or would you prefer to leave it as is?

I'd prefer if you just put the addition at the right slot. I don't think
we mean to fully specify collating rules here. Alphabetical / numerical
sorting should be kept, but whether meta-characters like * are
meant to sort before or after alphanumeric ones is, I would say,
simply undefined here.

Jan


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

Re: [PATCH v2 13/20] livepatch: Initial ARM64 support.

[Julien Grall]

Hi Konrad,

On 07/09/2016 01:31, Konrad Rzeszutek Wilk wrote:
>>> +void arch_livepatch_apply_jmp(struct livepatch_func *func)
>>> +{
>>> +    uint32_t insn;
>>> +    uint32_t *old_ptr;
>>> +    uint32_t *new_ptr;
>>> +
>>> +    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
>>> +    BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
>>> +
>>> +    ASSERT(vmap_of_xen_text);
>>> +
>>> +    /* Save old one. */
>>> +    old_ptr = func->old_addr;
>>> +    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
>>
>> I don't see any value to use a temporary variable (old_ptr) to hold
>> func->old_addr here.
>>
>>> +
>>> +    if ( func->new_addr )
>>> +        insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
>>> +                                           (unsigned long)func->new_addr,
>>> +                                           AARCH64_INSN_BRANCH_NOLINK);
>>> +    else
>>> +        insn = aarch64_insn_gen_nop();
>>> +
>>> +    ASSERT(insn != AARCH64_BREAK_FAULT);
>>
>> Could you document in the code what prevents aarch64_insn_gen_branch_imm to
>> not generate a break fault instruction?
>
> In the excitment of getting ARM32 implementation working - forgot
> to write the code that would have done the check for all the platforms
> (32MB for ARM, 128MB for ARM64, and 2GB on x86).
>
> It will be an followup patch, like so (compile tested):
>
> From 508157d81eaacab7ff621a84d7d885fb1bf689cc Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Tue, 6 Sep 2016 20:14:18 -0400
> Subject: [PATCH] livepatch: ARM/x86: Check range of old_addr between new_addr
>
> If the distance is too great we are in trouble - as our relocation
> distance can surely be clipped, or still have a valid width - but
> cause an overflow of distance.
>
> On various architectures the maximum displacement for a branch/jump
> varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86 for 32-bit
> relocations is +/- 2G.

It would be worth noting that the ARM64 and ARM32 displacement are only 
valid for unconditional branch (conditional one supports a smaller 
displacement). And livepatch is only using unconditional branch.

>
> [Note: On x86 we could use the 64-bit jmpq instruction which
> would provide much bigger displacement to do a jump, but we would
> still have issues with the new function not being able to reach
> any of the old functions (as all the relocations would assume 32-bit
> displacement)].
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
>
> v3: New submission.
> ---
>  xen/arch/arm/arm64/livepatch.c  |  1 +
>  xen/common/livepatch.c          |  4 ++++
>  xen/include/asm-arm/livepatch.h |  8 ++++++++
>  xen/include/asm-x86/livepatch.h | 23 +++++++++++++++++++++++
>  xen/include/xen/livepatch.h     | 18 +++++++++++++++++-
>  5 files changed, 53 insertions(+), 1 deletion(-)
>  create mode 100644 xen/include/asm-x86/livepatch.h
>
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index e4d2926..27a68ab 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -35,6 +35,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
>      else
>          insn = aarch64_insn_gen_nop();
>
> +    /* Verified in arch_livepatch_verify_distance. */
>      ASSERT(insn != AARCH64_BREAK_FAULT);
>      new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 86232b8..1ba6ca9 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -587,6 +587,10 @@ static int prepare_payload(struct payload *payload,
>          rc = resolve_old_address(f, elf);
>          if ( rc )
>              return rc;
> +
> +        rc = arch_livepatch_verify_distance(f);
> +        if ( rc )
> +            return rc;
>      }
>
>      sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
> diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h
> index 8c8d625..b1806d0 100644
> --- a/xen/include/asm-arm/livepatch.h
> +++ b/xen/include/asm-arm/livepatch.h
> @@ -6,6 +6,8 @@
>  #ifndef __XEN_ARM_LIVEPATCH_H__
>  #define __XEN_ARM_LIVEPATCH_H__
>
> +#include <xen/sizes.h> /* For SZ_* macros. */
> +
>  /* On ARM32,64 instructions are always 4 bytes long. */
>  #define PATCH_INSN_SIZE 4
>
> @@ -15,6 +17,12 @@
>   */
>  extern void *vmap_of_xen_text;
>
> +#ifdef CONFIG_ARM_32
> +#define LIVEPATCH_ARCH_RANGE SZ_32M
> +#else
> +#define LIVEPATCH_ARCH_RANGE SZ_128M
> +#endif

Sorry for been picky, can you document where these values come from:

- ARM32: A4.3 IN ARM DDI 0406C.j and we are using only ARM instruction 
in Xen.
- ARM64: C1.3.2 in ARM DDI 0487A.j

It might also be worth to mention that this is only valid for 
unconditional branch.

The rest looks good to me.

> +
>  #endif /* __XEN_ARM_LIVEPATCH_H__ */


Regards,

-- 
Julien Grall

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

Re: [PATCH v2 13/20] livepatch: Initial ARM64 support.

[Julien Grall]

On 07/09/2016 04:33, Konrad Rzeszutek Wilk wrote:
>> ...snip..
>>>> +        case R_AARCH64_ABS32:
>>>> +            ovf = reloc_data(RELOC_OP_ABS, dest, val, 32);
>>>> +            break;
>>>
>>> I have noticed that not all the relocations are implemented (e.g
>>> R_AARCH64_ABS16, R_AARCH64_MOVW_*...). I don't think there is anything
>>> preventing the compiler to use them. So is there any particular reasons to
>>> not include them?
>>
>> I followed the same principle Ross did on x86 - only implement the ones that
>> the compiler has generated. And that is what I initially had in the v1, but expanded
>> it per request.
>>
>> I can include more, but at what point should I stop?

Good question. My question was more, would it ever be possible to be 
generated by the same compiler when applying a patch?

>
>
>  xen/arch/arm/arm64/livepatch.c       |  140 +++++++
>  xen/include/xen/elfstructs.h         |   23 ++
>
> lines later and I added them all in.
>

-- 
Julien Grall

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

Re: [PATCH v2 13/20] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk]

On Wed, Sep 07, 2016 at 11:43:27AM +0100, Julien Grall wrote:
> 
> 
> On 07/09/2016 04:33, Konrad Rzeszutek Wilk wrote:
> > > ...snip..
> > > > > +        case R_AARCH64_ABS32:
> > > > > +            ovf = reloc_data(RELOC_OP_ABS, dest, val, 32);
> > > > > +            break;
> > > > 
> > > > I have noticed that not all the relocations are implemented (e.g
> > > > R_AARCH64_ABS16, R_AARCH64_MOVW_*...). I don't think there is anything
> > > > preventing the compiler to use them. So is there any particular reasons to
> > > > not include them?
> > > 
> > > I followed the same principle Ross did on x86 - only implement the ones that
> > > the compiler has generated. And that is what I initially had in the v1, but expanded
> > > it per request.
> > > 
> > > I can include more, but at what point should I stop?
> 
> Good question. My question was more, would it ever be possible to be
> generated by the same compiler when applying a patch?

If the hypervisor did not have them, then the payload will most likely not have
them either.

And vice-versa - if hypervisor does have them, then the payload will most likely
have them.

Either way I think we are debating academics as I already added all that code in :-)
> 
> > 
> > 
> >  xen/arch/arm/arm64/livepatch.c       |  140 +++++++
> >  xen/include/xen/elfstructs.h         |   23 ++
> > 
> > lines later and I added them all in.
> > 
> 
> -- 
> Julien Grall

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

Re: [PATCH v2 20/20] livepatch: ARM32 support.

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2016 at 09:37:35AM -0400, Konrad Rzeszutek Wilk wrote:
> The patch piggybacks on: livepatch: Initial ARM64 support, which
> brings up all of the neccessary livepatch infrastructure pieces in.
> 
> This patch adds three major pieces:
> 
>  1) ELF relocations. ARM32 uses SHT_REL instead of SHT_RELA which
>     means the adddendum had to be extracted from within the
>     instruction. Which required parsing BL/BLX, B/BL<cond>,
>     MOVT, and MOVW instructions.
> 
>     The code was written from scratch using the ARM ELF manual
>     (and the ARM Architecture Reference Manual)
> 
>  2) Inserting an trampoline. We use the B (branch to address)
>     which uses an offset that is based on the PC value: PC + imm32.
>     Because we insert the branch at the start of the old function
>     we have to account for the instruction already being fetched
>     and subtract -4 from the delta (new_addr - old_addr).
> 
>  3) Allows the test-cases to be built under ARM 32.
>     The "livepatch: tests: Make them compile under ARM64"
>     put in the right infrastructure for it and we piggyback on it.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> 
> v2: First submission.
> ---
>  xen/arch/arm/arm32/livepatch.c | 252 ++++++++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/arm64/livepatch.c |   7 ++
>  xen/arch/arm/livepatch.c       |   7 --
>  xen/common/Kconfig             |   2 +-
>  xen/include/xen/elfstructs.h   |  24 +++-
>  xen/test/Makefile              |   2 -
>  xen/test/livepatch/Makefile    |   3 +
>  7 files changed, 284 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index c33b68d..63e450b 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -3,28 +3,276 @@
>   */
>  
>  #include <xen/errno.h>
> +#include <xen/kernel.h>
>  #include <xen/lib.h>
>  #include <xen/livepatch_elf.h>
>  #include <xen/livepatch.h>
>  
> +#include <asm/page.h>
> +#include <asm/livepatch.h>
> +
>  void arch_livepatch_apply_jmp(struct livepatch_func *func)
>  {
> +    uint32_t insn;
> +    uint32_t *old_ptr;

This is now removed.
> +    uint32_t *new_ptr;
> +
> +    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> +    BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
> +
> +    ASSERT(vmap_of_xen_text);
> +
> +    /* Save old one. */
> +    old_ptr = func->old_addr;
> +    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);

Which makes this smaller.
> +
> +    if ( func->new_addr )
> +    {
> +        s32 delta;
> +
> +        /*
> +         * The -4 is to account for the b <offset> instruction placed at
> +         * the start of the func->old_addr.
> +         */
> +        delta = (s32)(func->new_addr - func->old_addr - 4);

And I made this a bit simpler:

	delta = (s32)func->new_addr - (s32)func->old_addr - PATCH_INSN_SIZE;

Along with a comment refering to the ARM DDI 0406C.c  A8.8.18

Anyhow, when I posted this patch I was excited that everything "worked". 

But a more dilligient test showed that in fact the SP is being corrupted.

That is if I call 'xl info' before patching (with this inline patch):

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index d0edb13..793e219 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -240,6 +240,7 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         xen_extraversion_t extraversion;
 
         memset(extraversion, 0, sizeof(extraversion));
+        printk("%s: %p %p\n", &extraversion, xen_extra_version());
         safe_strcpy(extraversion, deny ? xen_deny() : xen_extra_version());
         if ( copy_to_guest(arg, extraversion, ARRAY_SIZE(extraversion)) )
             return -EFAULT;

I get:
(XEN) do_xen_version: dst=43fd7ad8 src=0028b020

OK, good.

With the hypervisor being patched I get:

(XEN) do_xen_version: dst=ffffffe8 src=00805038

The src is corrected - it points to the payload .rodata section.

But the SP is all messed up! And that ends in tears with the hypervisor:
 Assertion 'diff < STACK_SIZE' failed at traps.c:864


Decoding the instructions that are being called (the new xen_extra_version())
yields:

   0:   e52db004        push	{fp}
   4:   e28db000        add     fp, sp, #0
   8:   e3050038        movw    r0, #20536      ; 0x5038
   c:   e3400080        movt    r0, #128        ; 0x80
  10:   e24bd000        sub     sp, fp, #0
  14:   e12fff1e        bx      lr

(This is after the relocation has been done).
And the unconditional branch that is put in the old xen_extra_version
is: 

 0:   ea1710d0        b       0x5c4348

	Which is correct too - to check for correctness I added two brkp.
	One (71 00 20 e1) at the start of the new 'xen_extra_version'.
	And then another four bytes in front of it (72 02 20 e1). The signature
	of them is different and the exception we hit was the first Prefetch Abort
        (71 00 20 e1).

Anyhow something is amiss here. I am not sure if there are some hidden
semantics in regard to a B condition.

I am going to try to patch in the old xen_extra_version an BL instruction
followed by BX LR and see if that makes a difference.

But in the meantime please ignore this patch. It needs some more
work.

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