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

Re: [Xen-devel] [PATCH v5 16/16] livepatch: arm[32, 64], x86: NOP test-case



Hi Konrad,

On 21/09/16 18:32, Konrad Rzeszutek Wilk wrote:
The test-case is quite simple - we NOP the 'xen_minor_version'.
The amount of NOPs depends on the architecture.

On x86 the function is 11 bytes long:

   55                      push   %rbp      <- NOP
   48 89 e5                mov    %rsp,%rbp <- NOP
   b8 04 00 00 00          mov    $0x4,%eax <- NOP
   5d                      pop    %rbp      <- NOP
   c3                      retq

We can NOP everything but the last instruction (so 10 bytes).

On ARM64 its 8 bytes:

  52800100    mov w0, #0x8         <- NOP
  d65f03c0    ret

We can NOP the first instruction.

While on ARM32 there are 24 bytes:

  e52db004        push    {fp}
  e28db000        add     fp, sp, #0 <- NOP
  e3a00008        mov     r0, #8     <- NOP
  e24bd000        sub     sp, fp, #0 <- NOP
  e49db004        pop     {fp}
  e12fff1e        bx      lr

And we can NOP instruction 2,3, and 4.

Why don't you NOP push {fp} and pop {fp}? At first glance, I am a bit surprised that the compiler is generating them (maybe it is because you have debug enabled?) and would have expect to have:

mov r0, #8
bx lr

NOPing all the instructions but the last one would simplify at lot the test case below.

[...]

--- /dev/null
+++ b/xen/test/livepatch/xen_nop.c
@@ -0,0 +1,49 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/types.h>
+
+#include <public/sysctl.h>
+
+/*
+ * All of the .new_size and .old_addr are based on assumptions that the
+ * code for 'xen_minor_version' is compiled in specific way. Before
+ * running this test-case you MUST verify that the assumptions are
+ * correct (Hint: make debug and look in xen.s).
+ */
+struct livepatch_func __section(".livepatch.funcs") livepatch_nop = {
+    .version = LIVEPATCH_PAYLOAD_VERSION,
+    .old_size = MINOR_VERSION_SZ,
+
+#ifdef CONFIG_X86
+    .old_addr = (void *)MINOR_VERSION_ADDR,
+    /* Everything but the last instruction: "req". */
+    .new_size = MINOR_VERSION_SZ-1,
+#endif
+
+#ifdef CONFIG_ARM_64
+    .old_addr = (void *)MINOR_VERSION_ADDR,
+    /* Replace the first one: "mov w0, #0x8".  */
+    .new_size = 4,
+#endif
+
+#ifdef CONFIG_ARM_32
+    /* Skip the first instruction: "push {fp}". */
+    .old_addr = (void *)(MINOR_VERSION_ADDR + 4),
+    /* And replace the next three instructions. */
+    .new_size = 3 * 4,
+#endif
+};

With my suggestion above, the two ARM code could become:

#ifdef CONFIG_ARM
        .old_addr = (void *)MINOR_VERSION_ADDR,
        .new_size = MINOR_VERSION_SIZE - 4,
#endif

The "- 4" is to avoid replacing the return.

Regards,

--
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®.