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

From: Fenghua Yu <fenghua.yu@intel.com>
To: Dave Hansen <dave.hansen@intel.com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>,
	"Luck, Tony" <tony.luck@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Ashok Raj <ashok.raj@intel.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>,
	Peter Zijlstra <peterz@infradead.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:35:07 -0700	[thread overview]
Message-ID: <20200626183506.GB32961@romley-ivt3.sc.intel.com> (raw)
In-Reply-To: <aa3d7b7c-d6aa-06b4-30f7-0e90af50a1f3@intel.com>

Hi, Dave,

On Fri, Jun 26, 2020 at 11:23:12AM -0700, Dave Hansen wrote:
> 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?

ENQCMD instruction needs bind_mm()/unbind_mm() defined in svm.c
which is only compiled by:
obj-$(CONFIG_INTEL_IOMMU_SVM) += intel/svm.o

So I think ENQCMD instruction is truly dependent on CONFIG_INTEL_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".

This makes code simpler and cleaner.

Thanks.

-Fenghua
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu