Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID

From: Dave Hansen <dave.hansen@intel.com>
To: "Luck, Tony" <tony.luck@intel.com>,
	Peter Zijlstra <peterz@infradead.org>
Cc: Fenghua Yu <fenghua.yu@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Ashok Raj <ashok.raj@intel.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	x86 <x86@kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>,
	iommu@lists.linux-foundation.org, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>,
	Jacob Jun Pan <jacob.jun.pan@intel.com>,
	H Peter Anvin <hpa@zytor.com>,
	Christoph Hellwig <hch@infradeed.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID
Date: Fri, 26 Jun 2020 11:23:12 -0700	[thread overview]
Message-ID: <aa3d7b7c-d6aa-06b4-30f7-0e90af50a1f3@intel.com> (raw)
In-Reply-To: <20200626181000.GA22833@agluck-desk2.amr.corp.intel.com>

On 6/26/20 11:10 AM, Luck, Tony wrote:
> On Fri, Jun 26, 2020 at 11:44:50AM +0200, Peter Zijlstra wrote:
>> On Thu, Jun 25, 2020 at 01:17:22PM -0700, Fenghua Yu wrote:
>>
>>> +static bool fixup_pasid_exception(void)
>>> +{
>>> +	if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
>>> +		return false;
>>> +	if (!static_cpu_has(X86_FEATURE_ENQCMD))
>>> +		return false;
>>
>> elsewhere you had another variation:
>>
>> +       if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
>> +               return;
>> +
>> +       if (!cpu_feature_enabled(X86_FEATURE_ENQCMD))
>> +               return;
>>
>> Which is it, and why do we need the CONFIG thing when combined with the
>> enabled thing?
> 
> Do we have a standard way of coding for a feature that depends on multiple
> other features?  For this case the system needs to both suport the ENQCMD
> instruction, and also have kernel code that programs the IOMMU.

Not really a standard one.

We could setup_clear_cpu_cap(X86_FEATURE_ENQCMD) during boot if we see
that CONFIG_INTEL_IOMMU_SVM=n or if we don't have a detected IOMMU.
That would at least get static value patching which would make some of
the feature checks very cheap.

That means we can't use ENQCMD at all in the kernel, though.  Is that an
issue?  Is the CPU feature truly dependent on IOMMU_SVM?

> And/or guidance on when to use each of the very somewhat simlar looking
> 	boot_cpu_has()
> 	static_cpu_has()
> 	IS_ENABLED()
> 	cpu_feature_enabled(X86_FEATURE_ENQCMD)
> options?

cpu_feature_enabled() is by go-to for checking X86_FEATUREs.  It has
compile-time checking along with static checking.

If you use cpu_feature_enabled(), and we added:

#ifdef CONFIG_INTEL_IOMMU_SVM
# define DISABLE_ENQCMD         0
#else
# define DISABLE_ENQCMD           (1 << (X86_FEATURE_ENQCMD & <bitval>))
#endif

to arch/x86/include/asm/disabled-features.h, then we could check only
X86_FEATURE_ENQCMD, and we'd get that IS_ENABLED() check for "free".
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu