Jonathan Cameron wrote:
> 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?
No, date authored / created, but the Date: tag in these ABI entries is
not generally useful, just going to drop it.
> > +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);
One line works for me.
>
> > +}
> > +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.
Done.
>
> > +
> > + 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()
Turns out in the new version I came to the same conclusion.
> This ordering thing is common enough though that maybe we can just
> not worry about it.
>
> > +
>
> Push whitespace change back to earlier patch.
Lost that whitespace along the way...
>
> > 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.
Ack.
>
>
> > +
> > +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_type is gone in the new version per Aneesh's insight that it can
be derived from other data in the pci_tsm context.
> > + * @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.
Changed to:
@pdev: Back ref to device function, distinguishes type of pci_tsm context.
>
> > + * @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.
Went ahead and added a simple Documentation/drivers-api/pci/tsm.rst that
includes pci-tsm.h and tsm.c. Yes, this warning was still there in the
latest version.
