On Fri, May 30, 2025 at 02:03:00PM +0530, Aneesh Kumar K.V wrote:
> Jason Gunthorpe <jgg@xxxxxxxxxx> 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
