Message-ID: <20250729155650.000017b3@huawei.com>
Date: Tue, 29 Jul 2025 15:56:50 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Dan Williams <dan.j.williams@...el.com>
CC: <linux-coco@...ts.linux.dev>, <linux-pci@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>, <aik@....com>,
	<lukas@...ner.de>, Samuel Ortiz <sameo@...osinc.com>, Xu Yilun
	<yilun.xu@...ux.intel.com>
Subject: Re: [PATCH v4 04/10] PCI/TSM: Authenticate devices via platform TSM

On Thu, 17 Jul 2025 11:33:52 -0700
Dan Williams <dan.j.williams@...el.com> wrote:

> The PCIe 6.1 specification, section 11, introduces the Trusted Execution
> Environment (TEE) Device Interface Security Protocol (TDISP).  This
> protocol definition builds upon Component Measurement and Authentication
> (CMA), and link Integrity and Data Encryption (IDE). It adds support for
> assigning devices (PCI physical or virtual function) to a confidential
> VM such that the assigned device is enabled to access guest private
> memory protected by technologies like Intel TDX, AMD SEV-SNP, RISCV
> COVE, or ARM CCA.
> 
> The "TSM" (TEE Security Manager) is a concept in the TDISP specification
> of an agent that mediates between a "DSM" (Device Security Manager) and
> system software in both a VMM and a confidential VM. A VMM uses TSM ABIs
> to setup link security and assign devices. A confidential VM uses TSM
> ABIs to transition an assigned device into the TDISP "RUN" state and
> validate its configuration. From a Linux perspective the TSM abstracts
> many of the details of TDISP, IDE, and CMA. Some of those details leak
> through at times, but for the most part TDISP is an internal
> implementation detail of the TSM.
> 
> CONFIG_PCI_TSM adds an "authenticated" attribute and "tsm/" subdirectory
> to pci-sysfs. Consider that the TSM driver may itself be a PCI driver.
> Userspace can watch for the arrival of a "TSM" device,
> /sys/class/tsm/tsm0/uevent KOBJ_CHANGE, to know when the PCI core has
> initialized TSM services.
> 
> The operations that can be executed against a PCI device are split into
> 2 mutually exclusive operation sets, "Link" and "Security" (struct
> pci_tsm_{link,security}_ops). The "Link" operations manage physical link
> security properties and communication with the device's Device Security
> Manager firmware. These are the host side operations in TDISP. The
> "Security" operations coordinate the security state of the assigned
> virtual device (TDI). These are the guest side operations in TDISP. Only
> link management operations are defined at this stage and placeholders
> provided for the security operations.
> 
> The locking allows for multiple devices to be executing commands
> simultaneously, one outstanding command per-device and an rwsem
> synchronizes the implementation relative to TSM
> registration/unregistration events.
> 
> Thanks to Wu Hao for his work on an early draft of this support.
> 
> Cc: Lukas Wunner <lukas@...ner.de>
> Cc: Samuel Ortiz <sameo@...osinc.com>
> Cc: Alexey Kardashevskiy <aik@....com>
> Acked-by: Bjorn Helgaas <bhelgaas@...gle.com>
> Co-developed-by: Xu Yilun <yilun.xu@...ux.intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@...ux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
Various things inline.

> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> new file mode 100644
> index 000000000000..0784cc436dd3
> --- /dev/null
> +++ b/drivers/pci/tsm.c
> @@ -0,0 +1,554 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * TEE Security Manager for the TEE Device Interface Security Protocol
> + * (TDISP, PCIe r6.1 sec 11)
> + *
> + * Copyright(c) 2024 Intel Corporation. All rights reserved.
> + */

