| Message ID | 20240412081516.31168-12-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommufd support pasid attach/replace | expand |
> From: Liu, Yi L <yi.l.liu@intel.com> > 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; 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. 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? Thanks Kevin
On 2024/4/17 17:19, Tian, Kevin wrote: >> From: Liu, Yi L <yi.l.liu@intel.com> >> 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.
On Wed, Apr 17, 2024 at 05:35:47PM +0800, Yi Liu wrote: > > 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. On set_dev_pasid() failure the driver should make no change to the translation. It is the only sane answer. Jason
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index fff7dea012a7..9e79ffdd47db 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -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); + ret = prepare_domain_attach_device(domain, dev); if (ret) return ret;
Existing intel_iommu_set_dev_pasid() does not support domain replacement. However, iommu layer requires set_dev_pasid() to handle domain replacement. Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/intel/iommu.c | 4 ++++ 1 file changed, 4 insertions(+)