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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.04.18 at 16:33, <jgross@xxxxxxxx> wrote:
> 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.

The first sentence is fine (and useful), so please don't drop it
wholesale.

>>> --- 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?

No - the involved symbol names together make clear that the
NOFLUSH bit can't be included. The same bit can't possibly be
part of the address _and_ be the signal to not flush.

Jan


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