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

So the latter is strictly required, while the former might be not
necessary.

What you are telling me is I need to set the flag on the other cpus,
too. I didn't experience any problems omitting that, but maybe this was
only because the flag would be set eventually some time later (e.g. in
case of a vcpu scheduling event or a scheduling in the guest leading to
the flag being set), resulting in a short hang instead of crash.


Juergen

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