[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2 13/20] livepatch: Initial ARM64 support.
Hi Konrad,
On 07/09/2016 01:31, 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);
I don't see any value to use a temporary variable (old_ptr) to hold
func->old_addr here.
+
+ if ( func->new_addr )
+ 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();
+
+ ASSERT(insn != AARCH64_BREAK_FAULT);
Could you document in the code what prevents aarch64_insn_gen_branch_imm to
not generate a break fault instruction?
In the excitment of getting ARM32 implementation working - forgot
to write the code that would have done the check for all the platforms
(32MB for ARM, 128MB for ARM64, and 2GB on x86).
It will be an followup patch, like so (compile tested):
From 508157d81eaacab7ff621a84d7d885fb1bf689cc Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Tue, 6 Sep 2016 20:14:18 -0400
Subject: [PATCH] livepatch: ARM/x86: Check range of old_addr between new_addr
If the distance is too great we are in trouble - as our relocation
distance can surely be clipped, or still have a valid width - but
cause an overflow of distance.
On various architectures the maximum displacement for a branch/jump
varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86 for 32-bit
relocations is +/- 2G.
It would be worth noting that the ARM64 and ARM32 displacement are only
valid for unconditional branch (conditional one supports a smaller
displacement). And livepatch is only using unconditional branch.
[Note: On x86 we could use the 64-bit jmpq instruction which
would provide much bigger displacement to do a jump, but we would
still have issues with the new function not being able to reach
any of the old functions (as all the relocations would assume 32-bit
displacement)].
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
---
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
v3: New submission.
---
xen/arch/arm/arm64/livepatch.c | 1 +
xen/common/livepatch.c | 4 ++++
xen/include/asm-arm/livepatch.h | 8 ++++++++
xen/include/asm-x86/livepatch.h | 23 +++++++++++++++++++++++
xen/include/xen/livepatch.h | 18 +++++++++++++++++-
5 files changed, 53 insertions(+), 1 deletion(-)
create mode 100644 xen/include/asm-x86/livepatch.h
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index e4d2926..27a68ab 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -35,6 +35,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
else
insn = aarch64_insn_gen_nop();
+ /* Verified in arch_livepatch_verify_distance. */
ASSERT(insn != AARCH64_BREAK_FAULT);
new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 86232b8..1ba6ca9 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -587,6 +587,10 @@ static int prepare_payload(struct payload *payload,
rc = resolve_old_address(f, elf);
if ( rc )
return rc;
+
+ rc = arch_livepatch_verify_distance(f);
+ if ( rc )
+ return rc;
}
sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h
index 8c8d625..b1806d0 100644
--- a/xen/include/asm-arm/livepatch.h
+++ b/xen/include/asm-arm/livepatch.h
@@ -6,6 +6,8 @@
#ifndef __XEN_ARM_LIVEPATCH_H__
#define __XEN_ARM_LIVEPATCH_H__
+#include <xen/sizes.h> /* For SZ_* macros. */
+
/* On ARM32,64 instructions are always 4 bytes long. */
#define PATCH_INSN_SIZE 4
@@ -15,6 +17,12 @@
*/
extern void *vmap_of_xen_text;
+#ifdef CONFIG_ARM_32
+#define LIVEPATCH_ARCH_RANGE SZ_32M
+#else
+#define LIVEPATCH_ARCH_RANGE SZ_128M
+#endif
Sorry for been picky, can you document where these values come from:
- ARM32: A4.3 IN ARM DDI 0406C.j and we are using only ARM instruction
in Xen.
- ARM64: C1.3.2 in ARM DDI 0487A.j
It might also be worth to mention that this is only valid for
unconditional branch.
The rest looks good to me.
+
#endif /* __XEN_ARM_LIVEPATCH_H__ */
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
