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

[Jason Gunthorpe]

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

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

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

-- 
Jason