Re: [PATCH for-next 1/2] xen/x86/alternatives: Do not use sync_core() to serialize I$

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, May 19, 2017 at 07:49:06PM +0100, Andrew Cooper wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> We use sync_core() in the alternatives code to stop speculative
> execution of prefetched instructions because we are potentially changing
> them and don't want to execute stale bytes.
> 
> What it does on most machines is call CPUID which is a serializing
> instruction. And that's expensive.
> 
> However, the instruction cache is serialized when we're on the local CPU
> and are changing the data through the same virtual address. So then, we
> don't need the serializing CPUID but a simple control flow change. Last
> being accomplished with a CALL/RET which the noinline causes.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: Andy Lutomirski <luto@kernel.org>
> [Linux commit 34bfab0eaf0fb5c6fb14c6b4013b06cdc7984466]
> 
> Ported to Xen.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/arch/x86/alternative.c | 11 ++++++-----
>  xen/arch/x86/livepatch.c   | 20 +++++++++++++++-----

Weird that the CC list didn't pick me up.

But Acked-by on the livepatch part.

>  2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 6eaa10f..65062c2 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -128,13 +128,14 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
>   *
>   * You should run this with interrupts disabled or on code that is not
>   * executing.
> + *
> + * "noinline" to cause control flow change and thus invalidate I$ and
> + * cause refetch after modification.
>   */
> -static void *init_or_livepatch text_poke(void *addr, const void *opcode, size_t len)
> +static void *init_or_livepatch noinline
> +text_poke(void *addr, const void *opcode, size_t len)
>  {
> -    memcpy(addr, opcode, len);
> -    sync_core();
> -
> -    return addr;
> +    return memcpy(addr, opcode, len);
>  }
>  
>  /*
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 9663ef6..dd50dd1 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -46,7 +46,11 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
>      return 0;
>  }
>  
> -void arch_livepatch_apply(struct livepatch_func *func)
> +/*
> + * "noinline" to cause control flow change and thus invalidate I$ and
> + * cause refetch after modification.
> + */
> +void noinline arch_livepatch_apply(struct livepatch_func *func)
>  {
>      uint8_t *old_ptr;
>      uint8_t insn[sizeof(func->opaque)];
> @@ -75,15 +79,21 @@ void arch_livepatch_apply(struct livepatch_func *func)
>      memcpy(old_ptr, insn, len);
>  }
>  
> -void arch_livepatch_revert(const struct livepatch_func *func)
> +/*
> + * "noinline" to cause control flow change and thus invalidate I$ and
> + * cause refetch after modification.
> + */
> +void noinline arch_livepatch_revert(const struct livepatch_func *func)
>  {
>      memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
>  }
>  
> -/* Serialise the CPU pipeline. */
> -void arch_livepatch_post_action(void)
> +/*
> + * "noinline" to cause control flow change and thus invalidate I$ and
> + * cause refetch after modification.
> + */
> +void noinline arch_livepatch_post_action(void)
>  {
> -    cpuid_eax(0);
>  }
>  
>  static nmi_callback_t *saved_nmi_callback;
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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