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

[Juergen Gross <jgross@xxxxxxxx>]

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@xxxxxxxxxx> wrote:
>>> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>> 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.


Juergen

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