Re: [PATCH] x86/mm: Remove "INVPCID single" feature tracking

[Dave Hansen]

On 7/14/23 13:27, andrew.cooper3@xxxxxxxxxx wrote:
>> +	/* If PTI is off there is no user PCID and nothing to flush. */
>>  	if (!static_cpu_has(X86_FEATURE_PTI))
>>  		return;
> 
> As a minor observation, the common case is for the function to exit
> here, but you've got both this_cpu_read()'s ahead of a full compiler
> memory barrier.
> 
> If you move them here, you'll drop the reads from the common case.
> But...

That's a good point.  I was depending on the generosity of the compiler
but the invlpg throws that out the window.  I'll move them around.

>>  	/*
>> -	 * Some platforms #GP if we call invpcid(type=1/2) before CR4.PCIDE=1.
>> -	 * Just use invalidate_user_asid() in case we are called early.
>> +	 * invpcid_flush_one(pcid>0) will #GP if CR4.PCIDE==0.  Check
>> +	 * 'cpu_pcide' to ensure that *this* CPU will not trigger those
>> +	 * #GP's even if called before CR4.PCIDE has been initialized.
>>  	 */
>> -	if (!this_cpu_has(X86_FEATURE_INVPCID_SINGLE))
>> -		invalidate_user_asid(loaded_mm_asid);
>> -	else
>> +	if (boot_cpu_has(X86_FEATURE_INVPCID) && cpu_pcide)
> ... why can't this just be && loaded_mm_asid ?
> 
> There's no plausible way the asid can be nonzero here without CR4.PCIDE
> being set, and that avoids looking at cr4 directly.

Except that 0 is a valid, normal 'loaded_mm_asid' value.  It would be
quite possible to have loaded_mm_asid==0 during normal runtime which
would drop down into the invalidate_user_asid() case.  It would work,
but it would be unnecessarily destructive to the TLB.

I guess we _could_ adjust the asids to go from 1=>TLB_NR_DYN_ASIDS
instead of 0=>TLB_NR_DYN_ASIDS-1.  *But*, PTI is slow path code these
days.  I'd rather read one more (presumably) cache hot variable that's
logically clear than go messing with the ASID allocation code making
ASID 0 even more special.