Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> writes:
> On Mon, 28 Jul 2025 19:21:45 +0530
> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@xxxxxxxxxx> wrote:
>
>> Add operations bind and unbind used to bind a TDI to the secure guest.
>>
>> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@xxxxxxxxxx>
>
> Hi Aneesh,
>
> I'm mostly reading this to get head around it rather than fully review
> at this point.
>
> A few things inline though that I noticed whilst doing so.
>
> Jonathan
>
>> ---
>> drivers/iommu/iommufd/iommufd_private.h | 1 +
>> drivers/iommu/iommufd/main.c | 3 ++
>> drivers/iommu/iommufd/viommu.c | 50 +++++++++++++++++++++++++
>> drivers/vfio/pci/vfio_pci_core.c | 10 +++++
>> include/uapi/linux/iommufd.h | 18 +++++++++
>> 5 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
>> index fce68714c80f..e08186f1d102 100644
>> --- a/drivers/iommu/iommufd/iommufd_private.h
>> +++ b/drivers/iommu/iommufd/iommufd_private.h
>> @@ -697,6 +697,7 @@ void iommufd_vdevice_destroy(struct iommufd_object *obj);
>> void iommufd_vdevice_abort(struct iommufd_object *obj);
>> int iommufd_hw_queue_alloc_ioctl(struct iommufd_ucmd *ucmd);
>> void iommufd_hw_queue_destroy(struct iommufd_object *obj);
>> +int iommufd_vdevice_tsm_op_ioctl(struct iommufd_ucmd *ucmd);
>>
>> static inline struct iommufd_vdevice *
>> iommufd_get_vdevice(struct iommufd_ctx *ictx, u32 id)
>
>> diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
>> index 59f1e1176f7f..c934312e5397 100644
>> --- a/drivers/iommu/iommufd/viommu.c
>> +++ b/drivers/iommu/iommufd/viommu.c
>> @@ -162,6 +162,9 @@ void iommufd_vdevice_abort(struct iommufd_object *obj)
>>
>> lockdep_assert_held(&idev->igroup->lock);
>>
>> +#ifdef CONFIG_TSM
> Can we use stubs for some of this stuff so we don't need ifdefs in as many
> places.
>
>> + tsm_unbind(idev->dev);
>> +#endif
>> if (vdev->destroy)
>> vdev->destroy(vdev);
>> /* xa_cmpxchg is okay to fail if alloc failed xa_cmpxchg previously */
>> @@ -471,3 +474,50 @@ int iommufd_hw_queue_alloc_ioctl(struct iommufd_ucmd *ucmd)
>> iommufd_put_object(ucmd->ictx, &viommu->obj);
>> return rc;
>> }
>> +
>> +#ifdef CONFIG_TSM
>> +int iommufd_vdevice_tsm_op_ioctl(struct iommufd_ucmd *ucmd)
>
> Might want to split this out to a separate c file and use stubs in a header to
> keep the code clean here.
>
>> +{
>> + struct iommu_vdevice_tsm_op *cmd = ucmd->cmd;
>> + struct iommufd_vdevice *vdev;
>> + struct kvm *kvm;
>> + int rc = -ENODEV;
>> +
>> + if (cmd->flags)
>> + return -EOPNOTSUPP;
>> +
>> + 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);
>> +
>> + kvm = vdev->viommu->kvm_filp->private_data;
>> + if (kvm) {
>> + /*
>> + * tsm layer will make take care of parallel calls to tsm_bind/unbind
>
> Wrap comment to say under 80 chars. Or if file goes higher, use a single line
> comment.
>
> tsm layer will take care ...
>
> (stray 'make')
>
>> + */
>> + if (cmd->op == IOMMU_VDEICE_TSM_BIND)
>> + rc = tsm_bind(vdev->idev->dev, kvm, vdev->virt_id);
>> + else if (cmd->op == IOMMU_VDEICE_TSM_UNBIND)
>> + rc = tsm_unbind(vdev->idev->dev);
>> +
>> + if (rc) {
>> + rc = -ENODEV;
>
> If we want to eat an error code coming from elsewhere, maybe a comment on why?
>
>> + goto out_put_vdev;
>> + }
>> + } else {
>> + goto out_put_vdev;
>
> If this always skips the next line, does that imply that line should
> have been under if (kvm)? Maybe this makes more sense in
> later patches - if so ignore this comment.
>
>
>> + }
>> + rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
>> +
>> +out_put_vdev:
>> + iommufd_put_object(ucmd->ictx, &vdev->obj);
>> + return rc;
>> +}
>> +#else /* !CONFIG_TSM */
>> +int iommufd_vdevice_tsm_op_ioctl(struct iommufd_ucmd *ucmd)
>> +{
>> + return -ENODEV;
>> +}
>> +#endif
>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>> index bee3cf3226e9..afdb39c6aefd 100644
>> --- a/drivers/vfio/pci/vfio_pci_core.c
>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>> @@ -694,6 +694,16 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
>> #if IS_ENABLED(CONFIG_EEH)
>> eeh_dev_release(vdev->pdev);
>> #endif
>> +
>> +#if 0
>
> If you really need to do this, add a comment on why if 0
>
>> + /*
>> + * destroy vdevice which involves tsm unbind before we disable pci disable
>> + * A MSE/BME clear will transition the device to error state.
>> + */
>> + if (core_vdev->iommufd_device)
>> + iommufd_device_tombstone_vdevice(core_vdev->iommufd_device);
>> +#endif
>> +
>> vfio_pci_core_disable(vdev);
>>
This is something I’d like to get feedback on. According to the TSM
specification, we’re required to unlock before clearing MSE/BME via
calling `vfio_pci_core_disable(vdev)`.
However, in the current `iommufd` branch, we seem to call
`vdevice_destroy` a bit too late in the sequence to meet this
requirement.
>> mutex_lock(&vdev->igate);
>> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
>> index 9014c61a97d4..8b1fbf1ef25c 100644
>> --- a/include/uapi/linux/iommufd.h
>> +++ b/include/uapi/linux/iommufd.h
>> @@ -57,6 +57,7 @@ enum {
>
>> /**
>> @@ -1127,6 +1128,23 @@ enum iommu_veventq_flag {
>> IOMMU_VEVENTQ_FLAG_LOST_EVENTS = (1U << 0),
>> };
>>
>> +/**
>> + * struct iommu_vdevice_tsm_OP - ioctl(IOMMU_VDEVICE_TSM_OP)
>> + * @size: sizeof(struct iommu_vdevice_tsm_OP)
>
> _op I guess?
>
>> + * @op: Either TSM_BIND or TSM_UNBIMD
>> + * @flags: Must be 0
>> + * @vdevice_id: Object handle for the vDevice. Returned from IOMMU_VDEVICE_ALLOC
>> + */
>> +struct iommu_vdevice_tsm_op {
>> + __u32 size;
>> + __u32 op;
>> + __u32 flags;
>> + __u32 vdevice_id;
>> +};
>> +#define IOMMU_VDEVICE_TSM_OP _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VDEVICE_TSM_OP)
>> +#define IOMMU_VDEICE_TSM_BIND 0x1
>> +#define IOMMU_VDEICE_TSM_UNBIND 0x2
>> +
>> /**
>> * struct iommufd_vevent_header - Virtual Event Header for a vEVENTQ Status
>> * @flags: Combination of enum iommu_veventq_flag
Thanks for the review comments. I'll update the patch with the suggested changes.
-aneesh
