Messages in this thread | | | Date | Fri, 26 Jun 2020 11:16:24 -0700 | From | Fenghua Yu <> | Subject | Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID |
| |
Hi, Peter,
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? >
I will use the second one with cpu_feature_enabled() for both cases.
The CONFIG thing is for compilation time optimization when CONFIG_INTEL_IOMMU_SVM is not set.
If CONFIG_INTEL_IOMMU_SVM is not set, IS_ENABLED(CONFIG_INTEL_IOMMU_SVM) is "false" during compilation time. Then GCC will optimize fixup_pasid_execption() to empty and will not define __fixup_pasid_exception() at all because no one calls it.
If CONFIG_INTEL_IOMMU_SVM is set, IS_ENABLED(...) is always true. Depending on cpu_feature_enabled(X86_FEATURE_ENQCMD), __fixup_pasid_execption() will be called or not during run time.
Does it make sense?
Do you want me to define a helper enqcmd_enabled()?
static inline bool enqcmd_enabled(void) { if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM)) return false; if (!static_cpu_has(X86_FEATURE_ENQCMD)) return false; return true; }
Then both fixup_pasid_execption() and free_pasid() can call it.
static bool fixup_pasid_exception(void) { if (!enqcmd_enabled()) return false; return __fixup_pasid_exception(); }
statis inline void free_pasid(struct m_struct *mm) { if (!enqcmd_enabled()) return;
__free_pasid(mm); }
Please advice.
-Fenghua
| |