On 2024/3/19 00:52, Jason Gunthorpe wrote:
On Wed, Mar 13, 2024 at 04:11:41PM +0800, Yi Liu wrote:
yes. how about your opinion? @Jason. I noticed the set_dev_pasid callback and pasid_array update is under the group->lock, so update it should be fine to adjust the order to update pasid_array after set_dev_pasid returns.
Yes, it makes some sense
But, also I would like it very much if we just have the core pass in the actual old domain as a an addition function argument.
ok, this works too. For normal attach, just pass in a NULL old domain.
I think we have some small mistakes in multi-device group error unwinding for remove because the global xarray can't isn't actually going to be correct in all scenarios.
do you mean the __iommu_remove_group_pasid() call in the below function? Currently, it is called when __iommu_set_group_pasid() failed. However, __iommu_set_group_pasid() may need to do remove itself when error happens, so the helper can be more self-contained. Or you mean something else?
int iommu_attach_device_pasid(struct iommu_domain *domain, struct device *dev, ioasid_t pasid) { /* Caller must be a probed driver on dev */ struct iommu_group *group = dev->iommu_group; void *curr; int ret;
if (!domain->ops->set_dev_pasid) return -EOPNOTSUPP;
if (!group) return -ENODEV;
if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner) return -EINVAL;
mutex_lock(&group->mutex); curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL); if (curr) { ret = xa_err(curr) ? : -EBUSY; goto out_unlock; }
ret = __iommu_set_group_pasid(domain, group, pasid); if (ret) { __iommu_remove_group_pasid(group, pasid); xa_erase(&group->pasid_array, pasid); } out_unlock: mutex_unlock(&group->mutex); return ret; } EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
https://github.com/torvalds/linux/blob/b3603fcb79b1036acae10602bffc4855a4b9a...