Re: [PATCH v3 12/13] PCI/TSM: support TDI related operations for host TSM driver

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, May 27, 2025 at 07:56:09PM +0530, Aneesh Kumar K.V wrote:
> Jason Gunthorpe <jgg@nvidia.com> writes:
> 
> > On Tue, May 27, 2025 at 05:18:01PM +0530, Aneesh Kumar K.V wrote:
> >> > yeah, I guess, there is a couple of places like this
> >> >
> >> > git grep pci_dev drivers/iommu/iommufd/
> >> >
> >> > drivers/iommu/iommufd/device.c:                 struct pci_dev *pdev = to_pci_dev(idev->dev);
> >> > drivers/iommu/iommufd/eventq.c:         struct pci_dev *pdev = to_pci_dev(dev);
> >> >
> >> > Although I do not see any compelling reason to have pci_dev in the TSM API, struct device should just work and not spill any PCI details to IOMMUFD but whatever... Thanks,
> >> 
> >> Getting the kvm reference is tricky here.
> >
> > The KVM will come from the viommu object, passed in by userspace that
> > is the plan at least.. If you are not presenting a viommu to the guest
> > then I imagine we would still have some kind of NOP viommu object..
> 
> I assume you are not suggesting using IOMMU_VIOMMU_ALLOC? That would
> break the ABI, which we need to maintain.

Yes I am, what ABI are you talking about? CC is all new.

> Instead, my approach uses VFIO_DEVICE_BIND_IOMMUFD to associate the KVM
> context. The vfio device file descriptor had already been linked to the
> KVM instance via KVM_DEV_VFIO_FILE_ADD.
> 
> Through VFIO_DEVICE_BIND_IOMMUFD, we inherit the necessary KVM details
> and pass them along to iommufd_device, and subsequently to
> iommufd_vdevice, using IOMMU_VDEVICE_ALLOC.

It is not OK, we want this in the viommu not the device for a bunch of
other reasons. I don't want two copies of the KVM running around
inside iommfd..

> >> +	if (rc) {
> >> +		rc = -ENODEV;
> >> +		goto out_put_vdev;
> >> +	}
> >> +
> >> +	/* locking? */
> >> +	vdev->tsm_bound = true;
> >> +	refcount_inc(&vdev->obj.users);
> >
> > This refcount isn't going to work, it will make an error close()
> > crash..
> >
> > You need to auto-unbind on destruction I think.
> 
> Can you elaborate on that? if vdevice is tsm_bound,
> iommufd_vdevice_destroy() do call tsm_unbind in the changes I shared.

You are driving it from the vfio side? Then you don't need the
refcount at all here because the vfio facing APIs already take one.

Jason