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