Re: [PATCH v2 2/3] iommufd/tsm: add vdevice TSM bind/unbind ioctl

[Jonathan Cameron]

On Mon,  9 Mar 2026 16:47:03 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@xxxxxxxxxx> wrote:

> Introduce IOMMU_VDEVICE_TSM_OP to allow userspace to issue TSM bind/unbind
> operations for an iommufd vdevice.
> 
> The new ioctl:
> - looks up the vdevice object from vdevice_id
> - resolves the associated KVM VM from the vIOMMU KVM file reference
> - dispatches bind/unbind via tsm_bind()/tsm_unbind()
> 
> Also add common TSM helpers in tsm-core and wire vdevice teardown to unbind
> the device from TSM state.
> 
> This provides iommufd plumbing to bind a TDI to a confidential guest through
> the TSM layer.

> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@xxxxxxxxxx>
Hi Aneesh,

Some superficial code flow suggestions.  I've not gotten
my head around the broader picture yet, so may well come back with
more comments later.

Thanks,
Jonathan

> ---
>  drivers/iommu/iommufd/Makefile          |  2 +
>  drivers/iommu/iommufd/iommufd_private.h |  8 +++
>  drivers/iommu/iommufd/main.c            |  3 ++
>  drivers/iommu/iommufd/tsm.c             | 67 +++++++++++++++++++++++++
>  drivers/iommu/iommufd/viommu.c          |  3 ++
>  drivers/virt/coco/tsm-core.c            | 19 +++++++
>  include/linux/tsm.h                     | 18 +++++++
>  include/uapi/linux/iommufd.h            | 18 +++++++
>  8 files changed, 138 insertions(+)
>  create mode 100644 drivers/iommu/iommufd/tsm.c
> 
> diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile
> index 71d692c9a8f4..431089089ee9 100644
> --- a/drivers/iommu/iommufd/Makefile
> +++ b/drivers/iommu/iommufd/Makefile
> @@ -10,6 +10,8 @@ iommufd-y := \
>  	vfio_compat.o \
>  	viommu.o
>  
> +iommufd-$(CONFIG_TSM) += tsm.o
> +
Probably no blank line here. Style choices in these are a bit random
though so this is kind of a taste thing.

>  iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o
>  
>  obj-$(CONFIG_IOMMUFD) += iommufd.o

> diff --git a/drivers/iommu/iommufd/tsm.c b/drivers/iommu/iommufd/tsm.c
> new file mode 100644
> index 000000000000..401469110752
> --- /dev/null
> +++ b/drivers/iommu/iommufd/tsm.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2026 ARM Ltd.
> + */
> +
> +#include "iommufd_private.h"
> +#include <linux/tsm.h>
> +
> +/**
> + * iommufd_vdevice_tsm_op_ioctl - Handle vdevice TSM operations
> + * @ucmd: user command data for IOMMU_VDEVICE_TSM_OP
> + *
> + * Currently only supports TSM bind/unbind operations
> + * Resolve @iommu_vdevice_tsm_op::vdevice_id to a vdevice and dispatch the
> + * requested bind/unbind operation through the TSM core.
> + *
> + * Return: 0 on success, or a negative error code on failure.
> + */
> +int iommufd_vdevice_tsm_op_ioctl(struct iommufd_ucmd *ucmd)
> +{
> +	int rc;
> +	struct kvm *kvm;
> +	struct iommufd_vdevice *vdev;
> +	struct iommu_vdevice_tsm_op *cmd = ucmd->cmd;
> +
> +	if (cmd->flags)
> +		return -EOPNOTSUPP;
> +
> +	vdev = container_of(iommufd_get_object(ucmd->ictx, cmd->vdevice_id,
> +					       IOMMUFD_OBJ_VDEVICE),
> +			    struct iommufd_vdevice, obj);

I'd be tempted to do something with a helper function to simplify flow.

	obj = iommufd_get_object();...

	if (IS_ERR(obj) //I think it can be?
		return PTR_ERR(obj);

	rc = iommufd_vdevice_do_tsm_op_ioctl(obj, ...) //name could be improved
	iommufd_put_object(ucmd->ictx, &vdev->obj);

	return rc;

Then the helper function can do direct returns on errors given the
iommfd_object is managed in outer function.

> +	if (IS_ERR(vdev))
> +		return PTR_ERR(vdev);
> +
> +	if (!vdev->viommu->kvm_filp) {
> +		rc = -ENODEV;
> +		goto out_put_vdev;
> +	}
> +
> +	kvm = vdev->viommu->kvm_filp->private_data;
> +	if (!kvm) {
> +		rc = -ENODEV;
> +		goto out_put_vdev;
> +	}
> +
> +	/* tsm layer will take care of parallel calls to tsm_bind/unbind */
> +	switch (cmd->op) {
> +	case IOMMU_VDEVICE_TSM_BIND:
> +		rc = tsm_bind(vdev->idev->dev, kvm, vdev->virt_id);
> +		break;
> +	case IOMMU_VDEVICE_TSM_UNBIND:
> +		rc = tsm_unbind(vdev->idev->dev);
> +		break;
> +	default:
> +		rc = -EINVAL;
> +		goto out_put_vdev;
> +	}
> +
> +	if (rc)
> +		goto out_put_vdev;
> +
> +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> +
> +out_put_vdev:
> +	iommufd_put_object(ucmd->ictx, &vdev->obj);
> +	return rc;
> +}

> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index 7f72a154b6b2..9f2a7868021a 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -124,6 +124,24 @@ struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *ops);
>  void tsm_unregister(struct tsm_dev *tsm_dev);
>  struct tsm_dev *find_tsm_dev(int id);
>  struct pci_ide;
> +struct kvm;

Why up here? struct pci_ide is here for the next two calls, so no need
to group with that.

>  int tsm_ide_stream_register(struct pci_ide *ide);
>  void tsm_ide_stream_unregister(struct pci_ide *ide);

Feels like the forwards def belongs here.

> +#ifdef CONFIG_TSM
> +int tsm_bind(struct device *dev, struct kvm *kvm, u64 tdi_id);
> +int tsm_unbind(struct device *dev);
> +
White space is a bit inconsistent. I don't mind seeing some
here, but if so, add some around the ifdef above.

> +#else
> +
> +static inline int tsm_bind(struct device *dev, struct kvm *kvm, u64 tdi_id)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int tsm_unbind(struct device *dev)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #endif /* __TSM_H */