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 & ))
> #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
Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID
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 & ))
#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
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID
On Fri, Jun 26, 2020 at 11:10:00AM -0700, Luck, Tony wrote: > 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. Yes, you need both. Consider distros enabling everything so that they can ship a single kernel image. > And/or guidance on when to use each of the very somewhat simlar looking > boot_cpu_has() > static_cpu_has() See the comment over _static_cpu_has() in arch/x86/include/asm/cpufeature.h In the exception path you should use the static_ variant. > IS_ENABLED() This is testing CONFIG_ settings, i.e., build time. > cpu_feature_enabled() This too, indirectly. See arch/x86/include/asm/disabled-features.h -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID
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.
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()
options?
-Tony
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID
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?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID
Hi Fenghua,
On 2020/6/26 4:17, Fenghua Yu wrote:
A #GP fault is generated when ENQCMD instruction is executed without
a valid PASID value programmed in the current thread's PASID MSR. The
#GP fault handler will initialize the MSR if a PASID has been allocated
for this process.
Decoding the user instruction is ugly and sets a bad architecture
precedent. It may not function if the faulting instruction is modified
after #GP.
Thomas suggested to provide a reason for the #GP caused by executing ENQCMD
without a valid PASID value programmed. #GP error codes are 16 bits and all
16 bits are taken. Refer to SDM Vol 3, Chapter 16.13 for details. The other
choice was to reflect the error code in an MSR. ENQCMD can also cause #GP
when loading from the source operand, so its not fully comprehending all
the reasons. Rather than special case the ENQCMD, in future Intel may
choose a different fault mechanism for such cases if recovery is needed on
#GP.
The following heuristic is used to avoid decoding the user instructions
to determine the precise reason for the #GP fault:
1) If the mm for the process has not been allocated a PASID, this #GP
cannot be fixed.
2) If the PASID MSR is already initialized, then the #GP was for some
other reason
3) Try initializing the PASID MSR and returning. If the #GP was from
an ENQCMD this will fix it. If not, the #GP fault will be repeated
and will hit case "2".
For changes in Intel VT-d driver,
Reviewed-by: Lu Baolu
Best regards,
baolu
Suggested-by: Thomas Gleixner
Signed-off-by: Fenghua Yu
Reviewed-by: Tony Luck
---
v4:
- Change PASID type to u32 (Christoph)
v3:
- Check and set current->has_valid_pasid in fixup() (PeterZ)
- Add fpu__pasid_write() to update the MSR (PeterZ)
- Add ioasid_find() sanity check in fixup()
v2:
- Update the first paragraph of the commit message (Thomas)
- Add reasons why don't decode the user instruction and don't use
#GP error code (Thomas)
- Change get_task_mm() to current->mm (Thomas)
- Add comments on why IRQ is disabled during PASID fixup (Thomas)
- Add comment in fixup() that the function is called when #GP is from
user (so mm is not NULL) (Dave Hansen)
arch/x86/include/asm/iommu.h | 1 +
arch/x86/kernel/traps.c | 14 +++
drivers/iommu/intel/svm.c| 78
3 files changed, 93 insertions(+)
diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index ed41259fe7ac..e9365a5d6f7d 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -27,5 +27,6 @@ arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
}
void __free_pasid(struct mm_struct *mm);
+bool __fixup_pasid_exception(void);
#endif /* _ASM_X86_IOMMU_H */
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index f9727b96961f..2352f3f1f7ed 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -59,6 +59,7 @@
#include
#include
#include
+#include
#ifdef CONFIG_X86_64
#include
@@ -514,6 +515,16 @@ static enum kernel_gp_hint get_kernel_gp_address(struct
pt_regs *regs,
return GP_CANONICAL;
}
+static bool fixup_pasid_exception(void)
+{
+ if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
+ return false;
+ if (!static_cpu_has(X86_FEATURE_ENQCMD))
+ return false;
+
+ return __fixup_pasid_exception();
+}
+
#define GPFSTR "general protection fault"
DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
@@ -526,6 +537,9 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
cond_local_irq_enable(regs);
+ if (user_mode(regs) && fixup_pasid_exception())
+ goto exit;
+
if (static_cpu_has(X86_FEATURE_UMIP)) {
if (user_mode(regs) && fixup_umip_exception(regs))
goto exit;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 4c70b037..4a84c82a4f8c 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -1105,3 +1105,81 @@ void __free_pasid(struct mm_struct *mm)
*/
ioasid_free(pasid);
}
+
+/*
+ * Write the current task's PASID MSR/state. This is called only when PASID
+ * is enabled.
+ */
+static void fpu__pasid_write(u32 pasid)
+{
+ u64 msr_val = pasid | MSR_IA32_PASID_VALID;
+
+ fpregs_lock();
+
+ /*
+* If the MSR is active and owned by the current task's FPU, it can
+* be directly written.
+*
+* Otherwise, write the fpstate.
+*/
+ if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
+ wrmsrl(MSR_IA32_PASID, msr_val);
+ } else {
+ struct ia32_pasid_state *ppasid_state;
+
+ ppasid_state = get_xsave_addr(¤t->thread.fpu.state.xsave,
+ XFEATURE_PASID);
+ /*
+* ppasid_state shouldn't be NULL because XFEATURE_PASID
+* is enabled.
+*/
+
[PATCH v4 12/12] x86/traps: Fix up invalid PASID
A #GP fault is generated when ENQCMD instruction is executed without
a valid PASID value programmed in the current thread's PASID MSR. The
#GP fault handler will initialize the MSR if a PASID has been allocated
for this process.
Decoding the user instruction is ugly and sets a bad architecture
precedent. It may not function if the faulting instruction is modified
after #GP.
Thomas suggested to provide a reason for the #GP caused by executing ENQCMD
without a valid PASID value programmed. #GP error codes are 16 bits and all
16 bits are taken. Refer to SDM Vol 3, Chapter 16.13 for details. The other
choice was to reflect the error code in an MSR. ENQCMD can also cause #GP
when loading from the source operand, so its not fully comprehending all
the reasons. Rather than special case the ENQCMD, in future Intel may
choose a different fault mechanism for such cases if recovery is needed on
#GP.
The following heuristic is used to avoid decoding the user instructions
to determine the precise reason for the #GP fault:
1) If the mm for the process has not been allocated a PASID, this #GP
cannot be fixed.
2) If the PASID MSR is already initialized, then the #GP was for some
other reason
3) Try initializing the PASID MSR and returning. If the #GP was from
an ENQCMD this will fix it. If not, the #GP fault will be repeated
and will hit case "2".
Suggested-by: Thomas Gleixner
Signed-off-by: Fenghua Yu
Reviewed-by: Tony Luck
---
v4:
- Change PASID type to u32 (Christoph)
v3:
- Check and set current->has_valid_pasid in fixup() (PeterZ)
- Add fpu__pasid_write() to update the MSR (PeterZ)
- Add ioasid_find() sanity check in fixup()
v2:
- Update the first paragraph of the commit message (Thomas)
- Add reasons why don't decode the user instruction and don't use
#GP error code (Thomas)
- Change get_task_mm() to current->mm (Thomas)
- Add comments on why IRQ is disabled during PASID fixup (Thomas)
- Add comment in fixup() that the function is called when #GP is from
user (so mm is not NULL) (Dave Hansen)
arch/x86/include/asm/iommu.h | 1 +
arch/x86/kernel/traps.c | 14 +++
drivers/iommu/intel/svm.c| 78
3 files changed, 93 insertions(+)
diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index ed41259fe7ac..e9365a5d6f7d 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -27,5 +27,6 @@ arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
}
void __free_pasid(struct mm_struct *mm);
+bool __fixup_pasid_exception(void);
#endif /* _ASM_X86_IOMMU_H */
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index f9727b96961f..2352f3f1f7ed 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -59,6 +59,7 @@
#include
#include
#include
+#include
#ifdef CONFIG_X86_64
#include
@@ -514,6 +515,16 @@ static enum kernel_gp_hint get_kernel_gp_address(struct
pt_regs *regs,
return GP_CANONICAL;
}
+static bool fixup_pasid_exception(void)
+{
+ if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
+ return false;
+ if (!static_cpu_has(X86_FEATURE_ENQCMD))
+ return false;
+
+ return __fixup_pasid_exception();
+}
+
#define GPFSTR "general protection fault"
DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
@@ -526,6 +537,9 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
cond_local_irq_enable(regs);
+ if (user_mode(regs) && fixup_pasid_exception())
+ goto exit;
+
if (static_cpu_has(X86_FEATURE_UMIP)) {
if (user_mode(regs) && fixup_umip_exception(regs))
goto exit;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 4c70b037..4a84c82a4f8c 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -1105,3 +1105,81 @@ void __free_pasid(struct mm_struct *mm)
*/
ioasid_free(pasid);
}
+
+/*
+ * Write the current task's PASID MSR/state. This is called only when PASID
+ * is enabled.
+ */
+static void fpu__pasid_write(u32 pasid)
+{
+ u64 msr_val = pasid | MSR_IA32_PASID_VALID;
+
+ fpregs_lock();
+
+ /*
+* If the MSR is active and owned by the current task's FPU, it can
+* be directly written.
+*
+* Otherwise, write the fpstate.
+*/
+ if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
+ wrmsrl(MSR_IA32_PASID, msr_val);
+ } else {
+ struct ia32_pasid_state *ppasid_state;
+
+ ppasid_state = get_xsave_addr(¤t->thread.fpu.state.xsave,
+ XFEATURE_PASID);
+ /*
+* ppasid_state shouldn't be NULL because XFEATURE_PASID
+* is enabled.
+*/
+ WARN_ON_ONCE(!ppasid_state);
+ ppasid_state->pasid = msr_val;
+ }
+ fpregs_unlock();
+}
+
+/*
+ * Apply some heuristics to
