| Message ID | 20241104131842.13303-11-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make set_dev_pasid op supporting domain replacement | expand |
On 11/4/24 21:18, Yi Liu wrote: > There is no known usage that will attach SVA domain or detach SVA domain > by replacing PASID to or from SVA domain. It is supposed to use the > iommu_sva_{un}bind_device() which invoke the iommu_{at|de}tach_device_pasid(). > So Intel iommu driver decides to fail the domain replacement if the old > domain or new domain is SVA type. I would suggest dropping this patch. The iommu driver now supports switching SVA domains to and from other types of domains. The current limitation comes from the iommu core, where the SVA interface doesn't use replace. Looking forward, iommu_detach_device_pasid() will be converted to switch an SVA domain to a blocking domain. Once this is implemented, perhaps the check and failure scenario will no longer be relevant. -- baolu
On 2024/11/5 11:30, Baolu Lu wrote: > On 11/4/24 21:18, Yi Liu wrote: >> There is no known usage that will attach SVA domain or detach SVA domain >> by replacing PASID to or from SVA domain. It is supposed to use the >> iommu_sva_{un}bind_device() which invoke the >> iommu_{at|de}tach_device_pasid(). >> So Intel iommu driver decides to fail the domain replacement if the old >> domain or new domain is SVA type. > > I would suggest dropping this patch. > > The iommu driver now supports switching SVA domains to and from other > types of domains. The current limitation comes from the iommu core, > where the SVA interface doesn't use replace. I also noticed other iommu drivers (like ARM SMMUv3 and AMD iommu) would support SVA replacement. So may we also make intel_svm_set_dev_pasid() be able to handle the case in which @old==non-null? At first, it looks to be dead code when SVA interface does not use replace. But this may make all drivers aligned. > Looking forward, iommu_detach_device_pasid() will be converted to switch > an SVA domain to a blocking domain. Once this is implemented, perhaps > the check and failure scenario will no longer be relevant. you are right.
On 11/5/24 13:30, Yi Liu wrote: > On 2024/11/5 11:30, Baolu Lu wrote: >> On 11/4/24 21:18, Yi Liu wrote: >>> There is no known usage that will attach SVA domain or detach SVA domain >>> by replacing PASID to or from SVA domain. It is supposed to use the >>> iommu_sva_{un}bind_device() which invoke the iommu_{at|de} >>> tach_device_pasid(). >>> So Intel iommu driver decides to fail the domain replacement if the old >>> domain or new domain is SVA type. >> >> I would suggest dropping this patch. >> >> The iommu driver now supports switching SVA domains to and from other >> types of domains. The current limitation comes from the iommu core, >> where the SVA interface doesn't use replace. > > I also noticed other iommu drivers (like ARM SMMUv3 and AMD iommu) would > support SVA replacement. So may we also make intel_svm_set_dev_pasid() be > able to handle the case in which @old==non-null? At first, it looks to be > dead code when SVA interface does not use replace. But this may make all > drivers aligned. yeah, that works for me.
On Tue, Nov 05, 2024 at 11:30:25AM +0800, Baolu Lu wrote: > On 11/4/24 21:18, Yi Liu wrote: > > There is no known usage that will attach SVA domain or detach SVA domain > > by replacing PASID to or from SVA domain. It is supposed to use the > > iommu_sva_{un}bind_device() which invoke the iommu_{at|de}tach_device_pasid(). > > So Intel iommu driver decides to fail the domain replacement if the old > > domain or new domain is SVA type. > > I would suggest dropping this patch. Me too Drivers should not make assumptions like this, the driver facing API is clear, set_dev_pasid() is supposed to make the pasid domain the translation for the pasid and replace whatever happened to be there. Ideally hitlessly. Good driver structure should not require caring what used to be attached to the PASID. Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, November 5, 2024 10:44 PM > > On Tue, Nov 05, 2024 at 11:30:25AM +0800, Baolu Lu wrote: > > On 11/4/24 21:18, Yi Liu wrote: > > > There is no known usage that will attach SVA domain or detach SVA > domain > > > by replacing PASID to or from SVA domain. It is supposed to use the > > > iommu_sva_{un}bind_device() which invoke the > iommu_{at|de}tach_device_pasid(). > > > So Intel iommu driver decides to fail the domain replacement if the old > > > domain or new domain is SVA type. > > > > I would suggest dropping this patch. > > Me too > > Drivers should not make assumptions like this, the driver facing API > is clear, set_dev_pasid() is supposed to make the pasid domain the > translation for the pasid and replace whatever happened to be there. > Ideally hitlessly. > > Good driver structure should not require caring what used to be > attached to the PASID. > Agree
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 0e0e9eb5188a..7e82b3a4bba7 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4352,6 +4352,10 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, if (WARN_ON_ONCE(!(domain->type & __IOMMU_DOMAIN_PAGING))) return -EINVAL; + /* No SVA domain replacement usage so far */ + if (old && old->type == IOMMU_DOMAIN_SVA) + return -EOPNOTSUPP; + if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) return -EOPNOTSUPP; @@ -4620,6 +4624,10 @@ static int identity_domain_set_dev_pasid(struct iommu_domain *domain, struct intel_iommu *iommu = info->iommu; int ret; + /* No SVA domain replacement usage so far */ + if (old && old->type == IOMMU_DOMAIN_SVA) + return -EOPNOTSUPP; + if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) return -EOPNOTSUPP; diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c index 925afca7529e..06404716ad37 100644 --- a/drivers/iommu/intel/svm.c +++ b/drivers/iommu/intel/svm.c @@ -207,6 +207,9 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain, unsigned long sflags; int ret = 0; + if (old) + return -EOPNOTSUPP; + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); if (IS_ERR(dev_pasid)) return PTR_ERR(dev_pasid);
There is no known usage that will attach SVA domain or detach SVA domain by replacing PASID to or from SVA domain. It is supposed to use the iommu_sva_{un}bind_device() which invoke the iommu_{at|de}tach_device_pasid(). So Intel iommu driver decides to fail the domain replacement if the old domain or new domain is SVA type. Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/intel/iommu.c | 8 ++++++++ drivers/iommu/intel/svm.c | 3 +++ 2 files changed, 11 insertions(+)