Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Aug 15, 2016 at 04:27:12PM +0100, Andrew Cooper wrote:
> On 15/08/16 16:25, Julien Grall wrote:
> >
> >
> > On 15/08/2016 17:17, Konrad Rzeszutek Wilk wrote:
> >> On Mon, Aug 15, 2016 at 04:57:26PM +0200, Julien Grall wrote:
> >>> Hi Jan and Konrad,
> >>>
> >>> On 15/08/2016 16:23, Jan Beulich wrote:
> >>>>>>> On 15.08.16 at 16:09, <konrad.wilk@oracle.com> wrote:
> >>>>> On Mon, Aug 15, 2016 at 02:21:48AM -0600, Jan Beulich wrote:
> >>>>>>>>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote:
> >>>>>>> @@ -711,9 +711,15 @@ static int prepare_payload(struct payload
> >>>>>>> *payload,
> >>>>>>>                  return -EINVAL;
> >>>>>>>              }
> >>>>>>>          }
> >>>>>>> +#ifndef CONFIG_ARM
> >>>>>>>          apply_alternatives_nocheck(start, end);
> >>>>>>> +#else
> >>>>>>> +        apply_alternatives(start, sec->sec->sh_size);
> >>>>>>> +#endif
> >>>>>>
> >>>>>> Conditionals like this are ugly - can't this be properly abstracted?
> >>>>>
> >>>>> Yes, I can introduce an apply_alternatives_nocheck on ARM that will
> >>>>> hava the same set of arguments on x86.
> >>>>>
> >>>>> Or I can make a new function name?
> >>>>
> >>>> Either way is fine with me, with a slight preference to the former
> >>>> one.
> >>>
> >>> I am fine with the prototype of the function
> >>> apply_alternatives_nocheck but
> >>> I don't think the name is relevant for ARM.
> >>>
> >>> Is there any reason we don't want to call directly
> >>> apply_alternatives in
> >>> x86?
> >>
> >> It assumes (and has an ASSERT) that it is called with interrupts
> >> disabled.
> >> And we don't need to do that (as during livepatch loading we can
> >> modify the
> >> livepatch payload without worrying about interrupts).
> >
> > Oh, it makes more sense now.
> >
> >>
> >> P.S.
> >> loading != applying.
> >>
> >> I could do a patch where we rename 'apply_alternatives' ->
> >> 'apply_alternatives_boot'
> >> and 'apply_alternatives_nocheck' to 'apply_alternatives'.
> 
> The only reason apply_alternatives() is named thusly is to match Linux. 
> I am not fussed if it changes.

Would this be OK with folks?

There is a bit of disreprancy - ARM has 'const struct alt_instr *'
where the 'const' gets dropped later on. That can't be done x86
as 'apply_alternatives' at the gecko modifies the structure.

Thoughts? Make it the same on ARM and x86?

From 2c26d4d214926cd23b73f98c1fdaecd98b010da6 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 16 Aug 2016 22:20:54 -0400
Subject: [PATCH] alternatives: x86 rename and change parameters on ARM

On x86 we rename 'apply_alternatives' -> 'apply_alternatives_boot'
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.

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>

v3.1: First submission.
---
 xen/arch/arm/alternative.c        | 4 ++--
 xen/arch/x86/alternative.c        | 8 ++++----
 xen/common/livepatch.c            | 2 +-
 xen/include/asm-arm/alternative.h | 2 +-
 xen/include/asm-x86/alternative.h | 5 ++---
 5 files changed, 10 insertions(+), 11 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..7addc2c 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -144,7 +144,7 @@ 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(struct alt_instr *start, struct alt_instr *end)
 {
     struct alt_instr *a;
     u8 *instr, *replacement;
@@ -187,7 +187,7 @@ 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)
+void __init apply_alternatives_boot(struct alt_instr *start, struct alt_instr *end)
 {
     unsigned long cr0 = read_cr0();
 
@@ -196,7 +196,7 @@ void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
     /* Disable WP to allow application of alternatives to read-only pages. */
     write_cr0(cr0 & ~X86_CR0_WP);
 
-    apply_alternatives_nocheck(start, end);
+    apply_alternatives(start, end);
 
     /* Reinstate WP. */
     write_cr0(cr0);
@@ -225,7 +225,7 @@ void __init alternative_instructions(void)
      * expect a machine check to cause undue problems during to code
      * patching.
      */
-    apply_alternatives(__alt_instructions, __alt_instructions_end);
+    apply_alternatives_boot(__alt_instructions, __alt_instructions_end);
 
     set_nmi_callback(saved_nmi_callback);
 }
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 238492a..4458751 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -698,7 +698,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 58d5fa7..b6fa827 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -26,7 +26,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 b72c401..47fa1be 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);
+/* Similar to apply_alternatives_boot except it can be run with IRQs enabled. */
 extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
+extern void apply_alternatives_boot(struct alt_instr *start, 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