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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.18 at 17:15, <jgross@xxxxxxxx> wrote:
> On 29/03/18 16:19, Jan Beulich wrote:
>>>>> On 27.03.18 at 11:07, <jgross@xxxxxxxx> wrote:
>>> @@ -102,7 +103,21 @@ 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 ( cpu_has_invpcid )
>>> +        /*
>>> +         * If we are using PCID purge the TLB via INVPCID as loading cr3
>>> +         * will affect the current PCID only.
>> 
>> s/current/new/ ?
> 
> Okay.
> 
>> 
>>> +         * 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" );
>> 
>> What about the TLB entries that have been re-created between
>> the INVPCID and the write of CR3? It's not obvious to me that
>> leaving them around is not going to be a problem subsequently,
>> the more that you write cr3 frequently with the no-flush bit set.
> 
> The no-flush bit should not make any difference here. It controls only
> flushing of TLB-entries with the new PCID.

Right, but in a subsequent write to CR3 you may make active again
what was the old PCID here. This is in particular so for PCID 0 (which
has dual use).

>> Don't you need to do a single context invalidation for the prior
>> PCID (if different from the new one)?
> 
> Hmm, I don't think so, as the mov to %cr3 is a serializing instruction
> acting as a speculation barrier. So the only TLB entries which could
> survive would be the ones for the few instruction bytes after the
> invpcid instruction until the end of the mov to %cr3. Those are harmless
> as they are associated with the hypervisor PCID value, so there is no
> risk of any data leak to a guest. Maybe a comment explaining that would
> be a good idea.

Well, to be honest I don't trust in things like "speculation barrier"
anymore, at least not as far as things ahead of the barrier go.
Who knows what exactly the CPU does (and hence which TLB
entries it creates) between the INVPCID and the CR3 write. I
don't think we can blindly assume only entries for Xen mappings
could be created during that window.

Jan


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