Re: [PATCH 2/7] PCI/TSM: Add pci_tsm_guest_req() for managing TDIs

[<dan.j.williams@intel.com>]

Alexey Kardashevskiy wrote:
> 
> 
> On 27/8/25 13:52, Dan Williams wrote:
> > A PCIe device function interface assigned to a TVM is a TEE Device
> > Interface (TDI). A TDI instantiated by pci_tsm_bind() needs additional
> > steps to be accepted by a TVM and transitioned to the RUN state.
> > 
> > pci_tsm_guest_req() is a channel for the guest to request TDISP collateral,
> > like Device Interface Reports, and effect TDISP state changes, like
> > LOCKED->RUN transititions. Similar to IDE establishment and pci_tsm_bind(),
> 
> s/transititions/transitions/

Thanks, not sure why checkpatch spell check sometimes misses words.

> 
> > these are long running operations involving SPDM message passing via the
> > DOE mailbox, i.e. another 'struct pci_tsm_link_ops' operation.
> > 
> > The path for a guest to invoke pci_tsm_guest_request() is either via a kvm
> > handle_exit() or an ioctl() when an exit reason is serviced by a userspace
> > VMM.
> > 
> > Co-developed-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >   drivers/pci/tsm.c       | 60 +++++++++++++++++++++++++++++++++++++++++
> >   include/linux/pci-tsm.h | 55 +++++++++++++++++++++++++++++++++++--
> >   2 files changed, 113 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> > index 302a974f3632..3143558373e3 100644
> > --- a/drivers/pci/tsm.c
> > +++ b/drivers/pci/tsm.c
> > @@ -338,6 +338,66 @@ int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u32 tdi_id)
> >   }
> >   EXPORT_SYMBOL_GPL(pci_tsm_bind);
> >   
> > +/**
> > + * pci_tsm_guest_req() - helper to marshal guest requests to the TSM driver
> > + * @pdev: @pdev representing a bound tdi
> > + * @scope: security model scope for the TVM request
> > + * @req_in: Input payload forwarded from the guest
> > + * @in_len: Length of @req_in
> > + * @out_len: Output length of the returned response payload
> > + *
> > + * This is a common entry point for KVM service handlers in userspace responding
>
> KVM will have to know about the host's pci_dev which it does not. I'd
> postpone mentioning KVM here till it learns about pci_dev, if.

Oh, this was a copy paste mistake that I meant to cleanup after Yilun's
clarification [1]

[1]: http://lore.kernel.org/aCbglieuHI1BJDkz@yilunxu-OptiPlex-7050

So, the plan is *not* for KVM to have PCI device awareness.

> > + * to TDI information or state change requests. The scope parameter limits
> > + * requests to TDISP state management, or limited debug.
> > + *
> > + * Returns a pointer to the response payload on success, @req_in if there is no
> > + * response to a successful request, or an ERR_PTR() on failure.
> > + *
> > + * Caller is responsible for kvfree() on the result when @ret != @req_in and
> > + * !IS_ERR_OR_NULL(@ret).
> 
> Uff... So the caller (which is IOMMUFD vdevice) has to check and
> decide on whether to kvfree. ioctl() is likely to have 2 buffers
> anyway and preallocate the response buffer, why make IOMMUFD care
> about this?

No, iommufd does not need to preallocate the response buffer, the
response buffer is allocated by the responder.

This follows the example fwctl because this guest_req() transport is in
the same class of kernel bypass tunnels. No need to reinvent a common
device RPC transport mechanism.

> > + *
> > + * Context: Caller is responsible for calling this within the pci_tsm_bind()
> > + * state of the TDI.
> > + */
> > +void *pci_tsm_guest_req(struct pci_dev *pdev, enum pci_tsm_req_scope scope,
> > +			void *req_in, size_t in_len, size_t *out_len)
> > +{
> > +	const struct pci_tsm_ops *ops;
> > +	struct pci_tsm_pf0 *tsm_pf0;
> > +	struct pci_tdi *tdi;
> > +	int rc;
> > +
> > +	/*
> > +	 * Forbid requests that are not directly related to TDISP
> > +	 * operations
> > +	 */
> > +	if (scope > PCI_TSM_REQ_STATE_CHANGE)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	ACQUIRE(rwsem_read_intr, lock)(&pci_tsm_rwsem);
> > +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &lock)))
> 
> Why double braces?

