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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

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?

>> +/**
>> + * struct iommu_vdevice_id - ioctl(IOMMU_VDEVICE_TSM_BIND/UNBIND)
>> + * @size: sizeof(struct iommu_vdevice_id)
>> + * @vdevice_id: Object handle for the vDevice. Returned from IOMMU_VDEVICE_ALLOC
>> + */
>> +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?