Re: [Xen-devel] [PATCH v5 05/16] livepatch: Initial ARM64 support.
Hi Konrad,
On 23/09/2016 08:44, Konrad Rzeszutek Wilk wrote:
Here is the updated patch:
From deef8f6921c15a4d07209bfba1fc8697dbfeb605 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk
Date: Mon, 19 Sep 2016 12:24:09 -0400
Subject: [PATCH v6] livepatch: Initial ARM64 support.
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 was changed (see "x86,arm:
Change arch_livepatch_quiesce() declaration") to return
an error value which will be bubbled in payload->rc and
provided to the user (along with messages in the ring buffer).
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")
And while at it - we also utilize code from Linux to construct
the right branch instruction (see "arm64/insn: introduce
aarch64_insn_gen_{nop|branch_imm}() helper functions").
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.
Acked-by: Jan Beulich [non-arm parts]
Signed-off-by: Konrad Rzeszutek Wilk
You can add my acked-by on this version:
Acked-by: Julien Grall
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 05/16] livepatch: Initial ARM64 support.
On Fri, Sep 23, 2016 at 02:38:57PM +0100, Ross Lagerwall wrote:
> On 09/21/2016 06:32 PM, Konrad Rzeszutek Wilk wrote:
> snip
> > +
> > +void arch_livepatch_revert(const struct livepatch_func *func)
> > +{
> > +uint32_t *new_ptr;
> > +unsigned int i, len;
> > +
> > +new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> > +len = livepatch_insn_len(func) / sizeof(uint32_t);
> > +for ( i = 0; i < len; i++ )
> > +{
> > +uint32_t insn;
> > +
> > +memcpy(&insn, func->opaque + (i * sizeof(uint32_t)),
> > ARCH_PATCH_INSN_SIZE);
> > +/* PATCH! */
> > +*(new_ptr + i) = insn;
> > +}
>
> Why is this done in a loop? Can't we just memcpy livepatch_insn_len(func)
> bytes into *new_ptr?
It can be.
>
> > +}
> > +
> > +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;
> > +}
> > +
> snip
> > int arch_livepatch_verify_func(const struct livepatch_func *func)
> > {
> > -return -ENOSYS;
> > -}
> > +/* If NOPing only do up to maximum amount we can put in the ->opaque.
> > */
> > +if ( !func->new_addr && func->new_size > sizeof(func->opaque) &&
> > + func->new_size % ARCH_PATCH_INSN_SIZE )
> > +return -EOPNOTSUPP;
>
> Maybe I'm misunderstanding, but shouldn't this be ( !func->new_addr &&
> (func->new_size > sizeof(func->opaque) || func->new_size %
> ARCH_PATCH_INSN_SIZE) ) ?
Yes!
Here is the updated patch:
From deef8f6921c15a4d07209bfba1fc8697dbfeb605 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk
Date: Mon, 19 Sep 2016 12:24:09 -0400
Subject: [PATCH v6] livepatch: Initial ARM64 support.
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 was changed (see "x86,arm:
Change arch_livepatch_quiesce() declaration") to return
an error value which will be bubbled in payload->rc and
provided to the user (along with messages in the ring buffer).
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")
And while at it - we also utilize code from Linux to construct
the right branch instruction (see "arm64/insn: introduce
aarch64_insn_gen_{nop|branch_imm}() helper functions").
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.
Acked-by: Jan Beulich [non-arm parts]
Signed-off-by: Konrad Rzeszutek Wilk
---
Cc: Ross Lagerwall
Cc: Stefano Stabellini
Cc: Julien Grall
Cc Jan Beulich
Cc: Andrew Cooper
RFC: Wholy cow! It works!
v1: Finished the various TODOs and reviews outlined by Julien in RFC.
v2: Call check_for_livepatch_work in leave_hypervisor_tail not in
reset_stack_and_jump
- Move ARM 32 components in other patch
- Blacklist platform options in Kconfig
- Added R_AARCH64_LDST128_ABS_LO12_NC, R_AARCH64_TSTBR14, and
R_AARCH64_ADR_PREL_PG_HI21_NC
- Added do_reloc and reloc_data along with overflow checks from Linux.
- Use apply_alternatives without any #ifdef
- Use leave_hypervisor_tail to call check_for_livepatch_work
- Add ASSERT that isns is not AARCH64_BREAK_FAULT
- Spun out arch_livepatch_quiesce changes in seperate patch.
- Spun out changes to config.h (document ones) to seperate patch.
- Move the livepatch.h to include/xen/asm-arm.
- Add LIVEPATCH_VMAP_END and LIVEPATCH_VMAP_START in config.h
- In arch_livepatch_secure use switch for va_type.
- Drop the #ifndef CONFIG_ARM for .ex_table (see
"arm/x86: Add HAS_ALTERNATIVE and HAS_EX_TABLE")
- Piggyback on "x86: change modify_xe
Re: [Xen-devel] [PATCH v5 05/16] livepatch: Initial ARM64 support.
On 09/21/2016 06:32 PM, Konrad Rzeszutek Wilk wrote:
snip
+
+void arch_livepatch_revert(const struct livepatch_func *func)
+{
+uint32_t *new_ptr;
+unsigned int i, len;
+
+new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+len = livepatch_insn_len(func) / sizeof(uint32_t);
+for ( i = 0; i < len; i++ )
+{
+uint32_t insn;
+
+memcpy(&insn, func->opaque + (i * sizeof(uint32_t)),
ARCH_PATCH_INSN_SIZE);
+/* PATCH! */
+*(new_ptr + i) = insn;
+}
Why is this done in a loop? Can't we just memcpy
livepatch_insn_len(func) bytes into *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;
+}
+
snip
int arch_livepatch_verify_func(const struct livepatch_func *func)
{
-return -ENOSYS;
-}
+/* If NOPing only do up to maximum amount we can put in the ->opaque. */
+if ( !func->new_addr && func->new_size > sizeof(func->opaque) &&
+ func->new_size % ARCH_PATCH_INSN_SIZE )
+return -EOPNOTSUPP;
Maybe I'm misunderstanding, but shouldn't this be ( !func->new_addr &&
(func->new_size > sizeof(func->opaque) || func->new_size %
ARCH_PATCH_INSN_SIZE) ) ?
--
Ross Lagerwall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 05/16] livepatch: Initial ARM64 support.
Hi Konrad,
On 21/09/16 18:32, Konrad Rzeszutek Wilk wrote:
[...]
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index f2ae52a..ff2cfb8 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -1108,7 +1108,7 @@ and no .data or .bss sections.
The hypervisor should verify that the in-place patching would fit within
the code or data.
-### Trampoline (e9 opcode)
+### Trampoline (e9 opcode), x86
The e9 opcode used for jmpq uses a 32-bit signed displacement. That means
we are limited to up to 2GB of virtual address to place the new code
@@ -1143,3 +1143,15 @@ that in the hypervisor is advised.
The tool for generating payloads currently does perform a compile-time
check to ensure that the function to be replaced is large enough.
+ Trampoline (ea opcode), ARM
Please drop "(ea opcode)" as it is only valid for ARM32.
+
+The unconditional branch instruction (for the encoding see the
+DDI 0406C.c and DDI 0487A.j Architecture Reference Manual's).
+with proper offset is used for an unconditional branch to the new code.
+This means that that `old_size` **MUST** be at least four bytes if patching
+in trampoline.
+
+The new code is placed in the 8M - 10M virtual address space while the
+Xen code is in 2M - 4M. That gives us enough space.
+
+The hypervisor also checks the displacement during loading of the payload.
[...]
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
new file mode 100644
index 000..7cb1812
--- /dev/null
+++ b/xen/arch/arm/arm64/livepatch.c
[...]
+void arch_livepatch_apply(struct livepatch_func *func)
+{
+uint32_t insn;
+uint32_t *new_ptr;
+unsigned int i, len;
+
+BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque));
+BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn));
+
+ASSERT(vmap_of_xen_text);
+
+len = livepatch_insn_len(func);
+if ( !len )
+return;
+
+/* Save old ones. */
+memcpy(func->opaque, func->old_addr, len);
+
+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);
+
+new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+len = len / sizeof(uint32_t);
+
+/* PATCH! */
+for ( i = 0; i < len; i++ )
+*(new_ptr + i) = insn;
+
+/*
+* When we upload the payload, it will go through the data cache
+* (the region is cacheable). Until the data cache is cleaned, the data
+* may not reach the memory. And in the case the data and instruction cache
+* are separated, we may read invalid instruction from the memory because
+* the data cache have not yet synced with the memory. Hence sync it.
+*/
+if ( func->new_addr )
+clean_and_invalidate_dcache_va_range(func->new_addr, func->new_size);
Why did you drop the clean_and_invalidate_dcache_va_range to the
instruction just patched?
I guess it is because my mail in the previous thread was confusing,
sorry for that.
"> Or did you mean the old_addr (the one we just patched?)
We don't care about the previous function. It will never changed except
for the instruction patched.
"
I meant that it is not necessary to flush the whole region of the old
code (Xen already flushed it at boot time), but only the region you
patched. The interesting bit in the ARM ARM is B2.3.4 DDI 0487A.j.
So the code should look like:
if ( func->new_addr )
clean_and_invalidate_dcache_va_range(func->new_addr, func->new_size);
clean_and_invalidate_dcache_va_range(new_ptr, sizeof (*new_ptr) * len);
+}
+
+void arch_livepatch_revert(const struct livepatch_func *func)
+{
+uint32_t *new_ptr;
+unsigned int i, len;
+
+new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+len = livepatch_insn_len(func) / sizeof(uint32_t);
+for ( i = 0; i < len; i++ )
+{
+uint32_t insn;
+
+memcpy(&insn, func->opaque + (i * sizeof(uint32_t)),
ARCH_PATCH_INSN_SIZE);
+/* PATCH! */
+*(new_ptr + i) = insn;
+}
Similarly:
clean_and_invalidate_dcache_va_range(new_ptr, sizeof (*new_ptr) * len);
+}
+
[...]
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 7f067a0..9284766 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
[...]
void arch_livepatch_revive(void)
{
+/*
+ * Nuke the instruction cache. Data cache has been cleaned before in
+ * arch_livepatch_apply.
NIT: Would it be worth to mention arch_livepatch_revert here?
+ */
+invalidate_icache();
+
+if ( vmap_of_xen_text )
+vunmap(vmap_of_xen_text);
+
+vmap_of_xen_text = NULL;
}
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-de
