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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.17 at 20:49, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Do you have the background of this "same virtual address"
constraint? Caches are physically indexed, so I don't see the
connection. Yet if there is one, our stub generation in the
emulator may have an issue.

> 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@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> Reviewed-by: Andy Lutomirski <luto@xxxxxxxxxx>
> [Linux commit 34bfab0eaf0fb5c6fb14c6b4013b06cdc7984466]
> 
> Ported to Xen.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
with a question:

> --- 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);
>  }

What if this is patching memcpy() itself?

Jan


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