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

[Alexey Kardashevskiy <aik@amd.com>]

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