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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

(In fact - I'm considering using this property to try and allow us to
deliberately trigger MCEs on demand, for testing purposes).

~Andrew

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