Re: [Xen-devel] [PATCH v8 9/9] xen/x86: use PCID feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.04.18 at 11:37, <jgross@xxxxxxxx> wrote:
> On 18/04/18 11:13, Jan Beulich wrote:
>>>>> On 18.04.18 at 10:30, <jgross@xxxxxxxx> wrote:
>>> Avoid flushing the complete TLB when switching %cr3 for mitigation of
>>> Meltdown by using the PCID feature if available.
>>>
>>> We are using 4 PCID values for a 64 bit pv domain subject to XPTI and
>>> 2 values for the non-XPTI case:
>>>
>>> - guest active and in kernel mode
>>> - guest active and in user mode
>>> - hypervisor active and guest in user mode (XPTI only)
>>> - hypervisor active and guest in kernel mode (XPTI only)
>>>
>>> We use PCID only if PCID _and_ INVPCID are supported. With PCID in use
>>> we disable global pages in cr4. A command line parameter controls in
>>> which cases PCID is being used.
>>>
>>> As the non-XPTI case has shown not to perform better with PCID at least
>>> on some machines the default is to use PCID only for domains subject to
>>> XPTI.
>>>
>>> With PCID enabled we always disable global pages. This avoids having to
>>> either flush the complete TLB or do a cycle through all PCID values
>>> when invalidating a single global page.
>>>
>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> V6.1:
>>> - address some minor comments (Jan Beulich)
>> 
>> No v7 changes? Afaict ...
>> 
>>> @@ -717,7 +718,7 @@ int __init dom0_construct_pv(struct domain *d,
>>>          update_cr3(v);
>>>  
>>>      /* We run on dom0's page tables for the final part of the build 
> process. */
>>> -    switch_cr3_cr4(v->arch.cr3, read_cr4());
>>> +    switch_cr3_cr4(cr3_pa(v->arch.cr3), read_cr4());
>> 
>> ... at least this was added. And to be honest I'm not convinced cr3_pa() is
>> the right construct to use here: It's neither clear why the other bits don't
>> matter at this point, nor why any of the bits that you mask out this way
>> need masking out in the first place (e.g. why, in the crash case that Andrew
>> had observed, the noflush bit was wrongly set).
> 
> At this point in time we only want to use another cr3 value. So PCIDs
> should _not_ be used as this would require adapting cr4, resulting in
> writing the cr3_pa() bits only. It would have been possible to use
> write_cr3() here, but only together with open coding a TLB flush in
> order to avoid any global pages remaining in the TLB. So I decided to
> use switch_cr3_cr4().
> 
> The noflush bit was set as dom0 is subject to XPTI and PCID usage and
> update_cr3() is updating v->arch.cr3 accordingly.
> 
> I know you are not fond of setting noflush in v->arch.cr3. The only
> sensible alternative to that would be adding something like
> v->arch.cr3_pcid_bits being or-ed to the cr3 value in restore_all_guest.
> This would let us get rid of having to use cr3_pa() in some places
> while adding a (very little) performance penalty.

Well, I had accepted the current approach as the (performance wise)
better alternative, but with Andrew sharing that original concern of mine,
and with there having been a first real problem resulting from that
approach, more seriously considering the alternative certainly seems
necessary. Andrew?

>> I hope there aren't any other such hidden changes in this version of the
>> series.
> 
> Oh sorry, I missed to update the history.
> 
> I'm not aware of other changes in v8 (apart from patch 1 and the
> resulting needed change in patch 3).

Ah - I hadn't spotted that follow-on change, and I've just sent a small
comment in its regard.

Jan



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