> +static void tsm_remove(struct pci_tsm *tsm)
> +{
> +	struct pci_dev *pdev;
> +
> +	if (!tsm)

You protect against this in the DEFINE_FREE() so probably safe
to assume it is always set if we get here.

> +		return;
> +
> +	pdev = tsm->pdev;
> +	tsm->ops->remove(tsm);
> +	pdev->tsm = NULL;
> +}
> +DEFINE_FREE(tsm_remove, struct pci_tsm *, if (_T) tsm_remove(_T))
> +
> +static int call_cb_put(struct pci_dev *pdev, void *data,

Is this combination worth while?  I don't like the 'and' aspect of it
and it only saves a few lines...

vs
	if (pdev) {
		rc = cb(pdev, data);
		pci_dev_put(pdev);
		if (rc)
			return;
	}

> +		       int (*cb)(struct pci_dev *pdev, void *data))
> +{
> +	int rc;
> +
> +	if (!pdev)
> +		return 0;
> +	rc = cb(pdev, data);
> +	pci_dev_put(pdev);
> +	return rc;
> +}
> +
> +static void pci_tsm_walk_fns(struct pci_dev *pdev,
> +			     int (*cb)(struct pci_dev *pdev, void *data),
> +			     void *data)
> +{
> +	struct pci_dev *fn;
> +	int i;
> +
> +	/* walk virtual functions */
> +        for (i = 0; i < pci_num_vf(pdev); i++) {
> +		fn = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
> +						 pci_iov_virtfn_bus(pdev, i),
> +						 pci_iov_virtfn_devfn(pdev, i));
> +		if (call_cb_put(fn, data, cb))
> +			return;
> +        }
> +
> +	/* walk subordinate physical functions */
> +	for (i = 1; i < 8; i++) {
> +		fn = pci_get_slot(pdev->bus,
> +				  PCI_DEVFN(PCI_SLOT(pdev->devfn), i));
> +		if (call_cb_put(fn, data, cb))
> +			return;
> +	}
> +
> +	/* walk downstream devices */
> +        if (pci_pcie_type(pdev) != PCI_EXP_TYPE_UPSTREAM)

spaces rather than tabs...


> +                return;
> +
> +        if (!is_dsm(pdev))
> +                return;
> +
> +        pci_walk_bus(pdev->subordinate, cb, data);
> +}
> +
> +static void pci_tsm_walk_fns_reverse(struct pci_dev *pdev,
> +				     int (*cb)(struct pci_dev *pdev,
> +					       void *data),
> +				     void *data)
> +{
> +	struct pci_dev *fn;
> +	int i;
> +
> +	/* reverse walk virtual functions */
> +	for (i = pci_num_vf(pdev) - 1; i >= 0; i--) {
> +		fn = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
> +						 pci_iov_virtfn_bus(pdev, i),
> +						 pci_iov_virtfn_devfn(pdev, i));
> +		if (call_cb_put(fn, data, cb))
> +			return;
> +	}
> +
While it probably doesn't matter can we make this strict reverse by doing
the physical functions first?  I prefer not to think about whether it matters.


> +	/* reverse walk subordinate physical functions */
> +	for (i = 7; i >= 1; i--) {
> +		fn = pci_get_slot(pdev->bus,
> +				  PCI_DEVFN(PCI_SLOT(pdev->devfn), i));
> +		if (call_cb_put(fn, data, cb))
> +			return;
> +	}
> +
> +	/* reverse walk downstream devices */
> +	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_UPSTREAM)
> +		return;
> +
> +	if (!is_dsm(pdev))
> +		return;
> +
> +	pci_walk_bus_reverse(pdev->subordinate, cb, data);

Likewise, can we do this before the rest.

> +}

> +/*
> + * Find the PCI Device instance that serves as the Device Security
> + * Manger (DSM) for @pdev. Note that no additional reference is held for
> + * the resulting device because @pdev always has a longer registered
> + * lifetime than its DSM by virtue of being a child of or identical to
> + * its DSM.
> + */
> +static struct pci_dev *find_dsm_dev(struct pci_dev *pdev)
> +{
> +	struct pci_dev *uport_pf0;
> +
> +	if (is_pci_tsm_pf0(pdev))
> +		return pdev;
> +
> +	struct pci_dev *pf0 __free(pci_dev_put) = pf0_dev_get(pdev);
> +	if (!pf0)
> +		return NULL;
> +
> +	if (is_dsm(pf0))
> +		return pf0;


Unusual for a find command to not hold the device reference on the device
it returns.  Maybe just call that out in the comment.

> +
> +	/*
> +	 * For cases where a switch may be hosting TDISP services on
> +	 * behalf of downstream devices, check the first usptream port
> +	 * relative to this endpoint.
> +         */
Odd alignment. Space rather than tab.


> +	if (!pdev->dev.parent || !pdev->dev.parent->parent)
> +		return NULL;
> +
> +	uport_pf0 = to_pci_dev(pdev->dev.parent->parent);
> +	if (is_dsm(uport_pf0))
> +		return uport_pf0;
> +	return NULL;
> +}


