On 2024/4/17 17:19, Tian, Kevin wrote:
From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
Sent: Friday, April 12, 2024 4:15 PM
@@ -4646,6 +4646,10 @@ static int intel_iommu_set_dev_pasid(struct
iommu_domain *domain,
if (context_copied(iommu, info->bus, info->devfn))
return -EBUSY;
+ /* Block old translation */
+ if (old)
+ intel_iommu_remove_dev_pasid(dev, pasid, old);
+
let's talk about one scenario.
pasid#100 is currently attached to domain#1
the user requests to replace pasid#100 to domain#2, which enables
dirty tracking.
this function will return error before blocking the old translation:
if (domain->dirty_ops)
return -EINVAL;
yep. From what I learned from Joao, this check was added due to no actual
usage before SIOV. If only considering vSVM, the domain attached to pasids
won't have the dirty_ops. So I was planning to remove this check when
coming to SIOV series. But let me know if it's already the time to remove
it.
pasid#100 is still attached to domain#1.
then the error unwinding in iommu core tries to attach pasid#100
back to domain#1:
/*
* Rollback the devices/pasid that have attached to the new
* domain. And it is a driver bug to fail attaching with a
* previously good domain.
*/
if (device == last_gdev) {
WARN_ON(old->ops->set_dev_pasid(old, device->dev,
pasid, NULL));
break;
}
but intel iommu driver doesn't expect duplicated attaches e.g.
domain_attach_iommu() will increase the refcnt of the existing
DID but later the user will only call detach once.
good catch!!! This is a problem...Even if the domain->dirty_ops check is
removed, the set_dev_pasid() callback can fail for other reasons.
do we want to force iommu driver to block translation upon
any error in @set_dev_pasid (then rely on the core to recover
it correctly) or tolerate duplicated attaches?
The second one seems better? The first option looks a too heavy especially
considering the atomic requirement in certain scenarios.
--
Regards,
Yi Liu
