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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Jason Gunthorpe <jgg@nvidia.com> writes:

>> [ ... 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?
>


The previous discussion suggested that another kernel subsystem might
need to use this low-level TSM interface, although no concrete example
was identified. In other words, an opaque guest request could be issued
from either userspace or kernel space. 

>
>> [ ... 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 was added specifically for AMD SEV, which requires an IOMMU-side
update to enable DMA.

>
>> + * @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.
>


I will add kernel-doc comments for the operations used by CCA.

We have gone through several iterations to identify the guest
passthrough request facility we need. IIUC, both TDX and CCA give the
hypervisor some control over the request type, while SEV-TIO is more
opaque.


>
>> [ ... 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
>

That is required for SEV-TIO.

-aneesh