> +/**
> + * pci_tsm_pf0_constructor() - common 'struct pci_tsm_pf0' initialization
> + * @pdev: Physical Function 0 PCI device (as indicated by is_pci_tsm_pf0())
> + * @tsm: context to initialize

ops missing.  Run kernel-doc or do W=1 build to catch these.

> + */
> +int pci_tsm_pf0_constructor(struct pci_dev *pdev, struct pci_tsm_pf0 *tsm,
> +			    const struct pci_tsm_ops *ops)
> +{
> +	struct tsm_dev *tsm_dev = ops->owner;
> +
> +	mutex_init(&tsm->lock);
Might as well do devm_mutex_init()

> +	tsm->doe_mb = pci_find_doe_mailbox(pdev, PCI_VENDOR_ID_PCI_SIG,
> +					   PCI_DOE_PROTO_CMA);
> +	if (!tsm->doe_mb) {
> +		pci_warn(pdev, "TSM init failure, no CMA mailbox\n");
> +		return -ENODEV;
> +	}
> +
> +	if (tsm_pci_group(tsm_dev))
> +		sysfs_merge_group(&pdev->dev.kobj, tsm_pci_group(tsm_dev));
> +
> +	return pci_tsm_constructor(pdev, &tsm->tsm, ops);
> +}
> +EXPORT_SYMBOL_GPL(pci_tsm_pf0_constructor);

> diff --git a/drivers/virt/coco/tsm-core.c b/drivers/virt/coco/tsm-core.c
> index 1f53b9049e2d..093824dc68dd 100644
> --- a/drivers/virt/coco/tsm-core.c
> +++ b/drivers/virt/coco/tsm-core.c

> +/*
> + * Caller responsible for ensuring it does not race tsm_dev
> + * unregistration.
Wrap is a bit early. unregistration fits on the line above.
> + */
> +struct tsm_dev *find_tsm_dev(int id)
> +{
> +	guard(rcu)();
> +	return idr_find(&tsm_idr, id);
> +}

