On 2022/8/18 21:33, Jason Gunthorpe wrote:
On Wed, Aug 17, 2022 at 09:20:16AM +0800, Lu Baolu wrote:
+static int __iommu_set_group_pasid(struct iommu_domain *domain,
+ struct iommu_group *group, ioasid_t pasid)
+{
+ struct iommu_domain *ops_domain;
+ struct group_device *device;
+ int ret = 0;
+
+ if (domain == group->blocking_domain)
+ ops_domain = xa_load(&group->pasid_array, pasid);
+ else
+ ops_domain = domain;
This seems weird, why isn't this just always
domain->ops->set_dev_pasid()?
Sure. I will fix this in the next version.
+ if (curr) {
+ ret = xa_err(curr) ? : -EBUSY;
+ goto out_unlock;
+ }
+
+ ret = __iommu_set_group_pasid(domain, group, pasid);
+ if (ret) {
+ __iommu_set_group_pasid(group->blocking_domain, group, pasid);
+ xa_erase(&group->pasid_array, pasid);
I was looking at this trying to figure out why we are having
attach/detach semantics vs set and this error handling seems to be the
reason
Lets add a comment because it is subtle thing:
Setting a PASID to a blocking domain cannot fail, so we can always
safely error unwind a failure to attach a domain back to the original
group configuration of the PASID being unused.
Updated.
+/*
+ * iommu_detach_device_pasid() - Detach the domain from pasid of device
+ * @domain: the iommu domain.
+ * @dev: the attached device.
+ * @pasid: the pasid of the device.
+ *
+ * The @domain must have been attached to @pasid of the @dev with
+ * iommu_attach_device_pasid().
+ */
+void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid)
Don't pass domain here?
It is checked in the function to make sure that the detached domain is
the same one as the previous attached one.
+/*
+ * iommu_get_domain_for_dev_pasid() - Retrieve domain for @pasid of @dev
+ * @dev: the queried device
+ * @pasid: the pasid of the device
+ *
+ * This is a variant of iommu_get_domain_for_dev(). It returns the existing
+ * domain attached to pasid of a device. It's only for internal use of the
+ * IOMMU subsystem. The caller must take care to avoid any possible
+ * use-after-free case.
How exactly does the caller manage that?
"... the returned domain pointer could only be used before detaching
from the device PASID."
+ *
+ * Return: attached domain on success, NULL otherwise.
+ */
+struct iommu_domain *
+iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid)
+{
+ struct iommu_domain *domain;
+ struct iommu_group *group;
+
+ if (!pasid_valid(pasid))
+ return NULL;
Why bother? If the pasid is not valid then it definitely won't be in the xarray.
Removed.
But otherwise this overall thing seems fine to me
Thank you!
Best regards,
baolu