On Tue, Jun 03, 2025 at 10:30:30AM +0530, Aneesh Kumar K.V wrote:
> Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx> writes:
>
> > On Mon, Jun 02, 2025 at 04:38:09PM +0530, Aneesh Kumar K.V wrote:
> >> Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx> writes:
> >>
> >> >> + * 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;
> >> >> +#define IOMMU_VDEVICE_TSM_BIND _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VDEVICE_TSM_BIND)
> >> >> +#define IOMMU_VDEVICE_TSM_UNBIND _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VDEVICE_TSM_UNBIND)
> >> >
> >> > Hello, I see you are talking about the detailed implementation. But
> >> > could we firstly address the confusing whether this TSM Bind/Unbind
> >> > should be a VFIO uAPI or IOMMUFD uAPI?
> >> >
> >> > In this thread [1], I was talking about TSM Bind/Unbind affects VFIO
> >> > behavior so they cannot be iommufd uAPIs which VFIO is not aware of.
> >> > At least TDX Connect cares about this problem now. And the conclusion
> >> > seems to be "have a VFIO_DEVICE_BIND(iommufd vdevice id), then have
> >> > VFIO reach into iommufd".
> >> >
> >> > And some further findings [2] indicate this problem may also exist on
> >> > AMD when p2p is involved.
> >> >
> >> > [1]: https://lore.kernel.org/all/20250515175658.GR382960@xxxxxxxxxx/
> >> > [2]: https://lore.kernel.org/all/aDnXxk46kwrOcl0i@yilunxu-OptiPlex-7050/
> >> >
> >>
> >> Looking at your patch series, I understand the reason you need a vfio
> >> ioctl is to call pci_request_regions_exclusive—is that correct?
> >
> > The immediate reason is to unbind the TDI before unmapping the BAR.
> >
> >>
> >> In another thread, I asked whether this might be better handled by
> >> pci_tsm instead of vfio. I'd be interested in your thoughts on that.
> >>
> >> I also noticed you want to unbind the TDI before unmapping the BAR in
> >> vfio. From what I understand, this should still be possible if we use an
> >> iommufd ioctl.
> >
> > I'm not sure how is that possible.
> >
>
> IIUC, what you need is the below interface
> int iommufd_device_tsm_unbind(struct iommufd_device *idev) so that vfio
> can use vfio_iommufd_tsm_unbind() -> iommufd_device_tsm_unbind(vdev->iommufd_device);
I see. But I'm not sure if it can be a better story than ioctl(VFIO_TSM_BIND).
You want VFIO unaware of TSM bind, e.g. try to hide pci_request/release_region(),
but make VFIO aware of TSM unbind, which seems odd ...
Thanks,
Yilun
>
> The below iommufd changes can get that
>
> static struct mutex *vdev_lock(struct iommufd_vdevice *vdev)
> {
> if (mutex_lock_interruptible(&vdev->mutex) != 0)
> return NULL;
> return &vdev->mutex;
> }
> DEFINE_FREE(vdev_unlock, struct mutex *, if (_T) mutex_unlock(_T))
>
> static struct mutex *idev_lock(struct iommufd_device *idev)
> {
> if (mutex_lock_interruptible(&idev->igroup->lock) != 0)
> return NULL;
> return &idev->igroup->lock;
> }
> DEFINE_FREE(idev_unlock, struct mutex *, if (_T) mutex_unlock(_T))
>
> int iommufd_vdevice_tsm_bind_ioctl(struct iommufd_ucmd *ucmd)
> {
> struct iommu_vdevice_tsm_bind *cmd = ucmd->cmd;
> struct iommufd_vdevice *vdev;
> struct iommufd_device *idev;
> struct mutex *ilock __free(idev_unlock) = NULL;
> struct mutex *vlock __free(vdev_unlock) = NULL;
> struct kvm *kvm;
> int rc = 0;
>
> if (cmd->flags)
> return -EOPNOTSUPP;
>
> idev = iommufd_get_device(ucmd, cmd->dev_id);
> if (IS_ERR(idev))
> return PTR_ERR(idev);
>
> vdev = container_of(iommufd_get_object(ucmd->ictx, cmd->vdevice_id,
> IOMMUFD_OBJ_VDEVICE),
> struct iommufd_vdevice, obj);
> if (IS_ERR(vdev)) {
> rc = PTR_ERR(vdev);
> goto out_put_idev;
> }
>
> ilock = idev_lock(idev);
> if (!ilock) {
> rc = -EINTR;
> goto out_put_vdev;
> }
>
> if (idev->vdev) {
> /* if it is already bound */
> rc = -EINVAL;
> goto out_put_vdev;
> }
>
> vlock = vdev_lock(vdev);
> if (!vlock) {
> rc = -EINTR;
> goto out_put_vdev;
> }
>
> if (WARN_ON(vdev->idev)) {
> rc = -EINVAL;
> goto out_put_vdev;
> }
>
> kvm = vdev->viommu->kvm_filp->private_data;
> if (kvm) {
> /*
> * tsm layer will make take care of parallel calls to tsm_bind/unbind
> */
> rc = tsm_bind(vdev->dev, kvm, vdev->id);
> if (rc) {
> rc = -ENODEV;
> goto out_put_vdev;
> }
> } else {
> rc = -ENODEV;
> goto out_put_vdev;
> }
> idev->vdev = vdev;
> vdev->idev = idev;
> rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
>
> out_put_idev:
> iommufd_put_object(ucmd->ictx, &idev->obj);
> out_put_vdev:
> iommufd_put_object(ucmd->ictx, &vdev->obj);
> return rc;
> }
>
> static int iommufd_vdevice_tsm_unbind(struct iommufd_vdevice *vdev)
> {
> int rc = -EINVAL;
> struct mutex *lock __free(vdev_unlock) = vdev_lock(vdev);
> if (!lock)
> return -EINTR;
>
> if (!vdev->idev) {
> tsm_unbind(vdev->dev);
> vdev->idev = NULL;
> rc = 0;
> }
> return rc;
> }
>
> /**
> * iommufd_device_tsm_unbind - Move a device out of TSM bind state
> * @idev: device to detach
> *
> * Undo iommufd_device_tsm_bind(). This removes all Confidential Computing
> * configurations, Once this completes the device is unlocked (TDISP
> * CONFIG_UNLOCKED).
> */
> int iommufd_device_tsm_unbind(struct iommufd_device *idev)
> {
> struct mutex *lock __free(idev_unlock) = idev_lock(idev);
> if (!lock)
> return -EINTR;
>
> if (!idev->vdev)
> return -EINVAL;
>
> iommufd_vdevice_tsm_unbind(idev->vdev);
> idev->vdev = NULL;
> return 0;
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_device_tsm_unbind, "IOMMUFD");
>
> int iommufd_vdevice_tsm_unbind_ioctl(struct iommufd_ucmd *ucmd)
> {
> struct iommu_vdevice_tsm_unbind *cmd = ucmd->cmd;
> struct iommufd_vdevice *vdev;
> int rc = 0;
>
> vdev = container_of(iommufd_get_object(ucmd->ictx, cmd->vdevice_id,
> IOMMUFD_OBJ_VDEVICE),
> struct iommufd_vdevice, obj);
> if (IS_ERR(vdev))
> return PTR_ERR(vdev);
>
> rc = iommufd_device_tsm_unbind(vdev->idev);
> if (rc) {
> rc = -ENODEV;
> goto out_put_vdev;
> }
> rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
>
> out_put_vdev:
> iommufd_put_object(ucmd->ictx, &vdev->obj);
> return rc;
> }
>
