Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 26 Jun 2020 11:35:07 -0700 | From | Fenghua Yu <> | Subject | Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID |
| |
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
| ![\](/images/icornerr.gif) |