Re: [PATCH v3 03/13] PCI/TSM: Authenticate devices via platform TSM

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Thu, 15 May 2025 22:47:22 -0700
> Dan Williams <dan.j.williams@intel.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 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@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>
> 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@lists.linux.dev
> > +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.