|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 6/9] livepatch: Initial ARM64 support.
> > +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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |