On 2024/7/18 16:27, Tian, Kevin wrote:
From: Liu, Yi L yi.l.liu@intel.com Sent: Friday, June 28, 2024 5:06 PM
@@ -3289,7 +3290,20 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
if (device == last_gdev) break;
ops->remove_dev_pasid(device->dev, pasid, domain);
/* If no old domain, undo the succeeded devices/pasid */if (!old) {ops->remove_dev_pasid(device->dev, pasid, domain);continue;}/** Rollback the succeeded devices/pasid to the old domain.* And it is a driver bug to fail attaching with a previously* good domain.*/if (WARN_ON(old->ops->set_dev_pasid(old, device->dev,pasid, domain)))ops->remove_dev_pasid(device->dev, pasid, domain);I wonder whether @remove_dev_pasid() can be replaced by having blocking_domain support @set_dev_pasid?
how about your thought, @Jason?
+int iommu_replace_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 ||
pasid == IOMMU_NO_PASID)return -EINVAL;- mutex_lock(&group->mutex);
- /*
* The recorded domain is inconsistent with the domain pasid is* actually attached until pasid is attached to the new domain.* This has race condition with the paths that do not hold* group->mutex. E.g. the Page Request forwarding.*/so?
This is added per the below comment. Maybe I should have made it clearer. Due to the order of this xa operations, the domain in the xarray does not match the actual translation structure, but it will become consistent in the end.
https://lore.kernel.org/linux-iommu/20240429135512.GC941030@nvidia.com/
- curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
- if (!curr) {
xa_erase(&group->pasid_array, pasid);ret = -EINVAL;goto out_unlock;- }
- ret = xa_err(curr);
- if (ret)
goto out_unlock;- if (curr == domain)
goto out_unlock;- ret = __iommu_set_group_pasid(domain, group, pasid, curr);
- if (ret)
WARN_ON(domain != xa_store(&group->pasid_array, pasid,curr, GFP_KERNEL));above can follow Jason's suggestion to iommu_group_replace_domain () in Baolu's series, i.e. doing a xa_reserve() first.
yeah, I noticed it. But there is a minor difference. In Baolu's series no need to retrieve the old domain, but this path needs to get it and pass it to set_dev_pasid().