Re: [Xen-devel] [PATCH v7 1/9] x86/xpti: avoid copying L4 page table contents when possible

[Juergen Gross <jgross@xxxxxxxx>]

On 16/04/18 20:22, Juergen Gross wrote:
> On 16/04/18 17:34, Tim Deegan wrote:
>> Hi,
>>
>> At 02:43 -0600 on 13 Apr (1523587395), Jan Beulich wrote:
>>>>>> On 12.04.18 at 20:09, <jgross@xxxxxxxx> wrote:
>>>> For mitigation of Meltdown the current L4 page table is copied to the
>>>> cpu local root page table each time a 64 bit pv guest is entered.
>>>>
>>>> Copying can be avoided in cases where the guest L4 page table hasn't
>>>> been modified while running the hypervisor, e.g. when handling
>>>> interrupts or any hypercall not modifying the L4 page table or %cr3.
>>>>
>>>> So add a per-cpu flag indicating whether the copying should be
>>>> performed and set that flag only when loading a new %cr3 or modifying
>>>> the L4 page table.  This includes synchronization of the cpu local
>>>> root page table with other cpus, so add a special synchronization flag
>>>> for that case.
>>>>
>>>> A simple performance check (compiling the hypervisor via "make -j 4")
>>>> in dom0 with 4 vcpus shows a significant improvement:
>>>>
>>>> - real time drops from 112 seconds to 103 seconds
>>>> - system time drops from 142 seconds to 131 seconds
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> ---
>>>> V7:
>>>> - add missing flag setting in shadow code
>>>
>>> This now needs an ack from Tim (now Cc-ed).
>>
>> I may be misunderstanding how this flag is supposed to work, but this
>> seems at first glance to do both too much and too little.  The sl4
>> table that's being modified may be in use on any number of other
>> pcpus, and might not be in use on the current pcpu.
> 
> Okay.
> 
>> It looks like the do_mmu_update path issues a flush IPI; I think that
>> the equivalent IPI would be a better place to hook if you can.
> 
> I want to catch only cases where the L4 table is being modified.
> 
>> Also I'm not sure why the flag needs to be set in
>> l4e_propagate_from_guest() as well as shadow_set_l4e().  Can you
>> elaborate?
> 
> I narrowed down iteratively where the setting of the flag is mandatory.
> Doing it in shadow_set_l4e() seemed to look right, but it wasn't enough.
> Setting it also in l4e_propagate_from_guest() showed no further problems
> migrating the guest.

I just revisited this after looking into the code. Seems I remembered
the sequence of narrowing down wrong. l4e_propagate_from_guest() doesn't
need to set the flag.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel