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

Commit Message

Konrad Rzeszutek Wilk Sept. 23, 2016, 1:35 a.m. UTC

.snip..
> > 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.
> 
> [...]

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

Done!

From 8ade51e1866b86c7660277bb19100db337d432b4 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <[e-mail]>
Date: Sat, 10 Sep 2016 20:41:24 -0400
Subject: [PATCH v6] livepatch: arm[32,64],x86: NOP test-case

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}       <- NOP
  e28db000        add     fp, sp, #0 <- NOP
  e3a00008        mov     r0, #8     <- NOP
  e24bd000        sub     sp, fp, #0 <- NOP
  e49db004        pop     {fp}       <- NOP
  e12fff1e        bx      lr

And we can NOP instructions 1 through 5.

Granted this code may be different per compiler!

Hence if anybody does run this test-case - they should
verify that the assumptions made here are correct.

Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
---
Cc: Julien Grall <[e-mail]>
Cc: Stefano Stabellini <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>

v3: New submission.
v4: Redo the ARM code to NOP over push/pop as well.
---
 xen/test/livepatch/Makefile  | 15 +++++++++++++-
 xen/test/livepatch/xen_nop.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 1 deletion(-)
 create mode 100644 xen/test/livepatch/xen_nop.c

Comments

Julien Grall Sept. 23, 2016, 9:53 a.m. UTC | #1

Hi Konrad,

On 23/09/16 02:35, Konrad Rzeszutek Wilk wrote:
> diff --git a/xen/test/livepatch/xen_nop.c b/xen/test/livepatch/xen_nop.c
> new file mode 100644
> index 0000000..69dcbca
> --- /dev/null
> +++ b/xen/test/livepatch/xen_nop.c
> @@ -0,0 +1,48 @@
> +/*
> + * 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,

I would prefer if you use .new_size = MINOR_VERSION_SZ - 4 here.

> +#endif
> +
> +#ifdef CONFIG_ARM_32
> +    .old_addr = (void *)MINOR_VERSION_ADDR,
> +    /* And replace all but the return instruction. */
> +    .new_size = MINOR_VERSION_SZ-4,

It means that you could merge the ARM64 and ARM32 case.

With that:

Acked-by: Julien Grall <[e-mail]>

Regards,

Patch

diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 9439f62..76a779a 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -18,6 +18,7 @@  CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}')
 LIVEPATCH := xen_hello_world.livepatch
 LIVEPATCH_BYE := xen_bye_world.livepatch
 LIVEPATCH_REPLACE := xen_replace_world.livepatch
+LIVEPATCH_NOP := xen_nop.livepatch
 
 default: livepatch
 
@@ -25,10 +26,12 @@  install: livepatch
 	$(INSTALL_DATA) $(LIVEPATCH) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH)
 	$(INSTALL_DATA) $(LIVEPATCH_BYE) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_BYE)
 	$(INSTALL_DATA) $(LIVEPATCH_REPLACE) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_REPLACE)
+	$(INSTALL_DATA) $(LIVEPATCH_NOP) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_NOP)
 uninstall:
 	rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH)
 	rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_BYE)
 	rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_REPLACE)
+	rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_NOP)
 
 .PHONY: clean
 clean::
@@ -41,9 +44,13 @@  clean::
 .PHONY: config.h
 config.h: OLD_CODE_SZ=$(call CODE_SZ,$(BASEDIR)/xen-syms,xen_extra_version)
 config.h: NEW_CODE_SZ=$(call CODE_SZ,$<,xen_hello_world)
+config.h: MINOR_VERSION_SZ=$(call CODE_SZ,$(BASEDIR)/xen-syms,xen_minor_version)
+config.h: MINOR_VERSION_ADDR=$(call CODE_ADDR,$(BASEDIR)/xen-syms,xen_minor_version)
 config.h: xen_hello_world_func.o
 	(set -e; \
 	 echo "#define NEW_CODE_SZ $(NEW_CODE_SZ)"; \
+	 echo "#define MINOR_VERSION_SZ $(MINOR_VERSION_SZ)"; \
+	 echo "#define MINOR_VERSION_ADDR $(MINOR_VERSION_ADDR)"; \
 	 echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@
 
 xen_hello_world.o: config.h
@@ -91,5 +98,11 @@  xen_replace_world.o: config.h
 $(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o note.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^
 
+xen_nop.o: config.h
+
+.PHONY: $(LIVEPATCH_NOP)
+$(LIVEPATCH_NOP): xen_nop.o note.o
+	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
+
 .PHONY: livepatch
-livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE)
+livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)
diff --git a/xen/test/livepatch/xen_nop.c b/xen/test/livepatch/xen_nop.c
new file mode 100644
index 0000000..69dcbca
--- /dev/null
+++ b/xen/test/livepatch/xen_nop.c
@@ -0,0 +1,48 @@ 
+/*
+ * 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
+    .old_addr = (void *)MINOR_VERSION_ADDR,
+    /* And replace all but the return instruction. */
+    .new_size = MINOR_VERSION_SZ-4,
+#endif
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */