Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <92462889-16e9-5c7b-a524-fe53859f93c1@intel.com>
Date:   Fri, 14 Jul 2023 16:01:59 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] x86/mm: Remove "INVPCID single" feature tracking
Content-Language: en-US
To:     andrew.cooper3@citrix.com,
        Dave Hansen <dave.hansen@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org, jannh@google.com, x86@kernel.org,
        luto@kernel.org, peterz@infradead.org
References: <20230714183555.7DD052F4@davehans-spike.ostc.intel.com>
 <df0750e0-dd6f-7418-53bd-64a9ad1e0086@citrix.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <df0750e0-dd6f-7418-53bd-64a9ad1e0086@citrix.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/14/23 13:27, andrew.cooper3@citrix.com 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.