Re: [Xen-devel] [PATCH v5 7/7] xen/x86: use PCID feature

[Juergen Gross <jgross@xxxxxxxx>]

On 09/04/18 16:15, Jan Beulich wrote:
>>>> On 06.04.18 at 09:52, <jgross@xxxxxxxx> wrote:
>> @@ -100,12 +102,35 @@ void write_cr3_cr4(unsigned long cr3, unsigned long 
>> cr4)
>>      t = pre_flush();
>>  
>>      if ( read_cr4() & X86_CR4_PGE )
>> +        /*
>> +         * X86_CR4_PGE set means PCID being inactive.
>> +         * We have to purge the TLB via flipping cr4.pge.
>> +         */
>>          write_cr4(cr4 & ~X86_CR4_PGE);
>> +    else if ( use_invpcid )
>> +        /*
>> +         * If we are using PCID purge the TLB via INVPCID as loading cr3
>> +         * will affect the new PCID only.
>> +         * If INVPCID is not supported we don't use PCIDs so loading cr3
>> +         * will purge the TLB (we are in the "global pages off" branch).
>> +         * invpcid_flush_all_nonglobals() seems to be faster than
>> +         * invpcid_flush_all().
>> +         */
>> +        invpcid_flush_all_nonglobals();
>>  
>>      asm volatile ( "mov %0, %%cr3" : : "r" (cr3) : "memory" );
>>  
>>      if ( read_cr4() != cr4 )
>>          write_cr4(cr4);
>> +    else if ( old_pcid != (cr3 & X86_CR3_PCID_MASK) )
>> +        /*
>> +         * Make sure no TLB entries related to the old PCID created between
>> +         * flushing the TLB and writing the new %cr3 value remain in the 
>> TLB.
>> +         * Writing %cr3 is documented to be a speculation barrier, OTOH the
>> +         * performance impact of the additional flush is next to invisible.
>> +         * So better be save than sorry.
>> +         */
>> +        invpcid_flush_single_context(old_pcid);
> 
> I'm not really happy about this comment. The CR3 write being a
> speculation barrier is of no real interest here. Until the CPU's
> speculation logic reaches that insn, all sort of things can happen.
> We don't even know the exact code the compiler will generate,
> much less what that code will trigger inside the CPU.

In case you are feeling strong regarding this comment I can
remove it.

> 
> Also I think it's "safe" in the last sentence.

Of course.

> 
>> --- a/xen/include/asm-x86/x86-defns.h
>> +++ b/xen/include/asm-x86/x86-defns.h
>> @@ -45,7 +45,9 @@
>>  /*
>>   * Intel CPU flags in CR3
>>   */
>> -#define X86_CR3_NOFLUSH (_AC(1, ULL) << 63)
>> +#define X86_CR3_NOFLUSH    (_AC(1, ULL) << 63)
>> +#define X86_CR3_ADDR_MASK  (PAGE_MASK & PADDR_MASK & ~X86_CR3_NOFLUSH)
> 
> Why still ~X86_CR3_NOFLUSH now that you use PADDR_MASK?

I just want to make clear that NOFLUSH is not included. Would you
like a comment better?


Juergen

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