Re: [PATCH 6/7] x86/traps: Fix up invalid PASID
Fenghua Yu writes:
> A #GP fault is generated when ENQCMD instruction is executed without
> a valid PASID value programmed in.
Programmed in what?
> The #GP fault handler will initialize the current thread's PASID MSR.
>
> 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 we will hit case "2".
>
> Suggested-by: Thomas Gleixner
Just for the record I also suggested to have a proper errorcode in the
#GP for ENQCMD and I surely did not suggest to avoid decoding the user
instructions.
> 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 6ef00eb6fbb9..369b5ba94635 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -56,6 +56,7 @@
> #include
> #include
> #include
> +#include
>
> #ifdef CONFIG_X86_64
> #include
> @@ -488,6 +489,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"
>
> dotraplinkage void do_general_protection(struct pt_regs *regs, long
> error_code)
> @@ -499,6 +510,12 @@ dotraplinkage void do_general_protection(struct pt_regs
> *regs, long error_code)
> int ret;
>
> RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> +
> + if (user_mode(regs) && fixup_pasid_exception()) {
> + cond_local_irq_enable(regs);
The point of this conditional irq enable _AFTER_ calling into the fixup
function is? Also what's the reason for keeping interrupts disabled
while calling into that function? Comments exist for a reason.
> + return;
> + }
> +
> cond_local_irq_enable(regs);
>
> if (static_cpu_has(X86_FEATURE_UMIP)) {
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index da718a49e91e..5ed39a022adb 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -759,3 +759,40 @@ void __free_pasid(struct mm_struct *mm)
>*/
> ioasid_free(pasid);
> }
> +
> +/*
> + * Fix up the PASID MSR if possible.
> + *
> + * But if the #GP was due to another reason, a second #GP might be triggered
> + * to force proper behavior.
> + */
> +bool __fixup_pasid_exception(void)
> +{
> + struct mm_struct *mm;
> + bool ret = true;
> + u64 pasid_msr;
> + int pasid;
> +
> + mm = get_task_mm(current);
Why do you need a reference to current->mm ?
> + /* This #GP was triggered from user mode. So mm cannot be NULL. */
> + pasid = mm->context.pasid;
> + /* Ensure this process has been bound to a PASID. */
> + if (!pasid) {
> + ret = false;
> + goto out;
> + }
> +
> + /* Check to see if the PASID MSR has already been set for this task. */
> + rdmsrl(MSR_IA32_PASID, pasid_msr);
> + if (pasid_msr & MSR_IA32_PASID_VALID) {
> + ret = false;
> + goto out;
> + }
> +
> + /* Fix up the MSR. */
> + wrmsrl(MSR_IA32_PASID, pasid | MSR_IA32_PASID_VALID);
> +out:
> + mmput(mm);
Thanks,
tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
Fenghua Yu writes:
> PASID is shared by all threads in a process. So the logical place to keep
> track of it is in the "mm". Add the field to the architecture specific
> mm_context_t structure.
>
> A PASID is allocated for an "mm" the first time any thread attaches
> to an SVM capable device. Later device atatches (whether to the same
atatches?
> device or another SVM device) will re-use the same PASID.
>
> The PASID is freed when the process exits (so no need to keep
> reference counts on how many SVM devices are sharing the PASID).
I'm not buying that. If there is an outstanding request with the PASID
of a process then tearing down the process address space and freeing the
PASID (which might be reused) is fundamentally broken.
> +void __free_pasid(struct mm_struct *mm);
> +
> #endif /* _ASM_X86_IOMMU_H */
> diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h
> index bdeae9291e5c..137bf51f19e6 100644
> --- a/arch/x86/include/asm/mmu.h
> +++ b/arch/x86/include/asm/mmu.h
> @@ -50,6 +50,10 @@ typedef struct {
> u16 pkey_allocation_map;
> s16 execute_only_pkey;
> #endif
> +
> +#ifdef CONFIG_INTEL_IOMMU_SVM
> + int pasid;
int? It's a value which gets programmed into the MSR along with the
valid bit (bit 31) set.
> extern void switch_mm(struct mm_struct *prev, struct mm_struct *next,
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index d7f2a5358900..da718a49e91e 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -226,6 +226,45 @@ static LIST_HEAD(global_svm_list);
> list_for_each_entry((sdev), &(svm)->devs, list) \
> if ((d) != (sdev)->dev) {} else
>
> +/*
> + * If this mm already has a PASID we can use it. Otherwise allocate a new
> one.
> + * Let the caller know if we did an allocation via 'new_pasid'.
> + */
> +static int alloc_pasid(struct intel_svm *svm, struct mm_struct *mm,
> +int pasid_max, bool *new_pasid, int flags)
Again, data types please. flags are generally unsigned and not plain
int. Also pasid_max is certainly not plain int either.
> +{
> + int pasid;
> +
> + /*
> + * Reuse the PASID if the mm already has a PASID and not a private
> + * PASID is requested.
> + */
> + if (mm && mm->context.pasid && !(flags & SVM_FLAG_PRIVATE_PASID)) {
> + /*
> + * Once a PASID is allocated for this mm, the PASID
> + * stays with the mm until the mm is dropped. Reuse
> + * the PASID which has been already allocated for the
> + * mm instead of allocating a new one.
> + */
> + ioasid_set_data(mm->context.pasid, svm);
So if the PASID is reused several times for different SVMs then every
time ioasid_data->private is set to a different SVM. How is that
supposed to work?
> + *new_pasid = false;
> +
> + return mm->context.pasid;
> + }
> +
> + /*
> + * Allocate a new pasid. Do not use PASID 0, reserved for RID to
> + * PASID.
> + */
> + pasid = ioasid_alloc(NULL, PASID_MIN, pasid_max - 1, svm);
ioasid_alloc() uses ioasid_t which is
typedef unsigned int ioasid_t;
Can we please have consistent types and behaviour all over the place?
> + if (pasid == INVALID_IOASID)
> + return -ENOSPC;
> +
> + *new_pasid = true;
> +
> + return pasid;
> +}
> +
> int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct
> svm_dev_ops *ops)
> {
> struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
> @@ -324,6 +363,8 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int
> flags, struct svm_dev_
> init_rcu_head(>rcu);
>
> if (!svm) {
> + bool new_pasid;
> +
> svm = kzalloc(sizeof(*svm), GFP_KERNEL);
> if (!svm) {
> ret = -ENOMEM;
> @@ -335,15 +376,13 @@ int intel_svm_bind_mm(struct device *dev, int *pasid,
> int flags, struct svm_dev_
> if (pasid_max > intel_pasid_max_id)
> pasid_max = intel_pasid_max_id;
>
> - /* Do not use PASID 0, reserved for RID to PASID */
> - svm->pasid = ioasid_alloc(NULL, PASID_MIN,
> - pasid_max - 1, svm);
> - if (svm->pasid == INVALID_IOASID) {
> + svm->pasid = alloc_pasid(svm, mm, pasid_max, _pasid, flags);
> + if (svm->pasid < 0) {
> kfree(svm);
> kfree(sdev);
> - ret = -ENOSPC;
ret gets magically initialized to an error return value, right?
> goto out;
> }
> +
> svm->notifier.ops = _mmuops;
> svm->mm = mm;
> svm->flags = flags;
> @@ -353,7 +392,8 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int
> flags, struct svm_dev_
> if (mm) {
> ret =
Re: [PATCH 4/7] x86/msr-index: Define IA32_PASID MSR
Fenghua Yu writes: > The IA32_PASID MSR (0xd93) contains the Process Address Space Identifier > (PASID), a 20-bit value. Bit 31 must be set to indicate the value > programmed in the MSR is valid. Hardware uses PASID to identify which > process submits the work and direct responses to the right process. No. It does not identify the process. It identifies the process' address space as the name says. Please provide coherent and precise information. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/7] x86/fpu/xstate: Add supervisor PASID state for ENQCMD feature
Fenghua Yu writes: > From: Yu-cheng Yu > > The IA32_PASID MSR is used when a task submits work via the ENQCMD > instruction. Is used? > The per task MSR is stored in the task's supervisor FPU per task MSR? Lot's of MSRs > PASID state and is context switched by XSAVES/XRSTORS. > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/7] x86/cpufeatures: Enumerate ENQCMD and ENQCMDS instructions
Fenghua Yu writes: > A user space application can execute ENQCMD instruction to submit work > to device. The kernel executes ENQCMDS instruction to submit work to > device. So a user space application _can_ execute ENQCMD and the kernel executes ENQCMDS. And both submit work to device. > There is a lot of other enabling needed for the instructions to actually > be usable in user space and the kernel, and that enabling is coming later > in the series and in device drivers. That's important information to the enumeration of the instructions in which way? Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/7] docs: x86: Add a documentation for ENQCMD
Fenghua Yu writes: s/Add a documentation/Add documentation/ > From: Ashok Raj > > ENQCMD and Data Streaming Accelerator (DSA) and all of their associated > features are a complicated stack with lots of interconnected pieces. > This documentation provides a big picture overview for all of the > features. > > Signed-off-by: Ashok Raj > Co-developed-by: Fenghua Yu > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck > --- > Documentation/x86/enqcmd.rst | 185 +++ How is that hooked up into the Documentation index? Documentation/x86/enqcmd.rst: WARNING: document isn't included in any toctree > +++ b/Documentation/x86/enqcmd.rst > @@ -0,0 +1,185 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +Improved Device Interaction Overview So the document is about ENQCMD, right? Can you please make that in some way consistently named? > + > +== Background == This lacks any docbook formatting The resulting HTML looks like ... > + > +Shared Virtual Addressing (SVA) allows the processor and device to use the > +same virtual addresses avoiding the need for software to translate virtual > +addresses to physical addresses. ENQCMD is a new instruction on Intel > +platforms that allows user applications to directly notify hardware of new > +work, much like doorbells are used in some hardware, but carries a payload > +that carries the PASID and some additional device specific commands > +along with it. Sorry that's not background information, that's an agglomeration of words. Can you please explain properly what's the background of SVA, how it differs from regular device addressing and what kind of requirements it has? ENQCMD is not related to background. It's part of the new technology. > +== Address Space Tagging == > + > +A new MSR (MSR_IA32_PASID) allows an application address space to be > +associated with what the PCIe spec calls a Process Address Space ID > +(PASID). This PASID tag is carried along with all requests between > +applications and devices and allows devices to interact with the process > +address space. Sigh. The important part here is not the MSR. The important part is to explain what PASID is and where it comes from. Documentation has similar rules as changelogs: 1) Provide context 2) Explain requirements 3) Explain implementation The pile you provided is completely backwards and unstructured. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
