Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/03/18 12:54, Jan Beulich wrote:
>>>> On 05.03.18 at 13:35, <andrew.cooper3@citrix.com> wrote:
>> On 05/03/18 12:06, Juergen Gross wrote:
>>> On 05/03/18 12:50, Andrew Cooper wrote:
>>>> On 05/03/18 11:31, Jan Beulich wrote:
>>>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>>>> No description at all? I'd at least expect mention of how much of a
>>>>> performance win this is (for whichever hardware you happen to
>>>>> know that).
>>>>>
>>>>>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned 
>> int flags)
>>>>>>          else
>>>>>>          {
>>>>>>              u32 t = pre_flush();
>>>>>> -            unsigned long cr4 = read_cr4();
>>>>>>  
>>>>>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>>>>>> -            barrier();
>>>>>> -            write_cr4(cr4);
>>>>>> +            if ( !cpu_has_invpcid )
>>>>>> +            {
>>>>>> +                unsigned long cr4 = read_cr4();
>>>>>> +
>>>>>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>>>>>> +                barrier();
>>>>>> +                write_cr4(cr4);
>>>>>> +            }
>>>>>> +            else
>>>>>> +            {
>>>>>> +                /*
>>>>>> +                 * Using invpcid to flush all mappings works
>>>>>> +                 * regardless of whether PCID is enabled or not.
>>>>>> +                 * It is faster than read-modify-write CR4.
>>>>>> +                 */
>>>> Its a cr4 double write, rather than RMW.  We read from a cached value
>>>> anyway, not from hardware.
>>>>
>>>>>> +                invpcid_flush_all();
>>>>>> +            }
>>>>> The reference to PCID in the comment isn't really meaningful imo.
>>>>> PCID and INVPCID are independent features anyway. Also please
>>>>> don't create artificially short comment lines.
>>>>>
>>>>> Generally I also think such if() conditions would better be inverted:
>>>>> There's no reason to make the legacy form look as if it was
>>>>> preferred.
>>>>>
>>>>> And then - what about the use in write_cr3() and the two uses that
>>>>> remain after my XPTI follow-up series (which sadly looks to be stuck
>>>>> for whatever reason), or (without that series) the write_cr3
>>>>> assembler macro?
>>>> I don't think it is safe to use invpcid when we're also switching cr3. 
>>>> The new cr3 may have global pages with different translations, as they
>>>> are guest controlled.
>>> Can you elaborate a little bit more?
>>>
>>> How can a guest control any hypervisor mappings? As long as the new cr3
>>> is being loaded before the TLB is flushed via INVPCID I can't see how
>>> a problem should occur.
>>>
>>> In fact my series does exactly what Jan is asking above: it is replacing
>>> the remaining cr4 based TLB flushing by INVPCID if possible. So in case
>>> there is a flaw in my design please tell me.
>> At the moment, we have guest and hypervisor controlled global mappings.
>>
>> The current switch is:
>> cr4 &= ~PGE;
>> cr3 = new_cr3;
>> cr4 |= PGE;
>>
>> which means that all global mappings are flushed by the first action,
>> and no new global mappings can come into existence.  We then switch to
>> the new cr3 (again with global fully disabled), then allow global
>> mappings to come back into existence.
>>
>> With the invpcid route, we switch via:
>>
>> cr3 = new_cr3;
>> invpcid all+global;
>>
>> This has a race window where global mappings are active, and could
>> mismatch what is in cr3.  This yields #MC on at least some hardware, and
>> is specified to have undefined behaviour. 
> Oh, right, this would be okay only without what used to be named
> USER_MAPPINGS_ARE_GLOBAL (and what is now implied).

When we start using PCID for user mappings, then we don't need them to
be global, at which point we can require/expect that the only global
mappings are hypervisor ones which we expect to remain correct across a
write to cr3.  However, if we do this, then we need to use a bit other
than PAGE_GLOBAL to signify guest user mappings.

I think this is doable, but I don't think it is going to be trivial to
get correct.

~Andrew

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