|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [Very RFC PATCH 3/3] livepatch: Initial ARM32/64 support.
On Fri, Aug 12, 2016 at 05:00:47PM +0200, Julien Grall wrote:
> Hi Konrad,
>
> On 09/08/2016 06:19, Konrad Rzeszutek Wilk wrote:
> > The initial support for ARM64 - and livepatching
> > works:
>
> As it is a very RFC I only gave a quick look. I have few comments on it (see
> below).
>
> >
> > (XEN) livepatch: xen_hello_world: Applying 1 functions
> > (XEN) hi_func: Hi! (called 1 times)
> > (XEN) Hook executing.
> > (XEN) livepatch: xen_hello_world finished APPLY with rc=0
> > (XEN) livepatch.c:1687: livepatch: load_payload_fnc: rc=0 (p->rc=0)
> > (XEN) livepatch: Hello World
> > (XEN) 'x' pressed - Dumping all livepatch patches
> > (XEN) build-id: e144bafc4ee8635eee5bed8e3988b484765c46c8
> > (XEN) name=xen_hello_world state=APPLIED(2) 0000000000318000
> > (.data=0000000000319000, .rodata=000000000031a000) using 3 pages.
> > (XEN) xen_extra_version patch 0000000000233fac(12) with
> > 0000000000318000 (16)
> > (XEN) build-id=c4b842c276be43adbe4db788598b1e11bce04dc6
> > (XEN) depend-on=9affa110481e8e13606c61b21e5f6a357a3c8ef9
> >
> > ARM32 still has some issues.
> >
> > The are some TODOs left to be done:
> >
> > General:
> > - Bubble ALT_ORIG_PTR macro for both x86/ARM.
> > - Unify the ELF RELA checks - they are the same on x86/ARM[32,64].
> > - Makefile generating .livepatch.depends needs to ingest the
> > -O argument based on architecture
> > - Test cases should move to common/ ? [Needs Jan's Ack]
>
> In general, I would be in favor of sharing as much as possible the code.
>
> > ARM32 issues:
> > - vm_init_type: Assertion 'is_xen_heap_mfn(ma >> PAGE_SHIFT)' failed at
> > xen/include/asm/mm.h:23
>
> xen/include/asm/mm.h:23 points to the beginning of struct page_info. Did you
> mean to write instead 233?
Probably.
>
> This would point to maddr_virt. This would mean someone is trying to get the
> virtual address of MFN that is not in the xenheap (only the xenheap is
> mapped on ARM32). Do you have the full call stack?
No :-( I need to rebuild it and put it on the board. Maybe in a week?
>
> [...]
>
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>
> [...]
>
> > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> > index aba1320..67749ed 100644
> > --- a/xen/arch/arm/livepatch.c
> > +++ b/xen/arch/arm/livepatch.c
> > @@ -6,66 +6,100 @@
> > #include <xen/lib.h>
> > #include <xen/livepatch_elf.h>
> > #include <xen/livepatch.h>
> > +#include <xen/vmap.h>
> > +#include "livepatch.h"
> > +
> > +#include <asm/mm.h>
> > +
> > +void *vmap_of_xen_text;
> >
> > void arch_livepatch_quiesce(void)
> > {
> > + mfn_t text_mfn;
> > + unsigned int text_order;
> > +
> > + if ( vmap_of_xen_text )
> > + return;
> > +
> > + 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);
>
> Shouldn't we return an error if we fail to re-map the region?
>
> Otherwise the patch may silently be ignored (see arch_livepatch_apply_jmp).
We do actually bail out of patching if vmap_of_xen_text is NULL.
But let me add a return (-ENOMEM) to this so that we don't attempt to do any
patching and print a nice message.
>
> > }
> >
> > void arch_livepatch_revive(void)
> > {
> > + /* Nuke the instruction cache */
> > + invalidate_icache();
>
> I was about to say that this is not enough. But you called
> clean_and_invalidate_* every time you rewrite the jump. It might be worth to
> add a comment here, explain that the cache has been cleaned before hand.
>
> However, I can't find any data cache flush for the payload. Did I miss
> anything?
No just didn't occur to me - as we patch the .text not the .data.
I will take a look at the existing code and see if I can find out how
to do the data cache flush.
>
> Lastly, before I forgot, you will need to invalidate the branch predictor
> for ARMv7 as it may be architecturally visible to the software (see B2.2.4
> in ARM DDI 0406C.b).
OK.
>
> > +
> > + if ( vmap_of_xen_text )
> > + vunmap(vmap_of_xen_text);
> > +
> > + vmap_of_xen_text = NULL;
> > }
> >
> > int arch_livepatch_verify_func(const struct livepatch_func *func)
> > {
> > - return -ENOSYS;
> > -}
> > + /* No NOP patching yet. */
> > + if ( !func->new_size )
> > + return -EOPNOTSUPP;
> >
> > -void arch_livepatch_apply_jmp(struct livepatch_func *func)
> > -{
> > -}
> > + if ( func->old_size < PATCH_INSN_SIZE )
> > + return -EINVAL;
> >
> > -void arch_livepatch_revert_jmp(const struct livepatch_func *func)
> > -{
> > + return 0;
> > }
> >
> > void arch_livepatch_post_action(void)
> > {
> > + /* arch_livepatch_revive has nuked the instruction cache. */
> > }
> >
> > void arch_livepatch_mask(void)
> > {
> > + /* TODO: No NMI on ARM? */
>
> All interrupts can be masked on ARM so far. Although, local_irq_disable will
> only mask IRQ (i.e interrupt from the interrupt controller).
>
> We may want to mask SError (System Error) via local_abort_{disable,enable}
> to avoid receiving asynchronous abort whilst patching Xen. The interrupt
> will stay pending until it will be re-enabled in arch_livepatch_unmask.
/me nods.
Thanks!
>
> > }
> >
> > void arch_livepatch_unmask(void)
> > {
> > + /* TODO: No NMI on ARM? */
> > }
> >
> > -int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
> > +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;
> >
> > -int arch_livepatch_perform_rel(struct livepatch_elf *elf,
> > - const struct livepatch_elf_sec *base,
> > - const struct livepatch_elf_sec *rela)
> > -{
> > - return -ENOSYS;
> > -}
> > + ASSERT(va);
> > + ASSERT(pages);
> >
> > -int arch_livepatch_perform_rela(struct livepatch_elf *elf,
> > - const struct livepatch_elf_sec *base,
> > - const struct livepatch_elf_sec *rela)
> > -{
> > - return -ENOSYS;
> > -}
> > + if ( type == LIVEPATCH_VA_RX )
> > + flags = 0x2; /* R set,NX clear */
> > + else if ( type == LIVEPATCH_VA_RW )
> > + flags = 0x1; /* R clear, NX set */
> > + else
> > + flags = 0x3; /* R set,NX set */
> >
> > -int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type
> > type)
> > -{
> > - return -ENOSYS;
> > + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
> > +
> > + return 0;
> > }
> >
> > void __init arch_livepatch_init(void)
> > {
> > + void *start, *end;
> > +
> > + start = (void *)xen_virt_end;
> > + end = (void *)FIXMAP_ADDR(0);
>
> The space between xen_virt_end and FIXMAP_ADDR may be really small depending
> on the size of the Xen binary.
>
> I would introduce a specific region in the layout of few megabytes (not sure
> how much me need). Or re-use "early relocation address" (8M - 10M) as the
> region is only used during early boot.
OK, will take a look. Thanks!
>
> > +
> > + BUG_ON(start >= end);
> > +
> > + vm_init_type(VMAP_XEN, start, end);
> > }
> >
> > /*
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index eca7cdd..94b4911 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -143,6 +143,8 @@ vaddr_t xenheap_virt_end __read_mostly;
> > vaddr_t xenheap_virt_start __read_mostly;
> > #endif
> >
> > +vaddr_t xen_virt_end;
> > +
> > unsigned long frametable_base_pdx __read_mostly;
> > unsigned long frametable_virt_end __read_mostly;
> >
> > @@ -540,6 +542,9 @@ void __init setup_pagetables(unsigned long
> > boot_phys_offset, paddr_t xen_paddr)
> > /* No flush required here as page table is not hooked in yet. */
> > }
> >
> > + if ( i < LPAE_ENTRIES )
>
> Why this check?
/me scratches his head. That was for when I was also printing the entries.
Let me remove it.
>
> > + xen_virt_end = XEN_VIRT_START + (i << PAGE_SHIFT);
> > +
> > pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
> > pte.pt.table = 1;
> > write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte);
>
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index 88a79d8..6ffd2d0 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -618,7 +618,6 @@ static int prepare_payload(struct payload *payload,
> > sizeof(*region->frame[i].bugs);
> > }
> >
> > -#ifndef CONFIG_ARM
> > sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
> > if ( sec )
> > {
> > @@ -636,8 +635,14 @@ static int prepare_payload(struct payload *payload,
> >
> > for ( a = start; a < end; a++ )
> > {
> > +#ifndef CONFIG_ARM
> > + /* TODO: Bubble ALT_ORIG_PTR up. */
> > const void *instr = &a->instr_offset + a->instr_offset;
> > const void *replacement = &a->repl_offset + a->repl_offset;
> > +#else
> > + const void *instr = &a->orig_offset + a->orig_offset;
> > + const void *replacement = &a->alt_offset + a->alt_offset;
> > +#endif
> >
> > if ( (instr < region->start && instr >= region->end) ||
> > (replacement < region->start && replacement >=
> > region->end) )
> > @@ -647,9 +652,14 @@ 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
> > }
> >
> > +#ifndef CONFIG_ARM
> > sec = livepatch_elf_sec_by_name(elf, ".ex_table");
> > if ( sec )
> > {
> > 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;" \
>
> May I ask why check_for_livepatch_work is called in switch_stack_and_jump?
From 29f4ab0b0a4ff62589af35b7cbc2334e1d9acdcd:
To perform and action on a payload, the hypercall sets up a data
structure to schedule the work. A hook is added in the reset_stack_and_jump
to check for work and execute it if needed (specifically we check an
per-cpu flag to make this as quick as possible).
In this way, patches can be applied with all CPUs idle and without
stacks. The first CPU to run check_for_xsplice_work() becomes the
master and triggers a reschedule softirq to trigger all the other CPUs
to enter check_for_xsplice_work() with no stack. Once all CPUs
have rendezvoused, all CPUs disable their IRQs and NMIs are ignored.
The system is then quiscient and the master performs the action.
After this, all CPUs enable IRQs and NMIs are re-enabled.
>
> > + "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)
>
> Regards,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |