| Message ID | 20241104132033.14027-2-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support attaching PASID to the blocked_domain | expand |
On Mon, Nov 04, 2024 at 05:20:27AM -0800, Yi Liu wrote: > driver should implement both set_dev_pasid and remove_dev_pasid op, otherwise > it is a problem how to detach pasid. In reality, it is impossible that an > iommu driver implements set_dev_pasid() but no remove_dev_pasid() op. However, > it is better to check it. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/iommu/iommu.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) I was wondering if we really needed this, but it does make the patches a bit easier to understand Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, November 5, 2024 11:42 PM > > On Mon, Nov 04, 2024 at 05:20:27AM -0800, Yi Liu wrote: > > driver should implement both set_dev_pasid and remove_dev_pasid op, > otherwise > > it is a problem how to detach pasid. In reality, it is impossible that an > > iommu driver implements set_dev_pasid() but no remove_dev_pasid() op. > However, > > it is better to check it. > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > --- > > drivers/iommu/iommu.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > I was wondering if we really needed this, but it does make the patches > a bit easier to understand > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > me too. btw when Vasant's series introduces the PASID flag to domain above checks can be moved to the domain alloc time then here just needs to check whether the domain allows pasid attach. for now, Reviewed-by: Kevin Tian <kevin.tian@intel.com> and a nit - there is another reference to dev_iommu_ops in this function: if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner || could be replaced with ops too.
On 2024/11/7 17:33, Tian, Kevin wrote: >> From: Jason Gunthorpe <jgg@nvidia.com> >> Sent: Tuesday, November 5, 2024 11:42 PM >> >> On Mon, Nov 04, 2024 at 05:20:27AM -0800, Yi Liu wrote: >>> driver should implement both set_dev_pasid and remove_dev_pasid op, >> otherwise >>> it is a problem how to detach pasid. In reality, it is impossible that an >>> iommu driver implements set_dev_pasid() but no remove_dev_pasid() op. >> However, >>> it is better to check it. >>> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> --- >>> drivers/iommu/iommu.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> I was wondering if we really needed this, but it does make the patches >> a bit easier to understand >> >> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> >> > > me too. btw when Vasant's series introduces the PASID flag to > domain above checks can be moved to the domain alloc time > then here just needs to check whether the domain allows > pasid attach. hmmm. Are we sure all the caller of iommu_attach_device_pasid() will allocate domain with this flag? Besides iommufd, idxd driver would also call iommu_attach_device_pasid(), and it uses the DMA domain of the device. I suppose this domain is allocated with the pasid flag. Given that Vasant's series has been queued and this series is based on that. It might make sense to drop this patch. If the pasid capable domain can be allocated successfully, the iommu driver already indicates it has the ability to support set_dev_pasid and the undo op. > for now, > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > and a nit - there is another reference to dev_iommu_ops in this > function: > > if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner || > > could be replaced with ops too. If we decide to keep this patch, I'll add it.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index ea6b4a96186d..866559bbc4e4 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3454,11 +3454,13 @@ int iommu_attach_device_pasid(struct iommu_domain *domain, struct iommu_attach_handle *handle) { /* Caller must be a probed driver on dev */ + const struct iommu_ops *ops = dev_iommu_ops(dev); struct iommu_group *group = dev->iommu_group; struct group_device *device; int ret; - if (!domain->ops->set_dev_pasid) + if (!domain->ops->set_dev_pasid || + !ops->remove_dev_pasid) return -EOPNOTSUPP; if (!group)
driver should implement both set_dev_pasid and remove_dev_pasid op, otherwise it is a problem how to detach pasid. In reality, it is impossible that an iommu driver implements set_dev_pasid() but no remove_dev_pasid() op. However, it is better to check it. Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/iommu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)