Re: [RFC PATCH 3/3] iommufd/tsm: Add tsm_bind/unbind iommufd ioctls

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, May 30, 2025 at 02:03:00PM +0530, Aneesh Kumar K.V wrote:
> Jason Gunthorpe <jgg@nvidia.com> writes:
> 
> > On Thu, May 29, 2025 at 07:07:56PM +0530, Aneesh Kumar K.V (Arm) wrote:
> >
> >> +static struct mutex *vdev_lock(struct iommufd_vdevice *vdev)
> >> +{
> >> +
> >> +	if (device_lock_interruptible(vdev->dev) != 0)
> >> +		return NULL;
> >> +	return &vdev->dev->mutex;
> >> +}
> >> +DEFINE_FREE(vdev_unlock, struct mutex *, if (_T) mutex_unlock(_T))
> >
> > I know I suggested this, but maybe it would be happier to use a mutex
> > in the viommu?
> >
> > What is the locking model you need for TSM calls here anyhow? Can you
> > concurrently call tsms for vommu creation with bind/unbind or so on?
> >
> 
> Thinking about this more, I guess we likely don’t need a lock here. I
> initially added it to handle vdevice->tsm_bind, but concurrent TSM calls
> are already serialized via tsm_ops_lock.
> 
> Additionally, if tsm_bind is invoked on an already bound TDI, the TSM
> layer handles it gracefully. This suggests that maintaining
> vdevice->tsm_bound is unnecessary.
> 
> Since we're not modifying any vdevice state here, it appears safe to
> remove the vdev_lock() call?

Okay, that's a reasonable answer


> >> +struct iommu_vdevice_id {
> >> +	__u32 size;
> >> +	__u32 vdevice_id;
> >> +} __packed;
> >
> > ???
> > Why is it called vdevice_id?
> > Why is it packed?
> >
> > The struct should be per-ioctl. Does anyone need a TSM specific argument
> > blob for bind?
> 
> For both tsm_bind and tsm_unbind, we need the vdevice id. How do we pass
> that? 

You should have a struct attached to the ioctl, and not packed. Maybe
this is a sign you don't need two ioctls, or maybe you should have two
structs.

What you may really want is a TSM_OPERATION iommufd operation where
bind/unbind are just sub-ops there. It could unify the viommu and
vdevice related TSM ops that will be needed into one ioctl.

I think all the ops will have the same basic format of an id and a
blob of TSM specific information?

Jason