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

[Jason Gunthorpe <jgg@nvidia.com>]

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?

> @@ -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.

> +	if (pasid != IOMMU_NO_PASID && !hwpt->pasid_compat)
> +		return -EINVAL;

This hunks could safely go in the prior patch adding the pasid_compat?

> @@ -417,6 +418,31 @@ iommufd_get_device(struct iommufd_ucmd *ucmd, u32 id)
>  void iommufd_device_destroy(struct iommufd_object *obj);
>  int iommufd_get_hw_info(struct iommufd_ucmd *ucmd);
>  
> +typedef struct iommufd_hw_pagetable *(*attach_fn)(
> +			struct iommufd_device *idev, ioasid_t pasid,
> +			struct iommufd_hw_pagetable *hwpt);
> +
> +int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
> +			       struct iommufd_device *idev,
> +			       ioasid_t pasid);
> +void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
> +				struct iommufd_device *idev,
> +				ioasid_t pasid);
> +int iommufd_hwpt_replace_device(struct iommufd_device *idev,
> +				ioasid_t pasid,
> +				struct iommufd_hw_pagetable *hwpt,
> +				struct iommufd_hw_pagetable *old);
> +
> +int iommufd_device_change_pt(struct iommufd_device *idev, ioasid_t pasid,
> +			     u32 *pt_id, attach_fn do_attach);
> +
> +struct iommufd_hw_pagetable *
> +iommufd_device_pasid_do_attach(struct iommufd_device *idev, ioasid_t pasid,
> +			       struct iommufd_hw_pagetable *hwpt);
> +struct iommufd_hw_pagetable *
> +iommufd_device_pasid_do_replace(struct iommufd_device *idev, ioasid_t pasid,
> +				struct iommufd_hw_pagetable *hwpt);

Ugh is there going to be alot of stuf fin the pasid.c? Maybe it is
easier to just leave the new functions in device.

> +struct iommufd_hw_pagetable *
> +iommufd_device_pasid_do_attach(struct iommufd_device *idev, ioasid_t pasid,
> +			       struct iommufd_hw_pagetable *hwpt)
> +{
> +	void *curr;
> +	int rc;
> +
> +	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?

Jason