| Message ID | 20240328122958.83332-3-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Two enhancements to iommu_at[de]tach_device_pasid() | expand |
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Thursday, March 28, 2024 8:30 PM > > Existing remove_dev_pasid() callbacks of the underlying iommu drivers > get the attached domain from the group->pasid_array. However, the domain > stored in group->pasid_array is not always correct in all scenarios. > A wrong domain may result in failure in remove_dev_pasid() callback. > To avoid such problems, it is more reliable to pass the domain to the > remove_dev_pasid() op. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 3/28/24 8:29 PM, Yi Liu wrote: > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 2e925b5eba53..40dd439307e8 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -578,7 +578,8 @@ struct iommu_ops { > struct iommu_page_response *msg); > > int (*def_domain_type)(struct device *dev); > - void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid); > + void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid, > + struct iommu_domain *domain); Previously, this callback said "Hey, remove any domain associated with this pasid". Now this callback changes to "Hey, please remove *this* domain from the pasid". What the driver should do if it doesn't match the previously attached domain, or the pasid hasn't been attached to any domain? Best regards, baolu
On 2024/4/3 11:04, Baolu Lu wrote: > On 3/28/24 8:29 PM, Yi Liu wrote: >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index 2e925b5eba53..40dd439307e8 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -578,7 +578,8 @@ struct iommu_ops { >> struct iommu_page_response *msg); >> int (*def_domain_type)(struct device *dev); >> - void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid); >> + void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid, >> + struct iommu_domain *domain); > > Previously, this callback said "Hey, remove any domain associated with > this pasid". > > Now this callback changes to "Hey, please remove *this* domain from the > pasid". What the driver should do if it doesn't match the previously > attached domain, or the pasid hasn't been attached to any domain? I think the caller of this callback should know very well whether a pasid has been attached to this domain or not. So the problem you described should not happen at all. Otherwise, it is a bug in the iommu layer. Actually, there is similar concern in the iommu_detach_device_pasid(). The input domain may be different with what iommu layer tracks. If so there is a warn. This means the external callers of this API are buggy. While, I have more faith on iommu layer. :)
On Wed, Apr 03, 2024 at 11:25:35AM +0800, Yi Liu wrote: > On 2024/4/3 11:04, Baolu Lu wrote: > > On 3/28/24 8:29 PM, Yi Liu wrote: > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > > index 2e925b5eba53..40dd439307e8 100644 > > > --- a/include/linux/iommu.h > > > +++ b/include/linux/iommu.h > > > @@ -578,7 +578,8 @@ struct iommu_ops { > > > struct iommu_page_response *msg); > > > int (*def_domain_type)(struct device *dev); > > > - void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid); > > > + void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid, > > > + struct iommu_domain *domain); > > > > Previously, this callback said "Hey, remove any domain associated with > > this pasid". > > > > Now this callback changes to "Hey, please remove *this* domain from the > > pasid". What the driver should do if it doesn't match the previously > > attached domain, or the pasid hasn't been attached to any domain? > > I think the caller of this callback should know very well whether > a pasid has been attached to this domain or not. So the problem > you described should not happen at all. Otherwise, it is a bug in > the iommu layer. > > Actually, there is similar concern in the iommu_detach_device_pasid(). > The input domain may be different with what iommu layer tracks. If > so there is a warn. This means the external callers of this API are > buggy. While, I have more faith on iommu layer. :) Yeah, the iommu layer should obtain the domain from the pasid xarray and every driver today also obtains the domain from the pasid xarray. It is not something different, it is just moving code around. The core code should guarentee the invariant. Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 5ed036225e69..ced041777ec0 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3044,14 +3044,9 @@ static int arm_smmu_def_domain_type(struct device *dev) return 0; } -static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid) +static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) { - struct iommu_domain *domain; - - domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA); - if (WARN_ON(IS_ERR(domain)) || !domain) - return; - arm_smmu_sva_remove_dev_pasid(domain, dev, pasid); } diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 50eb9aed47cc..45c75a8a0ef5 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4587,19 +4587,15 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain, return 0; } -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid) +static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) { struct device_domain_info *info = dev_iommu_priv_get(dev); + struct dmar_domain *dmar_domain = to_dmar_domain(domain); struct dev_pasid_info *curr, *dev_pasid = NULL; struct intel_iommu *iommu = info->iommu; - struct dmar_domain *dmar_domain; - struct iommu_domain *domain; unsigned long flags; - domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0); - if (WARN_ON_ONCE(!domain)) - goto out_tear_down; - /* * The SVA implementation needs to handle its own stuffs like the mm * notification. Before consolidating that code into iommu core, let @@ -4610,7 +4606,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid) goto out_tear_down; } - dmar_domain = to_dmar_domain(domain); spin_lock_irqsave(&dmar_domain->lock, flags); list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) { if (curr->dev == dev && curr->pasid == pasid) { diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index bb3244df525e..c0b615f68a75 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3335,20 +3335,21 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain, if (device == last_gdev) break; - ops->remove_dev_pasid(device->dev, pasid); + ops->remove_dev_pasid(device->dev, pasid, domain); } return ret; } static void __iommu_remove_group_pasid(struct iommu_group *group, - ioasid_t pasid) + ioasid_t pasid, + struct iommu_domain *domain) { struct group_device *device; const struct iommu_ops *ops; for_each_group_device(group, device) { ops = dev_iommu_ops(device->dev); - ops->remove_dev_pasid(device->dev, pasid); + ops->remove_dev_pasid(device->dev, pasid, domain); } } @@ -3409,7 +3410,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev, struct iommu_group *group = dev->iommu_group; mutex_lock(&group->mutex); - __iommu_remove_group_pasid(group, pasid); + __iommu_remove_group_pasid(group, pasid, domain); WARN_ON(xa_erase(&group->pasid_array, pasid) != domain); mutex_unlock(&group->mutex); } diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 2e925b5eba53..40dd439307e8 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -578,7 +578,8 @@ struct iommu_ops { struct iommu_page_response *msg); int (*def_domain_type)(struct device *dev); - void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid); + void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain); const struct iommu_domain_ops *default_domain_ops; unsigned long pgsize_bitmap;
Existing remove_dev_pasid() callbacks of the underlying iommu drivers get the attached domain from the group->pasid_array. However, the domain stored in group->pasid_array is not always correct in all scenarios. A wrong domain may result in failure in remove_dev_pasid() callback. To avoid such problems, it is more reliable to pass the domain to the remove_dev_pasid() op. Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++------- drivers/iommu/intel/iommu.c | 11 +++-------- drivers/iommu/iommu.c | 9 +++++---- include/linux/iommu.h | 3 ++- 4 files changed, 12 insertions(+), 20 deletions(-)