Re: [PATCH v8 06/12] iommufd: Support pasid attach/replace

[Jason Gunthorpe <jgg@nvidia.com>]

On Sat, Mar 01, 2025 at 07:44:26PM +0800, Yi Liu wrote:
> On 2025/2/28 23:32, Jason Gunthorpe wrote:
> > On Wed, Feb 26, 2025 at 03:40:26AM -0800, Yi Liu wrote:
> > > This introduces three APIs for device drivers to manage pasid attach/
> > > replace/detach.
> > > 
> > >      int iommufd_device_pasid_attach(struct iommufd_device *idev,
> > > 				    ioasid_t pasid, u32 *pt_id);
> > >      int iommufd_device_pasid_replace(struct iommufd_device *idev,
> > > 				     ioasid_t pasid, u32 *pt_id);
> > >      void iommufd_device_pasid_detach(struct iommufd_device *idev,
> > > 				     ioasid_t pasid);
> > > 
> > > The pasid operations share underlying attach/replace/detach infrastructure
> > > with the device operations, but still have some different implications:
> > 
> > You don't want to just add a PASID to the existing APIs and keep with
> > PASID=0 means no pasid?
> 
> do you mean extending the iommufd_device_attach/detach/replace() APIs to
> support a @pasid? Sounds ok to me. VFIO side reuses ioctl for rid attach
> and pasid attach as well.

Yes

> > > @@ -136,6 +136,7 @@ void iommufd_device_destroy(struct iommufd_object *obj)
> > >   	struct iommufd_device *idev =
> > >   		container_of(obj, struct iommufd_device, obj);
> > > +	WARN_ON(!xa_empty(&idev->pasid_hwpts));
> > 
> > Should this be in the igroup? That's what the core code does, so some
> > of the protections you have here won't match the core's version if we
> > ever see a multi-device pasid capable group.
> 
> conceptually, yes. But I had an impression that pasid is only supported on
> singleton groups. Seems this does not stand since the SVA path dropped the
> device count in below commit. If so, indeed iommufd should add pasid_array
> per igroup.

I would keep it per igroup to match the code, even though we do expect
singleton groups.

> https://lore.kernel.org/linux-iommu/20221031005917.45690-10-baolu.lu@linux.intel.com/

PASID support is limited to single device groups directly currently,
so the SVA stuff was duplicate.

> > > +	mutex_lock(&idev->igroup->lock);
> > > +	curr = xa_cmpxchg(&idev->pasid_hwpts, pasid, NULL, hwpt, GFP_KERNEL);
> > > +	if (curr) {
> > 
> > I guess you could do the same trick you wanted in the core where the
> > xarray in the group stores the normal domain too, then I think these
> > special functions just go away since the xa tests are folded into the
> > normal functions in place of their normal domain tests?
> 
> do you mean storing the igroup->hwpt in the xarray?

Yes

That would be appealing if it avoids all the code duplication

Jason