Re: [PATCH v7 4/9] PCI/TSM: Establish Secure Sessions and Link Encryption

Jonathan Cameron
2025-10-29T15:53:08Z

On Thu, 23 Oct 2025 19:04:13 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The PCIe 7.0 specification, section 11, defines 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
> two 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", Secure Session and physical Link Encryption, operations are
> defined at this stage with placeholders for the device security, Trusted
> Computing Base entry / exit, operations.

That list probably needs an 'and'

>
> 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@wunner.de>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 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>

Some comments on comments/documentation inline. With those addressed
(which should be straight forward)
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> new file mode 100644
> index 000000000000..e3107ede2a0f
> --- /dev/null
> +++ b/include/linux/pci-tsm.h
> @@ -0,0 +1,159 @@
> +/* 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;
See below for note on a duplicate of this.

> +struct tsm_dev;
> +
> +/*
> + * 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

devsec_ops?

> + * function via the platform TSM (typically virtual function
> + * operations).
> + * @owner: Back reference to the TSM device that owns this instance.
Not seeing this below.

> + *
> + * This operations are mutually exclusive either a tsm_dev instance
> + * manages physical link properties or it manages function security
> + * states like TDISP lock/unlock.
> + */
> +struct pci_tsm_ops {
> + /*
> + * struct pci_tsm_link_ops - Manage physical link and the TSM/DSM session
> + * @probe: establish context with the TSM (allocate / wrap 'struct
> + * pci_tsm') for follow-on link operations
> + * @remove: destroy link operations context
> + * @connect: establish / validate a secure connection (e.g. IDE)
> + * with the device
> + * @disconnect: teardown the secure link
> + *
> + * 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.
> + */
> + struct_group_tagged(pci_tsm_link_ops, link_ops,
> + struct pci_tsm *(*probe)(struct tsm_dev *tsm_dev,
> + 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_devsec_ops - Manage the security state of the function
> + * @lock: establish context with the TSM (allocate / wrap 'struct
> + * pci_tsm') for follow-on security state transitions from the
> + * LOCKED state
> + * @unlock: destroy TSM context and return device to UNLOCKED state
> + *
> + * Context: @lock and @unlock run under pci_tsm_rwsem held for write to
> + * sync with TSM unregistration and each other
> + */
> + struct_group_tagged(pci_tsm_devsec_ops, devsec_ops,
> + struct pci_tsm *(*lock)(struct tsm_dev *tsm_dev,
> + struct pci_dev *pdev);
> + void (*unlock)(struct pci_tsm *tsm);
> + );
> +};

> +#ifdef CONFIG_PCI_TSM
> +struct tsm_dev;

Seems to be declared already (not under an ifdef) above.

> +int pci_tsm_register(struct tsm_dev *tsm_dev);
...

> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> new file mode 100644
> index 000000000000..094650454aa7
> --- /dev/null
> +++ b/drivers/pci/tsm.c

> +static int pci_tsm_connect(struct pci_dev *pdev, struct tsm_dev *tsm_dev)
> +{
> + int rc;
> + struct pci_tsm_pf0 *tsm_pf0;
> + const struct pci_tsm_ops *ops = tsm_dev->pci_ops;
> + struct pci_tsm *pci_tsm __free(tsm_remove) = ops->probe(tsm_dev, pdev);
> +
> + /* connect() mutually exclusive with subfunction pci_tsm_init() */

Extra space after ()

> + lockdep_assert_held_write(&pci_tsm_rwsem);
> +
> + if (!pci_tsm)
> + return -ENXIO;
> +
> + pdev->tsm = pci_tsm;
> + tsm_pf0 = to_pci_tsm_pf0(pdev->tsm);
> +
> + /* mutex_intr assumes connect() is always sysfs/user driven */
> + ACQUIRE(mutex_intr, lock)(&tsm_pf0->lock);
> + if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
> + return rc;
> +
> + rc = ops->connect(pdev);
> + if (rc)
> + return rc;
> +
> + pdev->tsm = no_free_ptr(pci_tsm);
> +
> + /*
> + * Now that the DSM is established, probe() all the potential
> + * dependent functions. Failure to probe a function is not fatal
> + * to connect(), it just disables subsequent security operations
> + * for that function.
> + *
> + * Note this is done unconditionally, without regard to finding
> + * PCI_EXP_DEVCAP_TEE on the dependent function, for robustness. The DSM
> + * is the ultimate arbiter of security state relative to a given
> + * interface id, and if it says it can manage TDISP state of a function,
> + * let it.
> + */
> + if (has_tee(pdev))
> + pci_tsm_walk_fns(pdev, probe_fn, pdev);
> + return 0;
> +}

> +/*
> + * Find the PCI Device instance that serves as the Device Security Manager (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.

This comment has me confused. I would normally expect parent to have the guaranteed
longer life span than the child. This seems to say the opposite.
Code itself is fine.

> + */
> +static struct pci_dev *find_dsm_dev(struct pci_dev *pdev)
> +{
> + struct device *grandparent;
> + struct pci_dev *uport;
> +
> + 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;
> +
> + /*
> + * For cases where a switch may be hosting TDISP services on behalf of
> + * downstream devices, check the first upstream port relative to this
> + * endpoint.
> + */
> + if (!pdev->dev.parent)
> + return NULL;
> + grandparent = pdev->dev.parent->parent;
> + if (!grandparent)
> + return NULL;
> + if (!dev_is_pci(grandparent))
> + return NULL;
> + uport = to_pci_dev(grandparent);
> + if (!pci_is_pcie(uport) ||
> + pci_pcie_type(uport) != PCI_EXP_TYPE_UPSTREAM)
> + return NULL;
> +
> + if (is_dsm(uport))
> + return uport;
> + return NULL;
> +}