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

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

Xu Yilun <yilun.xu@linux.intel.com> writes:

> On Mon, Jun 02, 2025 at 04:38:09PM +0530, Aneesh Kumar K.V wrote:
>> Xu Yilun <yilun.xu@linux.intel.com> 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@nvidia.com/
>> > [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);

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;
}