[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v1 6/9] livepatch: Initial ARM64 support.



Hi Konrad,

On 15/08/2016 01:07, Konrad Rzeszutek Wilk wrote:

[...]

diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index b20db64..5966de0 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -4,8 +4,8 @@ obj-$(EARLY_PRINTK) += debug.o
 obj-y += domctl.o
 obj-y += domain.o
 obj-y += entry.o
+obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-y += proc-v7.o proc-caxx.o
 obj-y += smpboot.o
 obj-y += traps.o
 obj-y += vfp.o
-

Spurious change?

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
new file mode 100644
index 0000000..89ee2f5
--- /dev/null
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -0,0 +1,55 @@
+/*
+ *  Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ */
+
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/livepatch_elf.h>
+#include <xen/livepatch.h>
+
+void arch_livepatch_apply_jmp(struct livepatch_func *func)
+{
+}
+
+void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+{
+}
+
+int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
+{
+    const Elf_Ehdr *hdr = elf->hdr;
+
+    if ( hdr->e_machine != EM_ARM ||
+         hdr->e_ident[EI_CLASS] != ELFCLASS32 )
+    {
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF Machine type!\n",
+                elf->name);
+        return -EOPNOTSUPP;
+    }
+
+    if ( (hdr->e_flags & EF_ARM_EABI_MASK) != EF_ARM_EABI_VER5 )
+    {
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF EABI(%x)!\n",
+                elf->name, hdr->e_flags);
+        return -EOPNOTSUPP;
+    }
+
+    return 0;

I would prefer if you introduce ARM32 implementation in one go. I.e can you only return -EOPNOTSUPP here for now?


[...]

+int arch_livepatch_perform_rela(struct livepatch_elf *elf,
+                                const struct livepatch_elf_sec *base,
+                                const struct livepatch_elf_sec *rela)
+{
+    const Elf_RelA *r;
+    unsigned int symndx, i;
+    uint64_t val;
+    void *dest;
+
+    for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
+    {
+        int err = 0;
+
+        r = rela->data + i * rela->sec->sh_entsize;
+
+        symndx = ELF64_R_SYM(r->r_info);
+
+        if ( symndx > elf->nsym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u 
which is past end!\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+
+        dest = base->load_addr + r->r_offset; /* P */
+        val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
+
+        /* ARM64 operations at minimum are always 32-bit. */
+        if ( r->r_offset >= base->sec->sh_size ||
+            (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
+            goto bad_offset;
+
+        switch ( ELF64_R_TYPE(r->r_info) ) {
+        /* Data */
+        case R_AARCH64_ABS64:
+            if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
+                goto bad_offset;

As you borrow the code from Linux, could we keep the abstraction with reloc_data and defer the overflow check? It would avoid to have the same if in multiple place in this code.

+            *(int64_t *)dest = val;
+            break;
+
+        case R_AARCH64_ABS32:
+            *(int32_t *)dest = val;
+            if ( (int64_t)val !=  *(int32_t *)dest )
+                err = -EOVERFLOW;
+            break;
+
+        case R_AARCH64_PREL64:
+            if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
+                goto bad_offset;
+
+            val -= (uint64_t)dest;
+            *(int64_t *)dest = val;
+            break;
+
+        case R_AARCH64_PREL32:
+            val -= (uint64_t)dest;
+            *(int32_t *)dest = val;
+            if ( (int64_t)val !=  *(int32_t *)dest )
+                err = -EOVERFLOW;
+            break;
+
+        /* Instructions. */
+        case R_AARCH64_ADR_PREL_LO21:
+            val -= (uint64_t)dest;
+            err = reloc_insn_imm(dest, val, 0, 21, AARCH64_INSN_IMM_ADR);
+            break;
+

It seems the implementation of R_AARCH64_ADR_PREL_PG_HI21_NC is missing.

+        case R_AARCH64_ADR_PREL_PG_HI21:
+            val = (val & ~0xfff) - ((u64)dest & ~0xfff);
+            err = reloc_insn_imm(dest, val, 12, 21, AARCH64_INSN_IMM_ADR);
+            break;

Hmmm, I think we want to avoid the payload having R_AARCH64_ADR_PREL_PG_HI21* relocations due to the erratum #843419 on some Cortex-A53.

I haven't yet looked at how you build the payload, but this may also affects the process to do it. I will give a look on it.

+
+        case R_AARCH64_LDST8_ABS_LO12_NC:
+        case R_AARCH64_ADD_ABS_LO12_NC:
+            err = reloc_insn_imm(dest, val, 0, 12, AARCH64_INSN_IMM_12);
+            if ( err == -EOVERFLOW )
+                err = 0;
+            break;
+
+        case R_AARCH64_LDST16_ABS_LO12_NC:
+            err = reloc_insn_imm(dest, val, 1, 11, AARCH64_INSN_IMM_12);
+            if ( err == -EOVERFLOW )
+                err = 0;
+            break;
+
+        case R_AARCH64_LDST32_ABS_LO12_NC:
+            err = reloc_insn_imm(dest, val, 2, 10, AARCH64_INSN_IMM_12);
+            if ( err == -EOVERFLOW )
+                err = 0;
+            break;
+
+        case R_AARCH64_LDST64_ABS_LO12_NC:
+            err = reloc_insn_imm(dest, val, 3, 9, AARCH64_INSN_IMM_12);
+            if ( err == -EOVERFLOW )
+                err = 0;
+            break;
+
+        case R_AARCH64_CONDBR19:
+            err = reloc_insn_imm(dest, val, 2, 19, AARCH64_INSN_IMM_19);
+            break;
+
+        case R_AARCH64_JUMP26:
+        case R_AARCH64_CALL26:
+            val -= (uint64_t)dest;
+            err = reloc_insn_imm(dest, val, 2, 26, AARCH64_INSN_IMM_26);
+            break;
+
+        default:
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation %lu\n",
+                    elf->name, ELF64_R_TYPE(r->r_info));
+             return -EOPNOTSUPP;
+        }
+
+        if ( err )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for 
%s!\n",
+                    elf->name, i, rela->name, base->name);
+            return err;
+        }
+    }
+    return 0;
+
+ bad_offset:
+    dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation offset is past %s 
section!\n",
+            elf->name, base->name);
+    return -EINVAL;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.