| Message ID | 1471216074-3007-7-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -222,7 +222,7 @@ endmenu > config LIVEPATCH > bool "Live patching support (TECH PREVIEW)" > default n > - depends on X86 && HAS_BUILD_ID = "y" > + depends on (X86 || ARM_64) && HAS_BUILD_ID = "y" Would this better become a black list? > @@ -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? > --- 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 Aren't these ARM32 definitions, which should be unneeded for ARM64 support? > @@ -171,6 +180,7 @@ typedef struct { > #define EM_PPC 20 /* PowerPC */ > #define EM_PPC64 21 /* PowerPC 64-bit */ > #define EM_ARM 40 /* Advanced RISC Machines ARM */ > +#define EM_AARCH64 183 /* ARM 64-bit */ > #define EM_ALPHA 41 /* DEC ALPHA */ > #define EM_SPARCV9 43 /* SPARC version 9 */ > #define EM_ALPHA_EXP 0x9026 /* DEC ALPHA */ I think this tries to be sorted by number. > +/* > + * 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_ADD_ABS_LO12_NC 277 /* ADD imm. [11:0]. S+A, NC */ > + > +#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 */ No R_AARCH64_TSTBR14? > +#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 */ What about R_AARCH64_LDST128_ABS_LO12_NC? Jan
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: > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -222,7 +222,7 @@ endmenu > > config LIVEPATCH > > bool "Live patching support (TECH PREVIEW)" > > default n > > - depends on X86 && HAS_BUILD_ID = "y" > > + depends on (X86 || ARM_64) && HAS_BUILD_ID = "y" > > Would this better become a black list? OK, that would make it easier. > > > @@ -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? > > > --- 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 > > Aren't these ARM32 definitions, which should be unneeded for > ARM64 support? Correct. Let me move that and also the arch/arm/arm32/livepatch:arch_livepatch_verify_elf code out of this patch. > > > @@ -171,6 +180,7 @@ typedef struct { > > #define EM_PPC 20 /* PowerPC */ > > #define EM_PPC64 21 /* PowerPC 64-bit */ > > #define EM_ARM 40 /* Advanced RISC Machines ARM */ > > +#define EM_AARCH64 183 /* ARM 64-bit */ > > #define EM_ALPHA 41 /* DEC ALPHA */ > > #define EM_SPARCV9 43 /* SPARC version 9 */ > > #define EM_ALPHA_EXP 0x9026 /* DEC ALPHA */ > > I think this tries to be sorted by number. > > > +/* > > + * 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_ADD_ABS_LO12_NC 277 /* ADD imm. [11:0]. S+A, NC */ > > + > > +#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 */ > > No R_AARCH64_TSTBR14? > > > +#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 */ > > What about R_AARCH64_LDST128_ABS_LO12_NC? I didnt' see it in the ELF of xen-syms or the livepatch tests cases so I omitted them. But then I added some that weren't in the ELF files either: LDST16 and LDST8. I will add the two missing ones and also sort this. Thanks! > > Jan >
>>> 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. Jan
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? Regards,
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). P.S. loading != applying. I could do a patch where we rename 'apply_alternatives' -> 'apply_alternatives_boot' and 'apply_alternatives_nocheck' to 'apply_alternatives'. Also the x86 version instead of having: apply_alternatives(struct alt_instr *start, struct alt_instr *end) would end up with: apply_alternatives(void *start, size_t length) to comply with ARM version. > > Regards, > > -- > Julien Grall
Hi Konrad, On 15/08/2016 01:07, Konrad Rzeszutek Wilk wrote: [...] > diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile > index b20db64..5966de0 100644 > --- a/xen/arch/arm/arm32/Makefile > +++ b/xen/arch/arm/arm32/Makefile > @@ -4,8 +4,8 @@ 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 > obj-y += vfp.o > - Spurious change? > diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c > new file mode 100644 > index 0000000..89ee2f5 > --- /dev/null > +++ b/xen/arch/arm/arm32/livepatch.c > @@ -0,0 +1,55 @@ > +/* > + * 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) > +{ > + 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; I would prefer if you introduce ARM32 implementation in one go. I.e can you only return -EOPNOTSUPP here for now? [...] > +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; > + > + for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ ) > + { > + int err = 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 */ > + > + /* 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; As you borrow the code from Linux, could we keep the abstraction with reloc_data and defer the overflow check? It would avoid to have the same if in multiple place in this code. > + *(int64_t *)dest = val; > + break; > + > + case R_AARCH64_ABS32: > + *(int32_t *)dest = val; > + if ( (int64_t)val != *(int32_t *)dest ) > + err = -EOVERFLOW; > + break; > + > + case R_AARCH64_PREL64: > + if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size ) > + goto bad_offset; > + > + val -= (uint64_t)dest; > + *(int64_t *)dest = val; > + break; > + > + case R_AARCH64_PREL32: > + val -= (uint64_t)dest; > + *(int32_t *)dest = val; > + if ( (int64_t)val != *(int32_t *)dest ) > + err = -EOVERFLOW; > + break; > + > + /* Instructions. */ > + case R_AARCH64_ADR_PREL_LO21: > + val -= (uint64_t)dest; > + err = reloc_insn_imm(dest, val, 0, 21, AARCH64_INSN_IMM_ADR); > + break; > + It seems the implementation of R_AARCH64_ADR_PREL_PG_HI21_NC is missing. > + case R_AARCH64_ADR_PREL_PG_HI21: > + val = (val & ~0xfff) - ((u64)dest & ~0xfff); > + err = reloc_insn_imm(dest, val, 12, 21, AARCH64_INSN_IMM_ADR); > + break; Hmmm, I think we want to avoid the payload having R_AARCH64_ADR_PREL_PG_HI21* relocations due to the erratum #843419 on some Cortex-A53. I haven't yet looked at how you build the payload, but this may also affects the process to do it. I will give a look on it. > + > + case R_AARCH64_LDST8_ABS_LO12_NC: > + case R_AARCH64_ADD_ABS_LO12_NC: > + err = reloc_insn_imm(dest, val, 0, 12, AARCH64_INSN_IMM_12); > + if ( err == -EOVERFLOW ) > + err = 0; > + break; > + > + case R_AARCH64_LDST16_ABS_LO12_NC: > + err = reloc_insn_imm(dest, val, 1, 11, AARCH64_INSN_IMM_12); > + if ( err == -EOVERFLOW ) > + err = 0; > + break; > + > + case R_AARCH64_LDST32_ABS_LO12_NC: > + err = reloc_insn_imm(dest, val, 2, 10, AARCH64_INSN_IMM_12); > + if ( err == -EOVERFLOW ) > + err = 0; > + break; > + > + case R_AARCH64_LDST64_ABS_LO12_NC: > + err = reloc_insn_imm(dest, val, 3, 9, AARCH64_INSN_IMM_12); > + if ( err == -EOVERFLOW ) > + err = 0; > + break; > + > + case R_AARCH64_CONDBR19: > + err = reloc_insn_imm(dest, val, 2, 19, AARCH64_INSN_IMM_19); > + break; > + > + case R_AARCH64_JUMP26: > + case R_AARCH64_CALL26: > + val -= (uint64_t)dest; > + err = reloc_insn_imm(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 ( err ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n", > + elf->name, i, rela->name, base->name); > + return err; > + } > + } > + 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: > + */ Cheers,
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'. > > Also the x86 version instead of having: > > apply_alternatives(struct alt_instr *start, struct alt_instr *end) > > would end up with: > > apply_alternatives(void *start, size_t length) I would be fine with the prototype apply_alternatives(struct alt_instr *start, struct alt_instr *end) for ARM. It is up to you. Cheers,
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. ~Andrew >> >> Also the x86 version instead of having: >> >> apply_alternatives(struct alt_instr *start, struct alt_instr *end) >> >> would end up with: >> >> apply_alternatives(void *start, size_t length) > > I would be fine with the prototype > apply_alternatives(struct alt_instr *start, struct alt_instr *end) > > for ARM. It is up to you. > > Cheers, >
> > +int arch_livepatch_perform_rela(struct livepatch_elf *elf, > > + const struct livepatch_elf_sec *base, > > + const struct livepatch_elf_sec *rela) > > +{ .. snip.. > > + 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; > > As you borrow the code from Linux, could we keep the abstraction with > reloc_data and defer the overflow check? It would avoid to have the same if > in multiple place in this code. The above 'if' conditional is a check to make sure that we don't go past the section (sh_size). In other words it is a boundary check to make sure the Elf file is not messed up. I can still copy the reloc_data so we lessen the: > > + if ( (int64_t)val != *(int32_t *)dest ) > > + err = -EOVERFLOW; And such.
Hi Konrad, On 15/08/16 16:17, Julien Grall wrote: > On 15/08/2016 01:07, Konrad Rzeszutek Wilk wrote: >> + case R_AARCH64_ADR_PREL_PG_HI21: >> + val = (val & ~0xfff) - ((u64)dest & ~0xfff); >> + err = reloc_insn_imm(dest, val, 12, 21, >> AARCH64_INSN_IMM_ADR); >> + break; > > Hmmm, I think we want to avoid the payload having > R_AARCH64_ADR_PREL_PG_HI21* relocations due to the erratum #843419 on > some Cortex-A53. > > I haven't yet looked at how you build the payload, but this may also > affects the process to do it. I will give a look on it. Actually, I am not sure I will have time to look at it during the next few weeks. Could you add a TODO for now? Cheers,
Hi Konrad, On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote: [...] > diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c > new file mode 100644 > index 0000000..e348942 > --- /dev/null > +++ b/xen/arch/arm/arm64/livepatch.c > @@ -0,0 +1,247 @@ > +/* > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > + */ > + > +#include "../livepatch.h" > +#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> > + > +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_size ) > + insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr, > + (unsigned long)func->new_addr, > + AARCH64_INSN_BRANCH_NOLINK); The function aarch64_insn_gen_branch_imm will fail to create the branch if the difference between old_addr and new_addr is greater than 128MB. In this case, the function will return AARCH64_BREAK_FAULT which will crash Xen when the instruction is executed. I cannot find any sanity check in the hypervisor code. So are we trusting the payload? > + else > + insn = aarch64_insn_gen_nop(); > + > + 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; > +} > + [...] > 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 3093554..19f6033 100644 > --- a/xen/arch/arm/livepatch.c > +++ b/xen/arch/arm/livepatch.c > @@ -1,56 +1,93 @@ > /* > * Copyright (C) 2016 Citrix Systems R&D Ltd. > */ > + Spurious change? > +#include "livepatch.h" > #include <xen/errno.h> > #include <xen/init.h> > #include <xen/lib.h> > #include <xen/livepatch_elf.h> > #include <xen/livepatch.h> > +#include <xen/vmap.h> > + > +#include <asm/mm.h> > > -/* On ARM32,64 instructions are always 4 bytes long. */ > -#define PATCH_INSN_SIZE 4 Rather than moving again PATCH_INSN_SIZE in this patch. Can you directly move it in patch [1]? > +void *vmap_of_xen_text; > > int arch_verify_insn_length(unsigned long len) > { > return len != PATCH_INSN_SIZE; > } > > -void arch_livepatch_quiesce(void) > +int arch_livepatch_quiesce(void) It would have been nice to move the prototype change out of this patch to keep it "straight forward". > { > + mfn_t text_mfn; > + unsigned int text_order; > + > + if ( vmap_of_xen_text ) > + return -EINVAL; > + > + text_mfn = _mfn(virt_to_mfn(_stext)); > + text_order = get_order_from_bytes(_end - _start); It is a bit odd that you use _stext before and the _start. But I won't complain as I did the same in alternatives.c :/. However, I think it should be enough to remap _stext -> _etext. I had to map all the Xen binary for the alternatives because we may patch the init text. I forgot to mention it in the code, so I will send a patch to update it. > + > + /* > + * 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. It has been cleaned before in I guess you want to replace "It" by "Data cache" otherwise it does not make much sense. > + * arch_livepatch_apply_jmp. What about the payload? It may contain instructions, so we need to ensure that all the data reached the memory. > + */ > + invalidate_icache(); > + > + if ( vmap_of_xen_text ) > + vunmap(vmap_of_xen_text); > + > + vmap_of_xen_text = NULL; > + > + /* > + * Need to flush the branch predicator for ARMv7 as it may be s/predicator/predictor/ > + * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b). > + */ > + flush_xen_text_tlb_local(); I am a bit confused. In your comment you mention the branch but flush the TLBs. The two are not related. However, I would prefer the branch predictor to be flushed directly in invalidate_icache by calling BPIALLIS. This is because flushing the cache means that you likely want to flush the branch predictor too. > } > > int arch_livepatch_verify_func(const struct livepatch_func *func) > { > - return -ENOSYS; > -} > - > -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, > @@ -60,20 +97,34 @@ 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; > + > + ASSERT(va); > + ASSERT(pages); > + > + if ( type == LIVEPATCH_VA_RX ) > + flags = PTE_RO; /* R set, NX clear */ > + else if ( type == LIVEPATCH_VA_RW ) > + flags = PTE_NX; /* R clear, NX set */ > + else > + flags = PTE_NX | PTE_RO; /* R set, NX set */ va_type is an enum. So I would prefer to see a switch. So we can catch more easily any addition of a new member. > + > + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags); > + > + return 0; > } > > void __init arch_livepatch_init(void) > { > + void *start, *end; > + > + start = (void *)LIVEPATCH_VMAP; > + end = start + MB(2); Could you define the in config.h? So in the future we can rework more easily the memory layout. > + > + vm_init_type(VMAP_XEN, start, end); > } > > /* > diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h > new file mode 100644 > index 0000000..8c8d625 > --- /dev/null > +++ b/xen/arch/arm/livepatch.h I am not sure why this header is living in arch/arm/ and not include/asm-arm/ > @@ -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/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c > index 06c67bc..e3f3f37 100644 > --- a/xen/arch/x86/livepatch.c > +++ b/xen/arch/x86/livepatch.c > @@ -15,10 +15,12 @@ > > #define PATCH_INSN_SIZE 5 > > -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/Kconfig b/xen/common/Kconfig > index 51afa24..2fc76b6 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -222,7 +222,7 @@ endmenu > config LIVEPATCH > bool "Live patching support (TECH PREVIEW)" > default n > - depends on X86 && HAS_BUILD_ID = "y" > + depends on (X86 || ARM_64) && 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/common/livepatch.c b/xen/common/livepatch.c > index 9c45270..af9443d 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload, > sizeof(*region->frame[i].bugs); > } > > -#ifndef CONFIG_ARM > +#ifndef CONFIG_ARM_32 I would prefer if you use CONFIG_ALTERNATIVE rather than CONFIG_ARM_32. > sec = livepatch_elf_sec_by_name(elf, ".altinstructions"); > if ( sec ) > { > @@ -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 > } > +#endif > > +#ifndef CONFIG_ARM > sec = livepatch_elf_sec_by_name(elf, ".ex_table"); > if ( sec ) > { Any reason to not move .ex_table in an x86 specific file? Or maybe we should define a new config option HAVE_ARCH_EX_TABLE to avoid architecture specific check in the common code. > @@ -1083,12 +1089,17 @@ 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); > > - 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 > * code - which expects the spinlocks to run with IRQs enabled - we temporarly > @@ -1119,10 +1130,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/asm-arm/config.h b/xen/include/asm-arm/config.h > index a96f845..8d876f6 100644 > --- a/xen/include/asm-arm/config.h > +++ b/xen/include/asm-arm/config.h > @@ -80,9 +80,10 @@ > * 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 - 8M <COMMON> > + * 0 - 10M <COMMON> May I ask to have this change and ... > * > * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM > * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address > @@ -93,7 +94,7 @@ > * > * ARM64 layout: > * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0]) > - * 0 - 8M <COMMON> > + * 0 - 10M <COMMON> this change in a separate patch to keep this patch livepatch only? > * > * 1G - 2G VMAP: ioremap and early_ioremap > * > @@ -113,6 +114,9 @@ > #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 _AT(vaddr_t,0x00800000) > +#endif > > #define HYPERVISOR_VIRT_START XEN_VIRT_START > > diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h > index 65c0cdf..f4fcfd6 100644 > --- a/xen/include/asm-arm/current.h > +++ b/xen/include/asm-arm/current.h > @@ -33,8 +33,15 @@ static inline struct cpu_info *get_cpu_info(void) > > #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs) > > +#ifdef CONFIG_LIVEPATCH > +#define switch_stack_and_jump(stack, fn) \ > + asm volatile ("mov sp,%0;" \ > + "bl check_for_livepatch_work;" \ > + "b " STR(fn) : : "r" (stack) : "memory" ) > +#else > #define switch_stack_and_jump(stack, fn) \ > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) > +#endif I have commented on the previous version about switch_stack_and_jump a while after you send this series. So I will spare you new comments here :). > #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn) > [...] > 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); > [1] https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01832.html
> > +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_size ) > > + insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr, > > + (unsigned long)func->new_addr, > > + AARCH64_INSN_BRANCH_NOLINK); > > The function aarch64_insn_gen_branch_imm will fail to create the branch if > the difference between old_addr and new_addr is greater than 128MB. > > In this case, the function will return AARCH64_BREAK_FAULT which will crash > Xen when the instruction is executed. > > I cannot find any sanity check in the hypervisor code. So are we trusting > the payload? Ugh. This is a thorny one. I concur we need to check this - and better of do it when we load the payload to make sure the distance is correct. And that should also be on x86, so will spin of a seperate patch. And in here add an ASSERT that the insn != AARCH64_BREAK_FAULT .. snip.. > > -/* On ARM32,64 instructions are always 4 bytes long. */ > > -#define PATCH_INSN_SIZE 4 > > Rather than moving again PATCH_INSN_SIZE in this patch. Can you directly > move it in patch [1]? Sure. > > > +void *vmap_of_xen_text; > > > > int arch_verify_insn_length(unsigned long len) > > { > > return len != PATCH_INSN_SIZE; > > } > > > > -void arch_livepatch_quiesce(void) > > +int arch_livepatch_quiesce(void) > > It would have been nice to move the prototype change out of this patch to > keep it "straight forward". OK. > > > { > > + mfn_t text_mfn; > > + unsigned int text_order; > > + > > + if ( vmap_of_xen_text ) > > + return -EINVAL; > > + > > + text_mfn = _mfn(virt_to_mfn(_stext)); > > + text_order = get_order_from_bytes(_end - _start); > > It is a bit odd that you use _stext before and the _start. But I won't > complain as I did the same in alternatives.c :/. Heheh. > > However, I think it should be enough to remap _stext -> _etext. I had to map > all the Xen binary for the alternatives because we may patch the init text. I agree. > > I forgot to mention it in the code, so I will send a patch to update it. OK. > > > + > > + /* > > + * 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. It has been cleaned before in > > I guess you want to replace "It" by "Data cache" otherwise it does not make > much sense. > > > + * arch_livepatch_apply_jmp. > > What about the payload? It may contain instructions, so we need to ensure > that all the data reached the memory. > > > + */ > > + invalidate_icache(); > > + > > + if ( vmap_of_xen_text ) > > + vunmap(vmap_of_xen_text); > > + > > + vmap_of_xen_text = NULL; > > + > > + /* > > + * Need to flush the branch predicator for ARMv7 as it may be > > s/predicator/predictor/ > > > + * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b). > > + */ > > + flush_xen_text_tlb_local(); > > I am a bit confused. In your comment you mention the branch but flush the > TLBs. The two are not related. > > However, I would prefer the branch predictor to be flushed directly in > invalidate_icache by calling BPIALLIS. This is because flushing the cache > means that you likely want to flush the branch predictor too. OK. > > > } > > > > int arch_livepatch_verify_func(const struct livepatch_func *func) > > { > > - return -ENOSYS; > > -} > > - > > -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, > > @@ -60,20 +97,34 @@ 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; > > + > > + ASSERT(va); > > + ASSERT(pages); > > + > > + if ( type == LIVEPATCH_VA_RX ) > > + flags = PTE_RO; /* R set, NX clear */ > > + else if ( type == LIVEPATCH_VA_RW ) > > + flags = PTE_NX; /* R clear, NX set */ > > + else > > + flags = PTE_NX | PTE_RO; /* R set, NX set */ > > va_type is an enum. So I would prefer to see a switch. So we can catch more > easily any addition of a new member. > > > + > > + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags); > > + > > + return 0; > > } > > > > void __init arch_livepatch_init(void) > > { > > + void *start, *end; > > + > > + start = (void *)LIVEPATCH_VMAP; > > + end = start + MB(2); > > Could you define the in config.h? So in the future we can rework more easily > the memory layout. LIVEPATCH_VMAP_START and LIVEPATCH_VMAP_END ? > > > + > > + vm_init_type(VMAP_XEN, start, end); > > } > > > > /* > > diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h > > new file mode 100644 > > index 0000000..8c8d625 > > --- /dev/null > > +++ b/xen/arch/arm/livepatch.h > > I am not sure why this header is living in arch/arm/ and not > include/asm-arm/ > > > @@ -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/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c > > index 06c67bc..e3f3f37 100644 > > --- a/xen/arch/x86/livepatch.c > > +++ b/xen/arch/x86/livepatch.c > > @@ -15,10 +15,12 @@ > > > > #define PATCH_INSN_SIZE 5 > > > > -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/Kconfig b/xen/common/Kconfig > > index 51afa24..2fc76b6 100644 > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -222,7 +222,7 @@ endmenu > > config LIVEPATCH > > bool "Live patching support (TECH PREVIEW)" > > default n > > - depends on X86 && HAS_BUILD_ID = "y" > > + depends on (X86 || ARM_64) && 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/common/livepatch.c b/xen/common/livepatch.c > > index 9c45270..af9443d 100644 > > --- a/xen/common/livepatch.c > > +++ b/xen/common/livepatch.c > > @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload, > > sizeof(*region->frame[i].bugs); > > } > > > > -#ifndef CONFIG_ARM > > +#ifndef CONFIG_ARM_32 > > I would prefer if you use CONFIG_ALTERNATIVE rather than CONFIG_ARM_32. That is not exposed on x86 thought. I can expose HAVE_ALTERNATIVE on x86 and use that instead. > > > sec = livepatch_elf_sec_by_name(elf, ".altinstructions"); > > if ( sec ) > > { > > @@ -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 > > } > > +#endif > > > > +#ifndef CONFIG_ARM > > sec = livepatch_elf_sec_by_name(elf, ".ex_table"); > > if ( sec ) > > { > > Any reason to not move .ex_table in an x86 specific file? Or maybe we should Would need to create an arch specific hook for this. > define a new config option HAVE_ARCH_EX_TABLE to avoid architecture specific > check in the common code. That could do it too. > > > @@ -1083,12 +1089,17 @@ 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); > > > > - 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 > > * code - which expects the spinlocks to run with IRQs enabled - we temporarly > > @@ -1119,10 +1130,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/asm-arm/config.h b/xen/include/asm-arm/config.h > > index a96f845..8d876f6 100644 > > --- a/xen/include/asm-arm/config.h > > +++ b/xen/include/asm-arm/config.h > > @@ -80,9 +80,10 @@ > > * 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 - 8M <COMMON> > > + * 0 - 10M <COMMON> > > May I ask to have this change and ... > > > * > > * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM > > * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address > > @@ -93,7 +94,7 @@ > > * > > * ARM64 layout: > > * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0]) > > - * 0 - 8M <COMMON> > > + * 0 - 10M <COMMON> > > this change in a separate patch to keep this patch livepatch only? Of course. > > > * > > * 1G - 2G VMAP: ioremap and early_ioremap > > * > > @@ -113,6 +114,9 @@ > > #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 _AT(vaddr_t,0x00800000) > > +#endif > > > > #define HYPERVISOR_VIRT_START XEN_VIRT_START > > > > diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h > > index 65c0cdf..f4fcfd6 100644 > > --- a/xen/include/asm-arm/current.h > > +++ b/xen/include/asm-arm/current.h > > @@ -33,8 +33,15 @@ static inline struct cpu_info *get_cpu_info(void) > > > > #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs) > > > > +#ifdef CONFIG_LIVEPATCH > > +#define switch_stack_and_jump(stack, fn) \ > > + asm volatile ("mov sp,%0;" \ > > + "bl check_for_livepatch_work;" \ > > + "b " STR(fn) : : "r" (stack) : "memory" ) > > +#else > > #define switch_stack_and_jump(stack, fn) \ > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) > > +#endif > > I have commented on the previous version about switch_stack_and_jump a while > after you send this series. So I will spare you new comments here :). :-) > > > #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn) > > > > [...] > > > 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); > > > > [1] > https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01832.html > > -- > Julien Grall
On 17/08/2016 02:50, Konrad Rzeszutek Wilk wrote: >>> +int arch_livepatch_perform_rela(struct livepatch_elf *elf, >>> + const struct livepatch_elf_sec *base, >>> + const struct livepatch_elf_sec *rela) >>> +{ > .. snip.. >>> + 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; >> >> As you borrow the code from Linux, could we keep the abstraction with >> reloc_data and defer the overflow check? It would avoid to have the same if >> in multiple place in this code. > > The above 'if' conditional is a check to make sure that we don't > go past the section (sh_size). In other words it is a boundary check to > make sure the Elf file is not messed up. Oh, Linux does not do those check. Sorry I though it was done. So I am fine with that. Cheers,
Hi Konrad, On 17/08/2016 19:57, 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); >>> + >>> + if ( func->new_size ) >>> + insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr, >>> + (unsigned long)func->new_addr, >>> + AARCH64_INSN_BRANCH_NOLINK); >> >> The function aarch64_insn_gen_branch_imm will fail to create the branch if >> the difference between old_addr and new_addr is greater than 128MB. >> >> In this case, the function will return AARCH64_BREAK_FAULT which will crash >> Xen when the instruction is executed. >> >> I cannot find any sanity check in the hypervisor code. So are we trusting >> the payload? > > Ugh. This is a thorny one. I concur we need to check this - and better of > do it when we load the payload to make sure the distance is correct. > > And that should also be on x86, so will spin of a seperate patch. > > And in here add an ASSERT that the insn != AARCH64_BREAK_FAULT It sounds good to me. [...] >>> + >>> + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags); >>> + >>> + return 0; >>> } >>> >>> void __init arch_livepatch_init(void) >>> { >>> + void *start, *end; >>> + >>> + start = (void *)LIVEPATCH_VMAP; >>> + end = start + MB(2); >> >> Could you define the in config.h? So in the future we can rework more easily >> the memory layout. > > LIVEPATCH_VMAP_START and LIVEPATCH_VMAP_END ? Something like that. > >> >>> + >>> + vm_init_type(VMAP_XEN, start, end); >>> } >>> >>> /* >>> diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h >>> new file mode 100644 >>> index 0000000..8c8d625 >>> --- /dev/null >>> +++ b/xen/arch/arm/livepatch.h >> >> I am not sure why this header is living in arch/arm/ and not >> include/asm-arm/ >> >>> @@ -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/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c >>> index 06c67bc..e3f3f37 100644 >>> --- a/xen/arch/x86/livepatch.c >>> +++ b/xen/arch/x86/livepatch.c >>> @@ -15,10 +15,12 @@ >>> >>> #define PATCH_INSN_SIZE 5 >>> >>> -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/Kconfig b/xen/common/Kconfig >>> index 51afa24..2fc76b6 100644 >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -222,7 +222,7 @@ endmenu >>> config LIVEPATCH >>> bool "Live patching support (TECH PREVIEW)" >>> default n >>> - depends on X86 && HAS_BUILD_ID = "y" >>> + depends on (X86 || ARM_64) && 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/common/livepatch.c b/xen/common/livepatch.c >>> index 9c45270..af9443d 100644 >>> --- a/xen/common/livepatch.c >>> +++ b/xen/common/livepatch.c >>> @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload, >>> sizeof(*region->frame[i].bugs); >>> } >>> >>> -#ifndef CONFIG_ARM >>> +#ifndef CONFIG_ARM_32 >> >> I would prefer if you use CONFIG_ALTERNATIVE rather than CONFIG_ARM_32. > > That is not exposed on x86 thought. > > I can expose HAVE_ALTERNATIVE on x86 and use that instead. True. I would like to avoid using CONFIG_ARM_* in the common code as it is a call to forget to remove the #ifdef when ARM32 will gain alternative patching. You are the maintainer of this code, so it is your call :). Regards,
> > > -/* On ARM32,64 instructions are always 4 bytes long. */ > > > -#define PATCH_INSN_SIZE 4 > > > > Rather than moving again PATCH_INSN_SIZE in this patch. Can you directly > > move it in patch [1]? > > Sure. The patch [1] changed where it does not have <len> anymore. And because of that (and some other changes) this patch is the one that introduces the #define. So I think having it in include/asm-arm/livepatch.h would work. When I send out the patchset that hopefully should be more clarified. .. > > [1] > > https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01832.html > > > > -- > > Julien Grall
>> + flush_xen_text_tlb_local(); > > > I am a bit confused. In your comment you mention the branch but flush the > TLBs. The two are not related. That code also flushes the I-cache (which sounds redundant with invalidate_icache). And it also does 'isb' ("Ensure synchronization with previous changes to text ") which invalidate_icache() does not. I am wondering if should just move 'isb' in invalidate_icache() and drop altogether the call to flush_xen_text_tlb_local()? It works fine in simulator > > However, I would prefer the branch predictor to be flushed directly in > invalidate_icache by calling BPIALLIS. This is because flushing the cache > means that you likely want to flush the branch predictor too. I had no trouble adding invalidate_icache to asm-arm/arm32/page.h and having it do : asm volatile ( CMD_CP32(ICIALLUIS) /* Flush I-cache. */ CMD_CP32(BPIALLIS) /* Flush branch predictor. */ : : : "memory"); But obviously that does not help ARM64. And that is is where I hit an snag. "bp iallis" instruction does not compile? (I also tried "bp iall", "bpiallis"). The error is: {standard input}: Assembler messages: {standard input}:454: Error: unknown mnemonic `bpiallis' -- `bpiallis' Ideas?
Hi Konrad, On 23/08/2016 22:14, Konrad Rzeszutek Wilk wrote: >>> + flush_xen_text_tlb_local(); >> >> >> I am a bit confused. In your comment you mention the branch but flush the >> TLBs. The two are not related. > > That code also flushes the I-cache (which sounds redundant with > invalidate_icache). > And it also does 'isb' ("Ensure synchronization with previous changes > to text ") which > invalidate_icache() does not. > > I am wondering if should just move 'isb' in invalidate_icache() and > drop altogether the call to flush_xen_text_tlb_local()? flush_xen_text_tlb_local will flush the TLB for the local processor, I am not sure why we would want to do that. It would be better to have a invalidate_icache helpers on ARM32. > > It works fine in simulator > >> >> However, I would prefer the branch predictor to be flushed directly in >> invalidate_icache by calling BPIALLIS. This is because flushing the cache >> means that you likely want to flush the branch predictor too. > > I had no trouble adding invalidate_icache to asm-arm/arm32/page.h and > having it do : > > asm volatile ( > CMD_CP32(ICIALLUIS) /* Flush I-cache. */ > CMD_CP32(BPIALLIS) /* Flush branch predictor. */ > : : : "memory"); > > But obviously that does not help ARM64. > > And that is is where I hit an snag. "bp iallis" instruction does not > compile? (I also tried "bp iall", "bpiallis"). The error is: > > {standard input}: Assembler messages: > {standard input}:454: Error: unknown mnemonic `bpiallis' -- `bpiallis' The instruction bpiallis does not exist on ARM64 because the branch predictor does not require explicit flush. Regards,
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..5966de0 100644 --- a/xen/arch/arm/arm32/Makefile +++ b/xen/arch/arm/arm32/Makefile @@ -4,8 +4,8 @@ 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 obj-y += vfp.o - diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c new file mode 100644 index 0000000..89ee2f5 --- /dev/null +++ b/xen/arch/arm/arm32/livepatch.c @@ -0,0 +1,55 @@ +/* + * 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) +{ + 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; +} + +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..e348942 --- /dev/null +++ b/xen/arch/arm/arm64/livepatch.c @@ -0,0 +1,247 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + */ + +#include "../livepatch.h" +#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> + +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_size ) + 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(); + + 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; +} + +static int reloc_insn_imm(void *dest, u64 val, int lsb, int len, + enum aarch64_insn_imm_type imm_type) +{ + u64 imm, imm_mask; + s64 sval = val; + u32 insn = *(u32 *)dest; + + /* Calculate the relocation value. */ + 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; + + for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ ) + { + int err = 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 */ + + /* 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; + *(int64_t *)dest = val; + break; + + case R_AARCH64_ABS32: + *(int32_t *)dest = val; + if ( (int64_t)val != *(int32_t *)dest ) + err = -EOVERFLOW; + break; + + case R_AARCH64_PREL64: + if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size ) + goto bad_offset; + + val -= (uint64_t)dest; + *(int64_t *)dest = val; + break; + + case R_AARCH64_PREL32: + val -= (uint64_t)dest; + *(int32_t *)dest = val; + if ( (int64_t)val != *(int32_t *)dest ) + err = -EOVERFLOW; + break; + + /* Instructions. */ + case R_AARCH64_ADR_PREL_LO21: + val -= (uint64_t)dest; + err = reloc_insn_imm(dest, val, 0, 21, AARCH64_INSN_IMM_ADR); + break; + + case R_AARCH64_ADR_PREL_PG_HI21: + val = (val & ~0xfff) - ((u64)dest & ~0xfff); + err = reloc_insn_imm(dest, val, 12, 21, AARCH64_INSN_IMM_ADR); + break; + + case R_AARCH64_LDST8_ABS_LO12_NC: + case R_AARCH64_ADD_ABS_LO12_NC: + err = reloc_insn_imm(dest, val, 0, 12, AARCH64_INSN_IMM_12); + if ( err == -EOVERFLOW ) + err = 0; + break; + + case R_AARCH64_LDST16_ABS_LO12_NC: + err = reloc_insn_imm(dest, val, 1, 11, AARCH64_INSN_IMM_12); + if ( err == -EOVERFLOW ) + err = 0; + break; + + case R_AARCH64_LDST32_ABS_LO12_NC: + err = reloc_insn_imm(dest, val, 2, 10, AARCH64_INSN_IMM_12); + if ( err == -EOVERFLOW ) + err = 0; + break; + + case R_AARCH64_LDST64_ABS_LO12_NC: + err = reloc_insn_imm(dest, val, 3, 9, AARCH64_INSN_IMM_12); + if ( err == -EOVERFLOW ) + err = 0; + break; + + case R_AARCH64_CONDBR19: + err = reloc_insn_imm(dest, val, 2, 19, AARCH64_INSN_IMM_19); + break; + + case R_AARCH64_JUMP26: + case R_AARCH64_CALL26: + val -= (uint64_t)dest; + err = reloc_insn_imm(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 ( err ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n", + elf->name, i, rela->name, base->name); + return err; + } + } + 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 3093554..19f6033 100644 --- a/xen/arch/arm/livepatch.c +++ b/xen/arch/arm/livepatch.c @@ -1,56 +1,93 @@ /* * Copyright (C) 2016 Citrix Systems R&D Ltd. */ + +#include "livepatch.h" #include <xen/errno.h> #include <xen/init.h> #include <xen/lib.h> #include <xen/livepatch_elf.h> #include <xen/livepatch.h> +#include <xen/vmap.h> + +#include <asm/mm.h> -/* On ARM32,64 instructions are always 4 bytes long. */ -#define PATCH_INSN_SIZE 4 +void *vmap_of_xen_text; int arch_verify_insn_length(unsigned long len) { return len != PATCH_INSN_SIZE; } -void arch_livepatch_quiesce(void) +int arch_livepatch_quiesce(void) { + mfn_t text_mfn; + unsigned int text_order; + + if ( vmap_of_xen_text ) + return -EINVAL; + + text_mfn = _mfn(virt_to_mfn(_stext)); + 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. It 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; + + /* + * Need to flush the branch predicator for ARMv7 as it may be + * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b). + */ + flush_xen_text_tlb_local(); } int arch_livepatch_verify_func(const struct livepatch_func *func) { - return -ENOSYS; -} - -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, @@ -60,20 +97,34 @@ 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; + + ASSERT(va); + ASSERT(pages); + + if ( type == LIVEPATCH_VA_RX ) + flags = PTE_RO; /* R set, NX clear */ + else if ( type == LIVEPATCH_VA_RW ) + flags = PTE_NX; /* R clear, NX set */ + else + flags = PTE_NX | PTE_RO; /* R set, NX set */ + + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags); + + return 0; } void __init arch_livepatch_init(void) { + void *start, *end; + + start = (void *)LIVEPATCH_VMAP; + end = start + MB(2); + + vm_init_type(VMAP_XEN, start, end); } /* diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h new file mode 100644 index 0000000..8c8d625 --- /dev/null +++ b/xen/arch/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/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 06c67bc..e3f3f37 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -15,10 +15,12 @@ #define PATCH_INSN_SIZE 5 -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/Kconfig b/xen/common/Kconfig index 51afa24..2fc76b6 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -222,7 +222,7 @@ endmenu config LIVEPATCH bool "Live patching support (TECH PREVIEW)" default n - depends on X86 && HAS_BUILD_ID = "y" + depends on (X86 || ARM_64) && 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/common/livepatch.c b/xen/common/livepatch.c index 9c45270..af9443d 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload, sizeof(*region->frame[i].bugs); } -#ifndef CONFIG_ARM +#ifndef CONFIG_ARM_32 sec = livepatch_elf_sec_by_name(elf, ".altinstructions"); if ( sec ) { @@ -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 } +#endif +#ifndef CONFIG_ARM sec = livepatch_elf_sec_by_name(elf, ".ex_table"); if ( sec ) { @@ -1083,12 +1089,17 @@ 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); - 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 * code - which expects the spinlocks to run with IRQs enabled - we temporarly @@ -1119,10 +1130,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/asm-arm/config.h b/xen/include/asm-arm/config.h index a96f845..8d876f6 100644 --- a/xen/include/asm-arm/config.h +++ b/xen/include/asm-arm/config.h @@ -80,9 +80,10 @@ * 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 - 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 +94,7 @@ * * ARM64 layout: * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0]) - * 0 - 8M <COMMON> + * 0 - 10M <COMMON> * * 1G - 2G VMAP: ioremap and early_ioremap * @@ -113,6 +114,9 @@ #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 _AT(vaddr_t,0x00800000) +#endif #define HYPERVISOR_VIRT_START XEN_VIRT_START diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h index 65c0cdf..f4fcfd6 100644 --- a/xen/include/asm-arm/current.h +++ b/xen/include/asm-arm/current.h @@ -33,8 +33,15 @@ static inline struct cpu_info *get_cpu_info(void) #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs) +#ifdef CONFIG_LIVEPATCH +#define switch_stack_and_jump(stack, fn) \ + asm volatile ("mov sp,%0;" \ + "bl check_for_livepatch_work;" \ + "b " STR(fn) : : "r" (stack) : "memory" ) +#else #define switch_stack_and_jump(stack, fn) \ asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) +#endif #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn) diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h index 5f2082e..a68874b 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 */ @@ -171,6 +180,7 @@ typedef struct { #define EM_PPC 20 /* PowerPC */ #define EM_PPC64 21 /* PowerPC 64-bit */ #define EM_ARM 40 /* Advanced RISC Machines ARM */ +#define EM_AARCH64 183 /* ARM 64-bit */ #define EM_ALPHA 41 /* DEC ALPHA */ #define EM_SPARCV9 43 /* SPARC version 9 */ #define EM_ALPHA_EXP 0x9026 /* DEC ALPHA */ @@ -353,12 +363,40 @@ 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_ADD_ABS_LO12_NC 277 /* ADD imm. [11:0]. S+A, NC */ + +#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 */ + /* Program Header */ typedef struct { Elf32_Word p_type; /* segment type */ 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);
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 is changed to return an error value which will be bubbled in payload->rc and provided to the user (along with messages in the ring buffer). And 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") 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. --- MAINTAINERS | 1 + xen/arch/arm/Makefile | 13 ++- xen/arch/arm/arm32/Makefile | 2 +- xen/arch/arm/arm32/livepatch.c | 55 +++++++++ xen/arch/arm/arm64/Makefile | 1 + xen/arch/arm/arm64/livepatch.c | 247 +++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/domain.c | 6 + xen/arch/arm/livepatch.c | 99 +++++++++++++---- xen/arch/arm/livepatch.h | 28 +++++ xen/arch/x86/livepatch.c | 4 +- xen/common/Kconfig | 2 +- xen/common/livepatch.c | 25 ++++- xen/include/asm-arm/config.h | 8 +- xen/include/asm-arm/current.h | 7 ++ xen/include/xen/elfstructs.h | 40 ++++++- xen/include/xen/livepatch.h | 2 +- 16 files changed, 503 insertions(+), 37 deletions(-) create mode 100644 xen/arch/arm/arm32/livepatch.c create mode 100644 xen/arch/arm/arm64/livepatch.c create mode 100644 xen/arch/arm/livepatch.h