Hi Baolu, thanks for your review.
> On 5/20/26 23:07, Joonwon Kang wrote:
> > For SVA, the IOMMU core always allocates PASID from the global PASID
> > space. The use of this global PASID space comes from the limitation of
> > the ENQCMD instruction in Intel CPUs that it fetches its PASID operand
> > from IA32_PASID, which is per-process; when a process wants to
> > communicate with multiple devices with the ENQCMD instruction, it cannot
> > change its PASID for each device without the kernel's intervention. Also
> > note that ARM introduced a similar instruction, which is ST64BV0.
> >
> > Due to this nature, SVA with ARM SMMU v3 has been found not working in
> > our environment when other modules/devices compete for PASID. The
> > environment looks as follows:
> >
> > - The device is not a PCIe device.
> > - The device is to use SVA.
> > - The supported SSID/PASID space is very small for the device; only 1 to
> > 3 SSIDs are supported.
> >
> > With this setup, when other modules have allocated all the PASIDs that
> > our device is expected to use from the global PASID space via APIs like
> > iommu_alloc_global_pasid() or iommu_sva_bind_device(), SVA binding to
> > our device fails due to the lack of available PASIDs.
> >
> > This commit resolves the issue by allowing device driver to maintain its
> > own PASID space and assign a PASID from that for the process-device bond
> > via a new API called `iommu_sva_bind_device_pasid(dev, mm, pasid)`. Doing
> > that, however, will disallow the process to execute the ENQCMD-like
> > instructions at EL0. It is because the process cannot change its PASID in
> > IA32_PASID(or ACCDATA_EL1 on ARM) for each device without the kernel's
> > intervention. For this reason, calling `iommu_sva_bind_device()` and then
> > `iommu_sva_bind_device_pasid()` for the same process will not be allowed
> > and vice versa.
> >
> > Currently, there is a limitation that a process simultaneously doing SVA
> > with multiple devices with different PASIDs is not supported. So, calling
> > `iommu_sva_bind_device_pasid()` multiple times for the same process with
> > different devices will not be allowed for now while that for
> > `iommu_sva_bind_device()` will be.
> >
> > Another limitation is that a process cannot do `iommu_sva_bind_device()`
> > if it has ever done `iommu_sva_bind_device_pasid()` even though it has
> > been unbound after use.
> >
> > Suggested-by: Jason Gunthorpe<jgg at ziepe.ca>
> > Suggested-by: Kevin Tian<kevin.tian at intel.com>
> > Signed-off-by: Joonwon Kang<joonwonkang at google.com>
> > ---
> > v2: Reuse iommu_mm->pasid after SVA bound by iommu_sva_bind_device_pasid()
> > is unbound.
> > v1: Initial version.
> >
> > arch/x86/kernel/traps.c | 9 +--
> > drivers/iommu/iommu-sva.c | 151 +++++++++++++++++++++++++++++---------
> > include/linux/iommu.h | 14 +++-
> > 3 files changed, 134 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index 0ca3912ecb7f..0131c8e5fb10 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -857,13 +857,12 @@ static bool try_fixup_enqcmd_gp(void)
> > return false;
> >
> > /*
> > - * If the mm has not been allocated a
> > - * PASID, the #GP can not be fixed up.
> > + * If the mm has not been allocated a PASID or ENQCMD has been
> > + * disallowed, the #GP can not be fixed up.
> > */
> > - if (!mm_valid_pasid(current->mm))
> > - return false;
> > -
> > pasid = mm_get_enqcmd_pasid(current->mm);
> > + if (pasid == IOMMU_PASID_INVALID)
> > + return false;
> >
> > /*
> > * Did this thread already have its PASID activated?
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index bc7c7232a43e..a83333651ad0 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -10,6 +10,9 @@
> >
> > #include "iommu-priv.h"
> >
> > +/* Whether pasid is to be allocated from the global PASID space */
> > +#define IOMMU_PASID_GLOBAL_ANY IOMMU_NO_PASID
> > +
> > static DEFINE_MUTEX(iommu_sva_lock);
> > static bool iommu_sva_present;
> > static LIST_HEAD(iommu_sva_mms);
> > @@ -17,10 +20,11 @@ static struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
> > struct mm_struct *mm);
> >
> > /* Allocate a PASID for the mm within range (inclusive) */
> > -static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct device *dev)
> > +static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm,
> > + struct device *dev,
> > + ioasid_t pasid)
> > {
> > struct iommu_mm_data *iommu_mm;
> > - ioasid_t pasid;
> >
> > lockdep_assert_held(&iommu_sva_lock);
> >
> > @@ -30,8 +34,27 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
> > iommu_mm = mm->iommu_mm;
> > /* Is a PASID already associated with this mm? */
> > if (iommu_mm) {
> > + if ((pasid == IOMMU_PASID_GLOBAL_ANY && !iommu_mm->pasid_global) ||
> > + (pasid != IOMMU_PASID_GLOBAL_ANY && iommu_mm->pasid_global))
> > + return ERR_PTR(-EBUSY);
> > +
> > + if (!iommu_mm->pasid_global) {
> > + if (list_empty(&iommu_mm->sva_domains))
> > + iommu_mm->pasid = pasid;
> > +
> > + if (pasid != iommu_mm->pasid) {
> > + /*
> > + * Currently, a process simultaneously doing
> > + * SVA with multiple devices with different
> > + * PASIDs is not supported.
> > + */
>
> I am a bit confused by the change in this helper and the comments above.
>
> Currently, when an mm is bound to a device, it uses a PASID allocated
> from the global pool. That implies that all devices access the
> application's address space with the same PASID. Now we want to extend
> this by allowing the device driver to manage the PASID for SVA, which
> should mean different devices might use different PASIDs to access the
> application's address space. But this does not seem to match the logic
> in this helper.
>
> Perhaps I overlooked something?
>
I think your understanding is correct. In the long run, the limitations in the
comment and also in the commit message should be removed. I left the work to a
later patch as I am focusing on removing the main blocker first, which is that
a process is blocked by another irrelevant process for doing SVA as described
in the commit message. Currently, SVA for a process with different PASIDs will
only be allowed one after another, not simultaneously, and the current users of
`iommu_sva_bind_device()` should not be affected by this patch.
So, this patch should be enough to fix our current main problem. Can we leave
it to a later patch? or do you think we should remove the limitations now
although there is no requirement yet?
Thanks,
Joonwon Kang
> > + return ERR_PTR(-ENOSPC);
> > + }
> > + }
> > +
> > if (iommu_mm->pasid >= dev->iommu->max_pasids)
> > return ERR_PTR(-EOVERFLOW);
> > +
> > return iommu_mm;
> > }