Xen project Mailing List

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

To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Date: Tue, 16 Aug 2016 22:45:58 -0400

Cc: sstabellini@xxxxxxxxxx, ross.lagerwall@xxxxxxxxxx, Julien Grall <julien.grall@xxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx

Delivery-date: Wed, 17 Aug 2016 02:46:35 +0000

List-id: Xen developer discussion <xen-devel.lists.xen.org>

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@xxxxxxxxxx> wrote: > >>>>> On Mon, Aug 15, 2016 at 02:21:48AM -0600, Jan Beulich wrote: > >>>>>>>>> On 15.08.16 at 01:07, <konrad.wilk@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> --- Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx> Cc: Julien Grall <julien.grall@xxxxxxx> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> Cc: Jan Beulich <JBeulich@xxxxxxxx> 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.