Re: [Xen-devel] [PATCH v4 10/16] livepatch: x86, ARM, alternative: Expose FEATURE_LIVEPATCH

[Julien Grall <julien.grall@xxxxxxx>]

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:
> To use as a common way of testing alternative patching for
> livepatches. Both architectures have this FEATURE and the
> test-cases can piggyback on that.
>
> Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>
> ---
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> v3: New submission
> v4: Move the LIVEPATCH_FEATURE to asm-x86/livepatch.h
> ---
>  xen/arch/arm/livepatch.c                  | 3 +++
>  xen/include/asm-arm/alternative.h         | 2 ++
>  xen/include/asm-arm/cpufeature.h          | 5 +++++
>  xen/include/asm-x86/livepatch.h           | 1 +
>  xen/test/livepatch/xen_hello_world_func.c | 8 +++++---
>  5 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index 13d6c10..b771cb7 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -8,6 +8,7 @@
>  #include <xen/livepatch.h>
>  #include <xen/vmap.h>
>
> +#include <asm/cpufeature.h>
>  #include <asm/livepatch.h>
>  #include <asm/mm.h>
>
> @@ -175,6 +176,8 @@ void __init arch_livepatch_init(void)
>      end = (void *)LIVEPATCH_VMAP_END;
>
>      vm_init_type(VMAP_XEN, start, end);
> +
> +    cpus_set_cap(LIVEPATCH_FEATURE);
>  }
>
>  /*
> diff --git a/xen/include/asm-arm/alternative.h 
> b/xen/include/asm-arm/alternative.h
> index 6851217..cca5f17 100644
> --- a/xen/include/asm-arm/alternative.h
> +++ b/xen/include/asm-arm/alternative.h
> @@ -165,6 +165,8 @@ static inline int apply_alternatives(void *start, size_t 
> lenght)
>      return 0;
>  }
>
> +#define ALTERNATIVE(oldinstr, newinstr, ...) ""
> +
Why this change? Nobody should ever use ALTERNATIVE when 
CONFIG_ALTERNATIVE is not enabled. This is a call to have a Xen not 
patched for a buggy hardware.
Not to mention that a void ALTERNATIVE is really buggy. We use 
ALTERNATIVE to switch between two set of instructions. So we always need 
at least one integrated in Xen.
>  #endif
>
>  #endif /* __ASM_ALTERNATIVE_H */
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 66e930f..19e768b 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -40,7 +40,12 @@
>  #define ARM32_WORKAROUND_766422 2
>  #define ARM64_WORKAROUND_834220 3
>
> +#ifdef CONFIG_LIVEPATCH
> +#define LIVEPATCH_FEATURE   4
> +#define ARM_NCAPS           5
> +#else
>  #define ARM_NCAPS           4
> +#endif
I would rather define the feature livepatch unconditionally to avoid the 
definition of ARM_NCAPS twice.
> diff --git a/xen/test/livepatch/xen_hello_world_func.c 
> b/xen/test/livepatch/xen_hello_world_func.c
> index 6f53ab4..765a871 100644
> --- a/xen/test/livepatch/xen_hello_world_func.c
> +++ b/xen/test/livepatch/xen_hello_world_func.c
> @@ -20,7 +20,6 @@ const char *xen_hello_world(void)
>      unsigned long tmp;
>      int rc;
>
> -    alternative(ASM_NOP8, ASM_NOP1, X86_FEATURE_LM);
Any reason to move the code later on?

>      /*
>       * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore
>       * exceptions will be caught and processed properly.
> @@ -28,8 +27,11 @@ const char *xen_hello_world(void)
>      rc = __get_user(tmp, non_canonical_addr);
>      BUG_ON(rc != -EFAULT);
>  #endif
> -#ifdef CONFIG_ARM_64
> -    asm(ALTERNATIVE("nop", "nop", ARM64_WORKAROUND_CLEAN_CACHE));
> +#ifdef CONFIG_ARM
> +    asm(ALTERNATIVE("nop", "nop", LIVEPATCH_FEATURE));
Ah, this is why you need a nop ALTERNATIVE. My comment above still 
stands, this definition of ALTERNATIVE maybe be valid for your use case, 
but it is invalid for most of the others.
> +#endif
> +#ifdef CONFIG_X86
> +    asm(ALTERNATIVE(ASM_NOP8, ASM_NOP1, LIVEPATCH_FEATURE));
>  #endif
>      return "Hello World";
>  }
Regards,

--
Julien Grall

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