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

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

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. 

~Andrew

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