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/b3603fcb79b1036acae10602bffc4855a4b9af80/drivers/iommu/iommu.c#L3376
--
Regards,
Yi Liu
