Re: [PATCH v7 10/16] iommufd: Add the vdevice TSM request ioctl

[Jason Gunthorpe]

> [ ... 300 lines skipped ... ]
> +/**
> + * enum iommu_vdevice_tsm_guest_req_op - Guest TSM request operations
> + * @TSM_REQ_VALIDATE_MMIO: Validate and map a guest MMIO range for the trusted
> + *                         device.
> + * @TSM_REQ_SET_TDI_STATE: Transition the trusted device to the unlocked,
> + *                         locked, or running state.
> + * @TSM_REQ_SEV_ENABLE_DMA: Enable DMA for an SEV device.
> + * @TSM_REQ_SEV_DISABLE_DMA: Disable DMA for an SEV device.
> + * @TSM_REQ_READ_OBJECT: Read bytes from a device object, such as a certificate,
> + *                       measurement, or interface report.
> + * @TSM_REQ_REGEN_OBJECT: Regenerate a device object, such as a measurement or
> + *                        interface report.
> + * @TSM_REQ_OBJECT_INFO: Query the size of a device object.
> + * @TSM_REQ_MAX: One past the last request operation; not a command.
> + *
> + * The request payload and response format depend on the TVM architecture.
> + */
> +enum iommu_vdevice_tsm_guest_req_op {
> +	TSM_REQ_VALIDATE_MMIO = 1,
> +	TSM_REQ_SET_TDI_STATE,
> +	TSM_REQ_SEV_ENABLE_DMA,
> +	TSM_REQ_SEV_DISABLE_DMA,
> +	TSM_REQ_READ_OBJECT,
> +	TSM_REQ_REGEN_OBJECT,
> +	TSM_REQ_OBJECT_INFO,
> +	TSM_REQ_MAX,
> +};

This is still no where close to being well defined enough.

Everything here must tie back to defined spec references, listed in
the comments.

I cannot guess what data is passed over any of these APIs, how things
like "REGEN_OBJECT" select which object to regen, or what the format
of those objects even is. The formats of req/resp are never specified.

The comment for this should probably be *enormous*. Every arch should
explain what they do on every one of these ops, and it should be
identical if it is structured like this.

I have a strong guess almost none of this is actually general, but
lets find out.

You can also put enum kdocs directly before each enum which helps alot
when they get as big as this stuff has to get.

My guesses with no information
1) VALIDATE_MMIO why would the hypervisor ever need this? Guest
   validates MMIO through secure communication to the secure world.
2) TDI_STATE I thought at least RMM did this with an RSI?
3) ENABLE/DISABLE DMA, see #1, must not be done by hypervisor
   See my reply to Alexey:
   https://lore.kernel.org/linux-iommu/20261022123058.GC3008132@xxxxxxxxxx/
4) READ/REGEN/INFO, what are the objects? What format are they in? How
   do you select them?
   Does this overlap with attestation subsystem? Should attestation
   subsystem be used to access the SPDM related data?

Update the comment to explain all this, don't need to answer here.

We really need to do these defines along with implementations, that
probably will settle alot of the questions above.

> +
> +/**
> + * 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, excluding TSM_REQ_MAX
> + * @tvm_arch: One of enum iommu_vdevice_tsm_guest_tvm_arch
> + * @req_len: Size in bytes of the input payload at @req_uptr, at most INT_MAX
> + * @resp_len: Size in bytes of the output buffer at @resp_uptr, at most INT_MAX
> + * @req_uptr: Userspace pointer to the guest-provided request payload
> + * @resp_uptr: Userspace pointer to the guest response buffer
> + * @out_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 vDEVICE implementation
> + * declares the TVM architecture and operations it accepts; iommufd checks
> + * both before forwarding the request.
> + *
> + * 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 out_tsm_code;
> +};

Should the viommu_id be passed in as well, or we know it implicitly
from the vdevice_id?

I think this is probably a fine generic API, it should probably not be
"TSM request" so that the other viommu people can reuse the transport.

I would probably try to stick to exactly like the other APIs do with a
single enum. Two enums for op/arch is saying already that the ops are
not well defined.

So maybe strip this patch out to have no tsm stuff, define no ops and
just give a command transport into the viommu driver?

-- 
Jason