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

[Juergen Gross <jgross@xxxxxxxx>]

On 18/04/18 17:32, Jan Beulich wrote:
>>>> 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'm fine with both variants. I just need to know which one will be
accepted. :-)


Juergen

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