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

[Julien Grall <julien.grall@xxxxxxx>]

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