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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/05/17 14:38, Jan Beulich wrote:
>>>> 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?

There was a long LKML thread on the subject. 
https://lkml.org/lkml/2016/12/3/108

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

I think https://lkml.org/lkml/2016/12/2/454 is probably the relevant
statement.

>
>> 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?

That would be fine, because its still the same linear addresses, and
would still be synchronised.

In reality for livepatching, we only modify the 5 byte prefix at the
head of the functions, so this memcpy() wouldn't actually hit itself.

Of course, you could construct a patch which would cause memcpy() to
actually hit itself, but that isn't going to work at all, irrespective
of serialising instructions.

~Andrew

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