> @@ -44,6 +76,29 @@ static struct tsm_dev *alloc_tsm_dev(struct device *parent,
>  	return no_free_ptr(tsm_dev);
>  }
>  
> +static struct tsm_dev *tsm_register_pci_or_reset(struct tsm_dev *tsm_dev,
> +						 struct pci_tsm_ops *pci_ops)
> +{
> +	int rc;
> +
> +	if (!pci_ops)
> +		return tsm_dev;
> +
> +	pci_ops->owner = tsm_dev;
> +	tsm_dev->pci_ops = pci_ops;
> +	rc = pci_tsm_register(tsm_dev);
> +	if (rc) {
> +		dev_err(tsm_dev->dev.parent,
> +			"PCI/TSM registration failure: %d\n", rc);
> +		device_unregister(&tsm_dev->dev);

As below. I'm fairly sure this device_unregister is nothing to do with
what this function is doing, so having it buried in here is less easy
to follow than pushing it up a layer.

> +		return ERR_PTR(rc);
> +	}
> +
> +	/* Notify TSM userspace that PCI/TSM operations are now possible */
> +	kobject_uevent(&tsm_dev->dev.kobj, KOBJ_CHANGE);
> +	return tsm_dev;
> +}
> +
>  static void put_tsm_dev(struct tsm_dev *tsm_dev)
>  {
>  	if (!IS_ERR_OR_NULL(tsm_dev))
> @@ -54,7 +109,8 @@ DEFINE_FREE(put_tsm_dev, struct tsm_dev *,
>  	    if (!IS_ERR_OR_NULL(_T)) put_tsm_dev(_T))
>  
>  struct tsm_dev *tsm_register(struct device *parent,
> -			     const struct attribute_group **groups)
> +			     const struct attribute_group **groups,
> +			     struct pci_tsm_ops *pci_ops)
>  {
>  	struct tsm_dev *tsm_dev __free(put_tsm_dev) =
>  		alloc_tsm_dev(parent, groups);
> @@ -73,12 +129,13 @@ struct tsm_dev *tsm_register(struct device *parent,
>  	if (rc)
>  		return ERR_PTR(rc);
>  
> -	return no_free_ptr(tsm_dev);
> +	return tsm_register_pci_or_reset(no_free_ptr(tsm_dev), pci_ops);

Having a function call that either succeeds or cleans up something it
never did on error is odd.  The or_reset hints at that oddity but
to me is not enough. If you want to use __free magic in here
maybe hand off the tsm_dev on succesful device registration.

	struct tsm_dev *registered_tsm_dev __free(unregister_tsm_dev) =
		no_free_ptr(tsm_dev);

	rc = tsm_register_pci(registered_tsm_dev, pci_ops);
	//change return type as no need for another tsm_dev
	if (rc)
		return ERR_PTR(rc);

	return no_free_ptr(registered_tsm_dev);
	

>  }
>  EXPORT_SYMBOL_GPL(tsm_register);
>  
>  void tsm_unregister(struct tsm_dev *tsm_dev)
>  {
> +	pci_tsm_unregister(tsm_dev);
>  	device_unregister(&tsm_dev->dev);
>  }
>  EXPORT_SYMBOL_GPL(tsm_unregister);
> diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> new file mode 100644
> index 000000000000..f370c022fac4
> --- /dev/null
> +++ b/include/linux/pci-tsm.h
> @@ -0,0 +1,158 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __PCI_TSM_H
> +#define __PCI_TSM_H
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +
> +struct pci_tsm;
> +
> +/*

/**

Or was this intentional? Feels like it should be kernel-doc. 

> + * struct pci_tsm_ops - manage confidential links and security state
> + * @link_ops: Coordinate PCIe SPDM and IDE establishment via a platform TSM.
> + * 	      Provide a secure session transport for TDISP state management
> + * 	      (typically bare metal physical function operations).
> + * @sec_ops: Lock, unlock, and interrogate the security state of the
> + *	     function via the platform TSM (typically virtual function
> + *	     operations).
> + * @owner: Back reference to the TSM device that owns this instance.
> + *
> + * This operations are mutually exclusive either a tsm_dev instance
> + * manages phyiscal link properties or it manages function security
> + * states like TDISP lock/unlock.
> + */
> +struct pci_tsm_ops {
> +	/*
Likewise though I'm not sure if kernel-doc deals with struct groups.

> +	 * struct pci_tsm_link_ops - Manage physical link and the TSM/DSM session
> +	 * @probe: probe device for tsm link operation readiness, setup
> +	 *	   DSM context
> +	 * @remove: destroy DSM context
> +	 * @connect: establish / validate a secure connection (e.g. IDE)
> +	 *	     with the device
> +	 * @disconnect: teardown the secure link
> +	 *
> +	 * @probe and @remove run in pci_tsm_rwsem held for write context. All
> +	 * other ops run under the @pdev->tsm->lock mutex and pci_tsm_rwsem held
> +	 * for read.
> +	 */
> +	struct_group_tagged(pci_tsm_link_ops, link_ops,
> +		struct pci_tsm *(*probe)(struct pci_dev *pdev);
> +		void (*remove)(struct pci_tsm *tsm);
> +		int (*connect)(struct pci_dev *pdev);
> +		void (*disconnect)(struct pci_dev *pdev);
> +	);
> +
> +	/*
> +	 * struct pci_tsm_security_ops - Manage the security state of the function
> +	 * @sec_probe: probe device for tsm security operation
> +	 *	       readiness, setup security context
> +	 * @sec_remove: destroy security context
> +	 *
> +	 * @sec_probe and @sec_remove run in pci_tsm_rwsem held for
> +	 * write context. All other ops run under the @pdev->tsm->lock
> +	 * mutex and pci_tsm_rwsem held for read.
> +	 */
> +	struct_group_tagged(pci_tsm_security_ops, ops,
> +		struct pci_tsm *(*sec_probe)(struct pci_dev *pdev);
> +		void (*sec_remove)(struct pci_tsm *tsm);
> +	);
> +	struct tsm_dev *owner;
> +};

> +
> +/**
> + * struct pci_tsm_pf0 - Physical Function 0 TDISP link context
> + * @tsm: generic core "tsm" context
> + * @lock: protect @state vs pci_tsm_ops invocation

What is @state referring to? 

> + * @doe_mb: PCIe Data Object Exchange mailbox
> + */
> +struct pci_tsm_pf0 {
> +	struct pci_tsm tsm;
> +	struct mutex lock;
> +	struct pci_doe_mb *doe_mb;
> +};

Powered by blists - more mailing lists