Re: [RFC PATCH v6 09/11] iommufd: Add the vdevice TSM request ioctl

[Alexey Kardashevskiy]

On 25/9/26 05:41, Jason Gunthorpe wrote:
> > [ ... 95 lines skipped ... ]
> > +static bool iommufd_vdevice_tsm_req_arch_valid(u32 tvm_arch)
> > +{
> > +	switch (tvm_arch) {
> > +	case IOMMU_VDEVICE_TSM_TVM_ARCH_CCA:
> > +	case IOMMU_VDEVICE_TSM_TVM_ARCH_SEV:
> > +	case IOMMU_VDEVICE_TSM_TVM_ARCH_TDX:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
>
> Generally speaking we should not do things like this, the viommu
> object should declare what it supports if we want to have validation
> on the iommufd side.
>
> > [ ... 140 lines skipped ... ]
> > +#ifdef CONFIG_TSM
> > +/**
> > + * struct tsm_guest_req_info - parameters for a guest-initiated TSM request
> > + * @op: operation for the guest-initiated request
> > + * @tvm_arch: guest TVM architecture
> > + * @req: request data buffer filled by guest
> > + * @req_len: the size of @req filled by guest
> > + * @resp: response data buffer filled by host
> > + * @resp_len: the size of @resp buffer filled by guest
> > + */
> > +struct tsm_guest_req_info {
> > +	enum iommu_vdevice_tsm_guest_req_op op;
> > +	enum iommu_vdevice_tsm_guest_tvm_arch tvm_arch;
> > +	sockptr_t req;
> > +	size_t req_len;
> > +	sockptr_t resp;
> > +	size_t resp_len;
> > +};
>
> Why is this struct in tsm land?
>
> I'm not seeing why the iommufd interface should be locked to tsm, as I
> said other viommus need this kind of command channel too. Can't we
> have a general one?
>
> Why would it ever not be tied to userspace pointers? I don't want
> an in kernel user ever using this kind of struct?


This mimics the TSM's pci_tsm_guest_req() API which it is a kernel component we did not want to hardcode to use userspace pointers as it does not have uAPI. Makes no sense in tsm_guest_req_info though.


> > [ ... 34 lines skipped ... ]
> > +/**
> > + * enum iommu_vdevice_tsm_guest_req_op - operation for guest TSM requests
> > + * @TSM_REQ_VALIDATE_MMIO: Validate MMIO for the TDI
> > + * @TSM_REQ_SET_TDI_STATE: Set TDI state
> > + * @TSM_REQ_SEV_ENABLE_DMA: Enable SEV DMA
>
> That seems wrong.. The hypervisor should not have control over T=1
> DMA.

This is a guest request to the PSP which does control T=1, the HV is forwarding it and does some extra stuff while there. Probably could be decoupled and go to a separate ioctl().

The HV does not control DMA, it is for configuring DTE in a specific way - the domainid becomes reserved (and its sDTE counterpart starts working) and some other DTE bits have to match whatever the PSP writes to sDTE ("I" == IOTLB enable) and probably more.

And then, yeah, "something something RMP" == cut 2MB mappings if corresponding RMP is not 2MB.

Thanks,

> > + * @TSM_REQ_SEV_DISABLE_DMA: Disable SEV DMA
> > + * @TSM_REQ_READ_OBJECT: Read a TSM object
> > + * @TSM_REQ_REGEN_OBJECT: Regenerate a TSM object
> > + * @TSM_REQ_OBJECT_INFO: Read TSM object information
> > + */
>
> These operations will need alot more commentary. Use kdocs inside the
> enum below to make that easier.
>
> > [ ... 10 lines skipped ... ]
> > +/**
> > + * struct iommu_vdevice_tsm_req - ioctl(IOMMU_VDEVICE_TSM_REQ)
> > + * @size: sizeof(struct iommu_vdevice_tsm_req)
> > + * @vdevice_id: vDevice ID the guest request is for
> > + * @op: One of enum iommu_vdevice_tsm_guest_req_op
> > + * @tvm_arch: One of enum iommu_vdevice_tsm_guest_tvm_arch
> > + * @req_len: Size in bytes of the input payload at @req_uptr
> > + * @resp_len: Size in bytes of the output buffer at @resp_uptr
> > + * @req_uptr: Userspace pointer to the guest-provided request payload
> > + * @resp_uptr: Userspace pointer to the guest response buffer
> > + * @tsm_code: TSM-specific result code returned by the TSM implementation
> > + *
> > + * Forward a TSM request to the TSM bound vDevice. This is intended for
> > + * guest TSM/TDISP message transport where the host kernel only marshals
> > + * bytes between userspace and the TSM implementation.
> > + *
> > + * The request operation is guest initiated. The TSM backend validates
> > + * @tvm_arch against its bound TVM architecture assumptions.
> > + *
> > + * The request payload is read from @req_uptr/@req_len. If a response is
> > + * expected, userspace provides @resp_uptr/@resp_len as writable storage for
> > + * response bytes returned by the TSM path.
> > + *
> > + * The ioctl is only suitable for commands and results that the host kernel
> > + * has no use, the host is only facilitating guest to TSM communication.
> > + */
> > +struct iommu_vdevice_tsm_req {
> > +	__u32 size;
> > +	__u32 vdevice_id;
> > +	__u32 op;
> > +	__u32 tvm_arch;
> > +	__u32 req_len;
> > +	__u32 resp_len;
> > +	__aligned_u64 req_uptr;
> > +	__aligned_u64 resp_uptr;
> > +	__aligned_u64 tsm_code;
>
> out_tsm_code

--
Alexey