[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.
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?
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?
[...]
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).
}
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?
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).
+
+ 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.
}
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.
+
+ 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?
+ 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?
+ "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
|
