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