Because of the assignment used as truth value. The evaluation of the
ACQUIRE_ERR() result in the assignment was a compactness choice that
PeterZ made in his original proposal [2] that made sense to me, so I
carried it forward.

[2]: http://lore.kernel.org/20250509104028.GL4439@noisy.programming.kicks-ass.net

[..]
> > @@ -50,6 +51,9 @@ struct pci_tsm_ops {
> >   		struct pci_tdi *(*bind)(struct pci_dev *pdev,
> >   					struct kvm *kvm, u32 tdi_id);
> >   		void (*unbind)(struct pci_tdi *tdi);
> > +		void *(*guest_req)(struct pci_dev *pdev,
> 
> We have pdev in pci_tdi, pci_tsm and pci_tsm_pf0 (via .base), using
> these in pci_tsm_ops will document better which call is allowed on
> what entity - DSM or TDI. Or may be ditch those back "pdev"
> references?

Not immediately understanding what change you want here. Do you want
iommufd to track the pci_tdi?

> > +				   enum pci_tsm_req_scope scope, void *req_in,
> > +				   size_t in_len, size_t *out_len);
> 
> 
> Out of curiosity (probably could go to the commit log) - for what kind
> of request and on which platform we do not know the response size in
> advance? On AMD, the request and response sizes are fixed.

I don't know. Given this is to support any possible combination of TSM
and ABI I took inspiration from fwctl which is trying to solve a similar
common transport problem.

> And the userspace (which makes such request) will allocate some memory
> before calling such ioctl(), can "void *req_in" be "void __user
> *reg_in"? The CCP driver is going to copy the request and response
> anyway as there are RMP rules about them.
> 
> And what is wrong with returning "int" as an error vs ERR_PTR(), is
> there a recommendation for this, or something?

Keep interface innovation to minimum and follow an existing pattern.

[..]
> > +/**
> > + * enum pci_tsm_req_scope - Scope of guest requests to be validated by TSM
> > + *
> > + * Guest requests are a transport for a TVM to communicate with a TSM + DSM for
> > + * a given TDI. A TSM driver is responsible for maintaining the kernel security
> > + * model and limit commands that may affect the host, or are otherwise outside
> > + * the typical TDISP operational model.
> > + */
> > +enum pci_tsm_req_scope {
> > +	/**
> > +	 * @PCI_TSM_REQ_INFO: Read-only, without side effects, request for
> > +	 * typical TDISP collateral information like Device Interface Reports.
> > +	 * No device secrets are permitted, and no device state is changed.
> > +	 */
> > +	PCI_TSM_REQ_INFO = 0,
> > +	/**
> > +	 * @PCI_TSM_REQ_STATE_CHANGE: Request to change the TDISP state from
> > +	 * UNLOCKED->LOCKED, LOCKED->RUN. No any other device state,
> > +	 * configuration, or data change is permitted.
> > +	 */
> > +	PCI_TSM_REQ_STATE_CHANGE = 1,
> > +	/**
> > +	 * @PCI_TSM_REQ_DEBUG_READ: Read-only request for debug information
> > +	 *
> > +	 * A method to facilitate TVM information retrieval outside of typical
> > +	 * TDISP operational requirements. No device secrets are permitted.
> > +	 */
> > +	PCI_TSM_REQ_DEBUG_READ = 2,
> > +	/**
> > +	 * @PCI_TSM_REQ_DEBUG_WRITE: Device state changes for debug purposes
> > +	 *
> > +	 * The request may affect the operational state of the device outside of
> > +	 * the TDISP operational model. If allowed, requires CAP_SYS_RAW_IO, and
> > +	 * will taint the kernel.
> > +	 */
> > +	PCI_TSM_REQ_DEBUG_WRITE = 3,
> 
> 
> What is going to enforce this and how? It is a guest request, ideally
> encrypted, and the host does not really have to know the nature of the
> request (if the guest wants something from the host to do in addition
> to what is it asking the TSM to do - then GHCB is for that). And 3 of
> 4 AMD TIO requests (STATE_CHANGE is a host request and no plan for
> DEBUG) do not fit in any category from the above anyway. imho we do
> not need it at least now.

While the TSM is in the trust boundary of the TVM, the TSM and the TVM
are not necessarily trusted by the VMM. It has a responsibility to
maintain its own security model especially when marshaling opaque blobs
on behalf of a guest. This scope parameter serves the same purpose as it
does in fwctl to maintain a security model and explicitly control for
requests that are out of scope.

The enforcement is market and regulatory forces to make solutions are
not bypass security model expectations of the operating system.