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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.05.17 at 15:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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 

Well, interesting reading (and at least part of it was Cc-ed to
xen-devel iirc), but none of it nor ...

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

... this one doesn't give any background at all of why the
virtual address would matter here. Searching the SDM I also can't
find any statement as to virtual or physical indexing being used
for any of the caches.

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

Hmm, my question was more from an alternative instruction
patching viewpoint.

Jan


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