On Thu, 15 May 2025 22:47:22 -0700
Dan Williams <dan.j.williams@xxxxxxxxx> 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 the "TSM" core device,
> /sys/class/tsm/tsm0/uevent, to know when the PCI core has initialized
> TSM services.
>
> The common verbs that the low-level TSM drivers implement are defined by
> 'struct pci_tsm_ops'. For now only 'connect' and 'disconnect' are
> defined for secure session and IDE establishment. The 'probe' and
> 'remove' operations setup per-device context objects starting with
> 'struct pci_tsm_pf0', the device Physical Function 0 that mediates
> communication to the device's Security Manager (DSM).
>
> 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@xxxxxxxxx>
> Cc: Samuel Ortiz <sameo@xxxxxxxxxxxx>
> Cc: Alexey Kardashevskiy <aik@xxxxxxx>
> Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Co-developed-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
Trivial stuff on this one. See inline.
Jonathan
> ---
> Documentation/ABI/testing/sysfs-bus-pci | 45 +++
> MAINTAINERS | 2 +
> drivers/pci/Kconfig | 14 +
> drivers/pci/Makefile | 1 +
> drivers/pci/pci-sysfs.c | 4 +
> drivers/pci/pci.h | 10 +
> drivers/pci/probe.c | 1 +
> drivers/pci/remove.c | 3 +
> drivers/pci/tsm.c | 437 ++++++++++++++++++++++++
> drivers/virt/coco/host/tsm-core.c | 19 +-
> include/linux/pci-tsm.h | 138 ++++++++
> include/linux/pci.h | 3 +
> include/linux/tsm.h | 4 +-
> include/uapi/linux/pci_regs.h | 1 +
> 14 files changed, 679 insertions(+), 3 deletions(-)
> create mode 100644 drivers/pci/tsm.c
> create mode 100644 include/linux/pci-tsm.h
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 69f952fffec7..1d38e0d3a6be 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -612,3 +612,48 @@ Description:
>
> # ls doe_features
> 0001:01 0001:02 doe_discovery
> +
> +What: /sys/bus/pci/devices/.../tsm/
> +Date: July 2024
Guess the date for merge?
> +Contact: linux-coco@xxxxxxxxxxxxxxx
> +Description:
> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> new file mode 100644
> index 000000000000..d00a8e471340
> --- /dev/null
> +++ b/drivers/pci/tsm.c
> @@ -0,0 +1,437 @@
> +// 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.
> + */
> +
> +#define dev_fmt(fmt) "TSM: " fmt
> +
> +#include <linux/bitfield.h>
> +#include <linux/xarray.h>
Not seeing an xa stuff yet.
Check the others are all needed or push them forwards to appropriate patch.
> +#include <linux/sysfs.h>
> +
> +#include <linux/pci.h>
> +#include <linux/pci-doe.h>
> +#include <linux/pci-tsm.h>
> +#include "pci.h"
> +static bool pci_tsm_pf0_group_visible(struct kobject *kobj)
> +{
> + struct device *dev = kobj_to_dev(kobj);
> + struct pci_dev *pdev = to_pci_dev(dev);
> +
> + if (pdev->tsm && is_pci_tsm_pf0(pdev))
> + return true;
> + return false;
Unless this is going to get more complex later
return pdev->tsm && is_pci_tsm_pf0(pdev);
> +}
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(pci_tsm_pf0);
> +
> +const struct attribute_group pci_tsm_auth_attr_group = {
> + .attrs = pci_tsm_auth_attrs,
> + .is_visible = SYSFS_GROUP_VISIBLE(pci_tsm_pf0),
> +};
> +static void pci_tsm_pf0_init(struct pci_dev *pdev)
> +{
> + bool tee_cap;
> +
> + tee_cap = pdev->devcap & PCI_EXP_DEVCAP_TEE;
Might as well put that on first line.
> +
> + if (!(pdev->ide_cap || tee_cap))
> + return;
> diff --git a/drivers/virt/coco/host/tsm-core.c b/drivers/virt/coco/host/tsm-core.c
> index 4f64af1a8967..51146f226a64 100644
> --- a/drivers/virt/coco/host/tsm-core.c
> +++ b/drivers/virt/coco/host/tsm-core.c
> @@ -8,11 +8,13 @@
> #include <linux/device.h>
> #include <linux/module.h>
> #include <linux/cleanup.h>
> +#include <linux/pci-tsm.h>
>
> static DECLARE_RWSEM(tsm_core_rwsem);
> static struct class *tsm_class;
> static struct tsm_core_dev {
> struct device dev;
> + const struct pci_tsm_ops *pci_ops;
> } *tsm_core;
>
> static struct tsm_core_dev *
> @@ -39,7 +41,8 @@ static void put_tsm_core(struct tsm_core_dev *core)
> DEFINE_FREE(put_tsm_core, struct tsm_core_dev *,
> if (!IS_ERR_OR_NULL(_T)) put_tsm_core(_T))
> struct tsm_core_dev *tsm_register(struct device *parent,
> - const struct attribute_group **groups)
> + const struct attribute_group **groups,
> + const struct pci_tsm_ops *pci_ops)
> {
> struct device *dev;
> int rc;
> @@ -61,10 +64,20 @@ struct tsm_core_dev *tsm_register(struct device *parent,
> if (rc)
> return ERR_PTR(rc);
>
> + rc = pci_tsm_core_register(pci_ops, NULL);
> + if (rc) {
> + dev_err(parent, "PCI initialization failure: %pe\n",
> + ERR_PTR(rc));
> + return ERR_PTR(rc);
> + }
> +
> rc = device_add(dev);
> - if (rc)
> + if (rc) {
> + pci_tsm_core_unregister(pci_ops);
> return ERR_PTR(rc);
> + }
>
> + core->pci_ops = pci_ops;
> tsm_core = no_free_ptr(core);
>
> return tsm_core;
> @@ -79,7 +92,9 @@ void tsm_unregister(struct tsm_core_dev *core)
> return;
> }
>
> + pci_tsm_core_unregister(core->pci_ops);
> device_unregister(&core->dev);
Using device_initialize() and device_add() in probe but device_unregister()
in remove results in trivial ordering mess like this. I'd split the
remove() path so we can take down in the reverse of setup with pci_tsm_core_unregister()
between device_del() and put_device()
This ordering thing is common enough though that maybe we can just
not worry about it.
> +
Push whitespace change back to earlier patch.
> tsm_core = NULL;
> }
> 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..00fdae087069
> --- /dev/null
> +++ b/include/linux/pci-tsm.h
> @@ -0,0 +1,138 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __PCI_TSM_H
> +#define __PCI_TSM_H
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +
> +struct pci_dev;
Given you have linux/pci.h no need for the forwards def.
> +
> +enum pci_tsm_state {
> + PCI_TSM_ERR = -1,
> + PCI_TSM_INIT,
> + PCI_TSM_CONNECT,
> +};
> +
> +/**
> + * enum pci_tsm_type - 'struct pci_tsm' object types
Kernel-doc should warn on incomplete docs. I'd add trivial comment for
INVALID to avoid that.
> + * @PCI_TSM_PF0: function0 that hosts a DOE mailbox that comprehends an
> + * Interface ID per potential TDI
> + * @PCI_TSM_VIRTFN: physfn-0 of this device is "tsm_pf0"
> + * @PCI_TSM_MFD: function0 of this device is "tsm_pf0"
Double space after "is" seems odd.
> + * @PCI_TSM_DOWNSTREAM: immediate Upstream Port of this device is "tsm_pf0"
> + */
> +enum pci_tsm_type {
> + PCI_TSM_INVALID,
> + PCI_TSM_PF0,
> + PCI_TSM_VIRTFN,
> + PCI_TSM_MFD,
> + PCI_TSM_DOWNSTREAM,
> +};
> +
> +/**
> + * struct pci_tsm - Core TSM context for a given PCIe endpoint
> + * @pdev: indicates the type of pci_tsm object
How does a pci_dev indicate a type? Maybe: Used to distinguish the type of pci_tsm object.
> + * @type: pci_tsm object type to disambiguate PCI_TSM_DOWNSTREAM and PCI_TSM_PF0
> + *
> + * This structure is wrapped by a low level TSM driver and returned by
> + * tsm_ops.probe(), it is freed by tsm_ops.remove(). Depending on
> + * whether @pdev is physical function 0, another physical function, or a
> + * virtual function determines the pci_tsm object type. E.g. see 'struct
> + * pci_tsm_pf0'.
> + */
> +struct pci_tsm {
> + struct pci_dev *pdev;
> + enum pci_tsm_type type;
> +};
> +
> +/**
> + * struct pci_tsm_pf0 - Physical Function 0 TDISP context
Missing tsm and kernel-doc warns if docs are complete.
> + * @state: reflect device initialized, connected, or bound
> + * @lock: protect @state vs pci_tsm_ops invocation
> + * @doe_mb: PCIe Data Object Exchange mailbox
> + */
> +struct pci_tsm_pf0 {
> + struct pci_tsm tsm;
> + enum pci_tsm_state state;
> + struct mutex lock;
> + struct pci_doe_mb *doe_mb;
> +};
