RE: [PATCH v3 1/7] iommu: Introduce a replace API for device pasid

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?

+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?

• 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.