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

[Alexey Kardashevskiy <aik@amd.com>]

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/

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


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

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

> +		return ERR_PTR(rc);
> +
> +	if (!pdev->tsm)
> +		return ERR_PTR(-ENXIO);
> +
> +	ops = pdev->tsm->ops;
> +
> +	if (!is_link_tsm(ops->owner))
> +		return ERR_PTR(-ENXIO);
> +
> +	tsm_pf0 = to_pci_tsm_pf0(pdev->tsm);
> +	ACQUIRE(mutex_intr, ops_lock)(&tsm_pf0->lock);
> +	if ((rc = ACQUIRE_ERR(mutex_intr, &ops_lock)))
> +		return ERR_PTR(rc);
> +
> +	tdi = pdev->tsm->tdi;
> +	if (!tdi)
> +		return ERR_PTR(-ENXIO);
> +	return ops->guest_req(pdev, scope, req_in, in_len, out_len);
> +}
> +EXPORT_SYMBOL_GPL(pci_tsm_guest_req);
> +
>   static void pci_tsm_unbind_all(struct pci_dev *pdev)
>   {
>   	pci_tsm_walk_fns_reverse(pdev, __pci_tsm_unbind, NULL);
> diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> index 337b566adfc5..5b61aac2e9f7 100644
> --- a/include/linux/pci-tsm.h
> +++ b/include/linux/pci-tsm.h
> @@ -33,14 +33,15 @@ struct pci_tsm_ops {
>   	 * @disconnect: teardown the secure link
>   	 * @bind: bind a TDI in preparation for it to be accepted by a TVM
>   	 * @unbind: remove a TDI from secure operation with a TVM
> +	 * @guest_req: marshal TVM information and state change requests
>   	 *
>   	 * Context: @probe, @remove, @connect, and @disconnect run under
>   	 * pci_tsm_rwsem held for write to sync with TSM unregistration and
>   	 * mutual exclusion of @connect and @disconnect. @connect and
>   	 * @disconnect additionally run under the DSM lock (struct
>   	 * pci_tsm_pf0::lock) as well as @probe and @remove of the subfunctions.
> -	 * @bind and @unbind run under pci_tsm_rwsem held for read and the DSM
> -	 * lock.
> +	 * @bind, @unbind, and @guest_req run under pci_tsm_rwsem held for read
> +	 * and the DSM lock.
>   	 */
>   	struct_group_tagged(pci_tsm_link_ops, link_ops,
>   		struct pci_tsm *(*probe)(struct pci_dev *pdev);
> @@ -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?


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

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?


>   	);
>   
>   	/*
> @@ -143,6 +147,44 @@ static inline bool is_pci_tsm_pf0(struct pci_dev *pdev)
>   	return PCI_FUNC(pdev->devfn) == 0;
>   }
>   
> +/**
> + * 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. Thanks,



> +};
> +
>   #ifdef CONFIG_PCI_TSM
>   struct tsm_dev;
>   int pci_tsm_register(struct tsm_dev *tsm_dev);
> @@ -154,6 +196,8 @@ int pci_tsm_pf0_constructor(struct pci_dev *pdev, struct pci_tsm_pf0 *tsm,
>   void pci_tsm_pf0_destructor(struct pci_tsm_pf0 *tsm);
>   int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u32 tdi_id);
>   void pci_tsm_unbind(struct pci_dev *pdev);
> +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);
>   #else
>   static inline int pci_tsm_register(struct tsm_dev *tsm_dev)
>   {
> @@ -169,5 +213,12 @@ static inline int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u64 tdi_id
>   static inline void pci_tsm_unbind(struct pci_dev *pdev)
>   {
>   }
> +static inline 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)
> +{
> +	return ERR_PTR(-ENXIO);
> +}
>   #endif
>   #endif /*__PCI_TSM_H */

-- 
Alexey