[RFC,5/5] PCI/TSM: Authenticate devices via platform TSM

The PCIe 6.1 specification, section 11, introduces the Trusted
Execution Environment (TEE) Device Interface Security Protocol (TDISP).
This interface definition builds upon CMA, component measurement and
authentication, and IDE, link integrity and data encryption. It adds
support for establishing virtual functions within a device that can be
assigned 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 device security manager (DSM) and
system software in both a VMM and a VM. 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.

Similar to the PCI core extensions to support CONFIG_PCI_CMA,
CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
attribute, and add more properties + controls in a tsm/ subdirectory of
the PCI device sysfs interface. Unlike CMA that can depend on a local to
the PCI core implementation, PCI_TSM needs to be prepared for late
loading of the platform TSM driver. Consider that the TSM driver may
itself be a PCI driver. Userspace can depend on the common TSM device
uevent to know when the PCI core has TSM services enabled. The PCI
device tsm/ subdirectory is supplemented by the TSM device pci/
directory for platform global TSM properties + controls.

All vendor TSM implementations share the property of asking the VMM to
perform DOE mailbox operations on behalf of the TSM. That common
capability is centralized in PCI core code that invokes an ->exec()
operation callback potentially multiple times to service a given request
(struct pci_tsm_req). Future operations / verbs will be handled
similarly with the "request + exec" model. For now, only "connect" and
"disconnect" are implemented which at a minimum is expected to establish
IDE for the link.

In addition to requests the low-level TSM implementation is notified of
device arrival and departure events so that it can filter devices that
the TSM is not prepared to support, or otherwise setup and teardown
per-device context.

Cc: Wu Hao <hao.wu@intel.com>
Cc: Yilun Xu <yilun.xu@intel.com>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Samuel Ortiz <sameo@rivosinc.com>
Cc: Alexey Kardashevskiy <aik@amd.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-pci   |   43 +++-
 Documentation/ABI/testing/sysfs-class-tsm |   23 ++
 drivers/pci/Kconfig                       |   15 +
 drivers/pci/Makefile                      |    2 
 drivers/pci/cma.c                         |    5 
 drivers/pci/pci-sysfs.c                   |    3 
 drivers/pci/pci.h                         |   14 +
 drivers/pci/probe.c                       |    1 
 drivers/pci/remove.c                      |    1 
 drivers/pci/tsm.c                         |  346 +++++++++++++++++++++++++++++
 drivers/virt/coco/tsm/Makefile            |    1 
 drivers/virt/coco/tsm/class.c             |   22 +-
 drivers/virt/coco/tsm/pci.c               |   83 +++++++
 drivers/virt/coco/tsm/tsm.h               |   28 ++
 include/linux/pci.h                       |    3 
 include/linux/tsm.h                       |   77 ++++++
 include/uapi/linux/pci_regs.h             |    3 
 17 files changed, 662 insertions(+), 8 deletions(-)
 create mode 100644 drivers/pci/tsm.c
 create mode 100644 drivers/virt/coco/tsm/pci.c
 create mode 100644 drivers/virt/coco/tsm/tsm.h

On Tue, Jan 30, 2024 at 01:24:14AM -0800, Dan Williams wrote:
> The PCIe 6.1 specification, section 11, introduces the Trusted
> Execution Environment (TEE) Device Interface Security Protocol (TDISP).
> This interface definition builds upon CMA, component measurement and
> authentication, and IDE, link integrity and data encryption. It adds
> support for establishing virtual functions within a device that can be
> assigned 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 device security manager (DSM) and
> system software in both a VMM and a VM. 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.
> 
> Similar to the PCI core extensions to support CONFIG_PCI_CMA,
> CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
> attribute, and add more properties + controls in a tsm/ subdirectory of
> the PCI device sysfs interface. Unlike CMA that can depend on a local to
> the PCI core implementation, PCI_TSM needs to be prepared for late
> loading of the platform TSM driver. Consider that the TSM driver may
> itself be a PCI driver. Userspace can depend on the common TSM device
> uevent to know when the PCI core has TSM services enabled. The PCI
> device tsm/ subdirectory is supplemented by the TSM device pci/
> directory for platform global TSM properties + controls.
> 
> All vendor TSM implementations share the property of asking the VMM to
> perform DOE mailbox operations on behalf of the TSM. That common
> capability is centralized in PCI core code that invokes an ->exec()
> operation callback potentially multiple times to service a given request
> (struct pci_tsm_req). Future operations / verbs will be handled
> similarly with the "request + exec" model. For now, only "connect" and
> "disconnect" are implemented which at a minimum is expected to establish
> IDE for the link.
> 
> In addition to requests the low-level TSM implementation is notified of
> device arrival and departure events so that it can filter devices that
> the TSM is not prepared to support, or otherwise setup and teardown
> per-device context.

Gulp, this is a good start and covers a lot of what I asked about
[1/5].  Should have read the whole series first ;)

Bjorn Helgaas wrote:
> On Tue, Jan 30, 2024 at 01:24:14AM -0800, Dan Williams wrote:
> > The PCIe 6.1 specification, section 11, introduces the Trusted
> > Execution Environment (TEE) Device Interface Security Protocol (TDISP).
> > This interface definition builds upon CMA, component measurement and
> > authentication, and IDE, link integrity and data encryption. It adds
> > support for establishing virtual functions within a device that can be
> > assigned 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 device security manager (DSM) and
> > system software in both a VMM and a VM. 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.
> > 
> > Similar to the PCI core extensions to support CONFIG_PCI_CMA,
> > CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
> > attribute, and add more properties + controls in a tsm/ subdirectory of
> > the PCI device sysfs interface. Unlike CMA that can depend on a local to
> > the PCI core implementation, PCI_TSM needs to be prepared for late
> > loading of the platform TSM driver. Consider that the TSM driver may
> > itself be a PCI driver. Userspace can depend on the common TSM device
> > uevent to know when the PCI core has TSM services enabled. The PCI
> > device tsm/ subdirectory is supplemented by the TSM device pci/
> > directory for platform global TSM properties + controls.
> > 
> > All vendor TSM implementations share the property of asking the VMM to
> > perform DOE mailbox operations on behalf of the TSM. That common
> > capability is centralized in PCI core code that invokes an ->exec()
> > operation callback potentially multiple times to service a given request
> > (struct pci_tsm_req). Future operations / verbs will be handled
> > similarly with the "request + exec" model. For now, only "connect" and
> > "disconnect" are implemented which at a minimum is expected to establish
> > IDE for the link.
> > 
> > In addition to requests the low-level TSM implementation is notified of
> > device arrival and departure events so that it can filter devices that
> > the TSM is not prepared to support, or otherwise setup and teardown
> > per-device context.
> 
> Gulp, this is a good start and covers a lot of what I asked about
> [1/5].  Should have read the whole series first ;)

I should have at least left a breadcrumb that the acronym soup is better
described in patch5. However, the specifications are certainly dense and
it is funny, in a laugh to keep from crying sort of way, that the
acronyms nest. "What do you mean the T in TDISP stands for TEE!?"

One interesting sidebar I had with Lukas about the naming was an
assertion that "Bjorn will want to call this the TDISP layer not the TSM
layer". The rationale being that the name of the PCIe specification
chapter that talks about the role of a 'TSM' is "Section 11. TEE Device
Interface Security Protocol (TDISP)". However, if there is one point I
want to get across in this posting is that TDISP is a protocol that is
spoken between the platform and the endpoint, i.e. between the TSM and
the DSM. A protocol abstracted away from Linux's view. Everything that
Linux needs to worry about is behind by the OS-to-TSM interface, and the
TDISP specification says next to nothing about what that OS-to-TSM
interface looks like. If it had standardized that, the job would be so
much easier.

So, I think Linux's role here is to act as a "standards body of last
resort" and try to hold platform definitions to a common understanding
of how much complexity is reasonable to export to Linux. Assert that the
per-platform portions intersect Linux at the same level of abstraction.
Basically, please no vendor-specific layering violations sprinkled
around the kernel, and enlighten the PCI core for core concepts (like
authentication). Do not build sidecars.

On 30/1/24 20:24, Dan Williams wrote:
> The PCIe 6.1 specification, section 11, introduces the Trusted
> Execution Environment (TEE) Device Interface Security Protocol (TDISP).
> This interface definition builds upon CMA, component measurement and
> authentication, and IDE, link integrity and data encryption. It adds
> support for establishing virtual functions within a device that can be
> assigned 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 device security manager (DSM) and
> system software in both a VMM and a VM. 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.
> 
> Similar to the PCI core extensions to support CONFIG_PCI_CMA,
> CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
> attribute, and add more properties + controls in a tsm/ subdirectory of
> the PCI device sysfs interface. Unlike CMA that can depend on a local to
> the PCI core implementation, PCI_TSM needs to be prepared for late
> loading of the platform TSM driver. Consider that the TSM driver may
> itself be a PCI driver. Userspace can depend on the common TSM device
> uevent to know when the PCI core has TSM services enabled. The PCI
> device tsm/ subdirectory is supplemented by the TSM device pci/
> directory for platform global TSM properties + controls.
> 
> All vendor TSM implementations share the property of asking the VMM to
> perform DOE mailbox operations on behalf of the TSM. That common
> capability is centralized in PCI core code that invokes an ->exec()
> operation callback potentially multiple times to service a given request
> (struct pci_tsm_req). Future operations / verbs will be handled
> similarly with the "request + exec" model. For now, only "connect" and
> "disconnect" are implemented which at a minimum is expected to establish
> IDE for the link.
> 
> In addition to requests the low-level TSM implementation is notified of
> device arrival and departure events so that it can filter devices that
> the TSM is not prepared to support, or otherwise setup and teardown
> per-device context.


It's a good start but I am still digesting this scaffolding.

> 
> Cc: Wu Hao <hao.wu@intel.com>
> Cc: Yilun Xu <yilun.xu@intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   Documentation/ABI/testing/sysfs-bus-pci   |   43 +++-
>   Documentation/ABI/testing/sysfs-class-tsm |   23 ++
>   drivers/pci/Kconfig                       |   15 +
>   drivers/pci/Makefile                      |    2
>   drivers/pci/cma.c                         |    5
>   drivers/pci/pci-sysfs.c                   |    3
>   drivers/pci/pci.h                         |   14 +
>   drivers/pci/probe.c                       |    1
>   drivers/pci/remove.c                      |    1
>   drivers/pci/tsm.c                         |  346 +++++++++++++++++++++++++++++
>   drivers/virt/coco/tsm/Makefile            |    1
>   drivers/virt/coco/tsm/class.c             |   22 +-
>   drivers/virt/coco/tsm/pci.c               |   83 +++++++
>   drivers/virt/coco/tsm/tsm.h               |   28 ++
>   include/linux/pci.h                       |    3
>   include/linux/tsm.h                       |   77 ++++++
>   include/uapi/linux/pci_regs.h             |    3
>   17 files changed, 662 insertions(+), 8 deletions(-)
>   create mode 100644 drivers/pci/tsm.c
>   create mode 100644 drivers/virt/coco/tsm/pci.c
>   create mode 100644 drivers/virt/coco/tsm/tsm.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 35b0e11fd0e6..0eef2128cf09 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -508,11 +508,16 @@ Description:
>   		This file contains "native" if the device authenticated
>   		successfully with CMA-SPDM (PCIe r6.1 sec 6.31). It contains
>   		"none" if the device failed authentication (and may thus be
> -		malicious).
> +		malicious). It transitions from "native" to "tsm" after
> +		successful connection to a tsm, see the "connect" attribute
> +		below.
>   
>   		Writing "native" to this file causes reauthentication with
>   		kernel-selected keys and the kernel's certificate chain.  That
> -		may be opportune after updating the .cma keyring.
> +		may be opportune after updating the .cma keyring. Note
> +		that once connected to a tsm this returns -EBUSY to attempts to
> +		write "native", i.e. first disconnect from the tsm to retrigger
> +		native authentication.
>   
>   		The file is not visible if authentication is unsupported
>   		by the device.
> @@ -529,3 +534,37 @@ Description:
>   		The reason why authentication support could not be determined
>   		is apparent from "dmesg".  To probe for authentication support
>   		again, exercise the "remove" and "rescan" attributes.
> +
> +What:		/sys/bus/pci/devices/.../tsm/
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		This directory only appears if a device supports CMA and IDE,
> +		and only after a TSM driver has loaded and accepted / setup this
> +		PCI device. Similar to the 'authenticated' attribute, trigger
> +		"remove" and "rescan" to retry the initialization. See
> +		Documentation/ABI/testing/sysfs-class-tsm for enumerating the
> +		platform's TSM capabilities.
> +
> +What:		/sys/bus/pci/devices/.../tsm/connect
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RW) Writing "1" to this file triggers the TSM to establish a
> +		secure connection with the device. This typically includes an
> +		SPDM (DMTF Security Protocols and Data Models) session over PCIe
> +		DOE (Data Object Exchange) and PCIe IDE (Integrity and Data
> +		Encryption) establishment. For TSMs and devices that support
> +		both modes of IDE ("link" and "selective") the "connect_mode"
> +		attribute selects the mode.
> +
> +What:		/sys/bus/pci/devices/.../tsm/connect_mode
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) Returns the available connection modes optionally with
> +		brackets around the currently active mode if the device is
> +		connected. For example it may show "link selective" for a
> +		disconnected device, "link [selective]" for a selective
> +		connected device, and it may hide a mode that is not supported
> +		by the device or TSM.
> diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm
> index 304b50b53e65..77957882738a 100644
> --- a/Documentation/ABI/testing/sysfs-class-tsm
> +++ b/Documentation/ABI/testing/sysfs-class-tsm
> @@ -10,3 +10,26 @@ Description:
>   		For software TSMs instantiated by a software module, @host is a
>   		directory with attributes for that TSM, and those attributes are
>   		documented below.
> +
> +
> +What:		/sys/class/tsm/tsm0/pci/link_capable
> +Date:		January, 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) When present this returns "1\n" to indicate that the TSM
> +		supports establishing Link IDE with a given root-port attached
> +		device. See "tsm/connect_mode" in
> +		Documentation/ABI/testing/sysfs-bus-pci

I am struggling to make sense of "a given root-port attached device".
There is one CCP device on AMD SEV and therefore one /sys/class/tsm/tsmX 
but still many root ports. How do root ports relate to /sys/class/tsm/tsm0 ?


> +
> +
> +What:		/sys/class/tsm/tsm0/pci/selective_streams
> +Date:		January, 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) When present this returns the number of currently available
> +		selective IDE streams available to the TSM. When a stream id is
> +		allocated this number is decremented and a link to the PCI
> +		device(s) consuming the stream(s) appears alonside this

s/alonside/alongside/

> +		attribute in the /sys/class/tsm/tsm0/pci/ directory. See
> +		"tsm/connect" and "tsm/connect_mode" in
> +		Documentation/ABI/testing/sysfs-bus-pci.
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index a5c3cadddd6f..11d788038d19 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -129,6 +129,21 @@ config PCI_CMA
>   	  A PCI DOE mailbox is used as transport for DMTF SPDM based
>   	  authentication, measurement and secure channel establishment.
>   
> +config PCI_TSM
> +	bool "TEE Security Manager for Device Security"

(discussed elsewhere, I'll rant here once more and then will shut up)

It is bool and not tristate :(
CMA, DOE are the same, quite annoying (as in these early days I am 
adding printks here and there and rmmod+modpbobe saves time but builtins 
mean reboot) and imho no really necessary as (from 4/5) "only next 
generation server hosts will start to include a platform TSM".


> +	depends on PCI_CMA
> +	depends on TSM
> +	help
> +	  The TEE (Trusted Execution Environment) Device Interface
> +	  Security Protocol (TDISP) defines a "TSM" as a platform agent
> +	  that manages device authentication, link encryption, link
> +	  integrity protection, and assignment of PCI device functions
> +	  (virtual or physical) to confidential computing VMs that can
> +	  access (DMA) guest private memory.
> +
> +	  Say Y to enable the PCI subsystem to enable the IDE and
> +	  TDISP capabilities of devices via TSM semantics.
> +
>   config PCI_DOE
>   	bool
>   
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index cc8b5d1d15b9..c4117d67ea83 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -38,6 +38,8 @@ obj-$(CONFIG_PCI_CMA)		+= cma.o cma.asn1.o
>   $(obj)/cma.o:			$(obj)/cma.asn1.h
>   $(obj)/cma.asn1.o:		$(obj)/cma.asn1.c $(obj)/cma.asn1.h
>   
> +obj-$(CONFIG_PCI_TSM)		+= tsm.o
> +
>   # Endpoint library must be initialized before its users
>   obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>   
> diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c
> index be7d2bb21b4c..5a69e9919589 100644
> --- a/drivers/pci/cma.c
> +++ b/drivers/pci/cma.c
> @@ -39,6 +39,9 @@ static ssize_t authenticated_store(struct device *dev,
>   	if (!sysfs_streq(buf, "native"))
>   		return -EINVAL;
>   
> +	if (pci_tsm_authenticated(pdev))
> +		return -EBUSY;
> +
>   	rc = pci_cma_reauthenticate(pdev);
>   	if (rc)
>   		return rc;
> @@ -55,6 +58,8 @@ static ssize_t authenticated_show(struct device *dev,
>   	    (pdev->cma_init_failed || pdev->doe_init_failed))
>   		return -ENOTTY;
>   
> +	if (pci_tsm_authenticated(pdev))
> +		return sysfs_emit(buf, "tsm\n");
>   	if (spdm_authenticated(pdev->spdm_state))
>   		return sysfs_emit(buf, "native\n");
>   	return sysfs_emit(buf, "none\n");
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 368c4f71cc55..4327f8c2e6b5 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1654,6 +1654,9 @@ const struct attribute_group *pci_dev_attr_groups[] = {
>   #endif
>   #ifdef CONFIG_PCI_CMA
>   	&pci_cma_attr_group,
> +#endif
> +#ifdef CONFIG_PCI_TSM
> +	&pci_tsm_attr_group,
>   #endif
>   	NULL,
>   };
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2b7d8d0b2e21..daa20866bc90 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -350,6 +350,20 @@ static inline int pci_cma_reauthenticate(struct pci_dev *pdev)
>   }
>   #endif
>   
> +#ifdef CONFIG_PCI_TSM
> +void pci_tsm_init(struct pci_dev *pdev);
> +void pci_tsm_destroy(struct pci_dev *pdev);
> +extern const struct attribute_group pci_tsm_attr_group;
> +bool pci_tsm_authenticated(struct pci_dev *pdev);
> +#else
> +static inline void pci_tsm_init(struct pci_dev *pdev) { }
> +static inline void pci_tsm_destroy(struct pci_dev *pdev) { }
> +static inline bool pci_tsm_authenticated(struct pci_dev *pdev)
> +{
> +	return false;
> +}
> +#endif
> +
>   /**
>    * pci_dev_set_io_state - Set the new error state if possible.
>    *
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6b09c962c0b8..f60d6c3c8c48 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2542,6 +2542,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
>   	pci_rcec_init(dev);		/* Root Complex Event Collector */
>   	pci_doe_init(dev);		/* Data Object Exchange */
>   	pci_cma_init(dev);		/* Component Measurement & Auth */
> +	pci_tsm_init(dev);		/* TEE Security Manager connection */
>   
>   	pcie_report_downtraining(dev);
>   	pci_init_reset_methods(dev);
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index f009ac578997..228fa6ccf911 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -39,6 +39,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
>   	list_del(&dev->bus_list);
>   	up_write(&pci_bus_sem);
>   
> +	pci_tsm_destroy(dev);
>   	pci_cma_destroy(dev);
>   	pci_doe_destroy(dev);
>   	pcie_aspm_exit_link_state(dev);
> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> new file mode 100644
> index 000000000000..f74de0ee49a0
> --- /dev/null
> +++ b/drivers/pci/tsm.c
> @@ -0,0 +1,346 @@
> +// 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/pci.h>
> +#include <linux/tsm.h>
> +#include <linux/sysfs.h>
> +#include <linux/xarray.h>
> +#include "pci.h"
> +
> +/* collect tsm capable devices to rendezvous with the tsm driver */
> +static DEFINE_XARRAY(pci_tsm_devs);

Not used anywhere.


> +
> +/*
> + * Provide a read/write lock against the init / exit of pdev tsm
> + * capabilities and arrival/departure of a tsm instance
> + */
> +static DECLARE_RWSEM(pci_tsm_rwsem);
> +static const struct tsm_pci_ops *tsm_ops;
> +
> +void generic_pci_tsm_req_free(struct pci_tsm_req *req)
> +{
> +	kfree(req);
> +}
> +EXPORT_SYMBOL_GPL(generic_pci_tsm_req_free);
> +
> +struct pci_tsm_req *generic_pci_tsm_req_alloc(struct pci_dev *pdev, enum pci_tsm_op op)
> +{
> +	struct pci_tsm_req *req = kzalloc(sizeof(*req), GFP_KERNEL);
> +
> +	if (!req)
> +		return NULL;
> +	req->op = op;
> +	return req;
> +}
> +EXPORT_SYMBOL_GPL(generic_pci_tsm_req_alloc);
> +
> +DEFINE_FREE(req_free, struct pci_tsm_req *, if (_T) tsm_ops->req_free(_T))
> +
> +static int pci_tsm_disconnect(struct pci_dev *pdev)
> +{
> +	struct pci_tsm_req *req __free(req_free) = NULL;
> +
> +	/* opportunistic state checks to skip allocating a request */
> +	if (pdev->tsm->state < PCI_TSM_CONNECT)
> +		return 0;
> +
> +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_DISCONNECT);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
> +		enum pci_tsm_op_status status;
> +
> +		/* revalidate state */
> +		if (pdev->tsm->state < PCI_TSM_CONNECT)
> +			return 0;
> +		if (pdev->tsm->state < PCI_TSM_INIT)
> +			return -ENXIO;
> +
> +		do {
> +			status = tsm_ops->exec(pdev, req);
> +			req->seq++;
> +			/* TODO: marshal SPDM request */
> +		} while (status == PCI_TSM_SPDM_REQ);
> +
> +		if (status == PCI_TSM_FAIL)
> +			return -EIO;
> +		pdev->tsm->state = PCI_TSM_INIT;
> +	}
> +	return 0;
> +}
> +
> +static int pci_tsm_connect(struct pci_dev *pdev)
> +{
> +	struct pci_tsm_req *req __free(req_free) = NULL;
> +
> +	/* opportunistic state checks to skip allocating a request */
> +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +		return 0;
> +
> +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_CONNECT);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
> +		enum pci_tsm_op_status status;
> +
> +		/* revalidate state */
> +		if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +			return 0;
> +		if (pdev->tsm->state < PCI_TSM_INIT)
> +			return -ENXIO;
> +
> +		do {
> +			status = tsm_ops->exec(pdev, req);
> +			req->seq++;
> +		} while (status == PCI_TSM_SPDM_REQ);
> +
> +		if (status == PCI_TSM_FAIL)
> +			return -EIO;
> +		pdev->tsm->state = PCI_TSM_CONNECT;
> +	}
> +	return 0;
> +}
> +
> +static ssize_t connect_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t len)
> +{
> +	bool connect;
> +	int rc = kstrtobool(buf, &connect);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (rc)
> +		return rc;
> +
> +	if (connect) {
> +		if (!spdm_authenticated(pdev->spdm_state)) {
> +			pci_dbg(pdev, "SPDM authentication pre-requisite not met.\n");
> +			return -ENXIO;
> +		}
> +		rc = pci_tsm_connect(pdev);
> +		if (rc)
> +			return rc;
> +		return len;
> +	}
> +
> +	rc = pci_tsm_disconnect(pdev);
> +	if (rc)
> +		return rc;
> +	return len;
> +}
> +
> +static ssize_t connect_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sysfs_emit(buf, "%d\n", pdev->tsm->state >= PCI_TSM_CONNECT);
> +}
> +static DEVICE_ATTR_RW(connect);
> +
> +static const char *const pci_tsm_modes[] = {
> +	[PCI_TSM_MODE_LINK] = "link",
> +	[PCI_TSM_MODE_SELECTIVE] = "selective",
> +};
> +
> +static ssize_t connect_mode_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t len)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int i;
> +
> +	guard(mutex)(tsm_ops->lock);
> +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +		return -EBUSY;
> +	for (i = 0; i < ARRAY_SIZE(pci_tsm_modes); i++)
> +		if (sysfs_streq(buf, pci_tsm_modes[i]))
> +			break;
> +	if (i == PCI_TSM_MODE_LINK) {
> +		if (pdev->tsm->link_capable)
> +			pdev->tsm->mode = PCI_TSM_MODE_LINK;
> +		return -EOPNOTSUPP;
> +	} else if (i == PCI_TSM_MODE_SELECTIVE) {
> +		if (pdev->tsm->selective_capable)
> +			pdev->tsm->mode = PCI_TSM_MODE_SELECTIVE;
> +		return -EOPNOTSUPP;
> +	} else
> +		return -EINVAL;
> +	return len;
> +}
> +
> +static ssize_t connect_mode_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	ssize_t count = 0;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pci_tsm_modes); i++) {
> +		if (i == PCI_TSM_MODE_LINK) {
> +			if (!pdev->tsm->link_capable)
> +				continue;
> +		} else if (i == PCI_TSM_MODE_SELECTIVE) {
> +			if (!pdev->tsm->selective_capable)
> +				continue;
> +		}
> +
> +		if (i == pdev->tsm->mode)
> +			count += sysfs_emit_at(buf, count, "[%s] ",
> +					       pci_tsm_modes[i]);
> +		else
> +			count += sysfs_emit_at(buf, count, "%s ",
> +					       pci_tsm_modes[i]);
> +	}
> +
> +	if (count)
> +		buf[count - 1] = '\n';
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(connect_mode);
> +
> +static umode_t pci_tsm_attr_visible(struct kobject *kobj, struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (a == &dev_attr_connect_mode.attr) {
> +		if (pdev->tsm->link_capable || pdev->tsm->selective_capable)
> +			return a->mode;
> +		return 0;
> +	}
> +
> +	return a->mode;
> +}
> +
> +static bool pci_tsm_group_visible(struct kobject *kobj)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (pdev->tsm && pdev->tsm->state > PCI_TSM_IDLE)
> +		return true;
> +	return false;
> +}
> +DEFINE_SYSFS_GROUP_VISIBLE(pci_tsm);
> +
> +static struct attribute *pci_tsm_attrs[] = {
> +	&dev_attr_connect.attr,
> +	&dev_attr_connect_mode.attr,
> +	NULL,
> +};
> +
> +const struct attribute_group pci_tsm_attr_group = {
> +	.name = "tsm",
> +	.attrs = pci_tsm_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(pci_tsm),
> +};
> +
> +static int pci_tsm_add(struct pci_dev *pdev)

Nothing checks the returned value.

> +{
> +	lockdep_assert_held(&pci_tsm_rwsem);
> +	if (!tsm_ops)
> +		return 0;
> +	scoped_guard(mutex, tsm_ops->lock) {
> +		if (pdev->tsm->state < PCI_TSM_INIT) {
> +			int rc = tsm_ops->add(pdev);
> +
> +			if (rc)
> +				return rc;
> +		}
> +		pdev->tsm->state = PCI_TSM_INIT;
> +	}
> +	return sysfs_update_group(&pdev->dev.kobj, &pci_tsm_attr_group);
> +}
> +
> +static void pci_tsm_del(struct pci_dev *pdev)
> +{
> +	lockdep_assert_held(&pci_tsm_rwsem);
> +	/* shutdown sysfs operations before tsm delete */
> +	pdev->tsm->state = PCI_TSM_IDLE;
> +	sysfs_update_group(&pdev->dev.kobj, &pci_tsm_attr_group);
> +	guard(mutex)(tsm_ops->lock);
> +	tsm_ops->del(pdev);
> +}
> +
> +int pci_tsm_register(const struct tsm_pci_ops *ops)
> +{
> +	struct pci_dev *pdev;
> +	unsigned long index;
> +
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	if (tsm_ops)
> +		return -EBUSY;
> +	tsm_ops = ops;
> +	xa_for_each(&pci_tsm_devs, index, pdev)
> +		pci_tsm_add(pdev);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_tsm_register);
> +
> +void pci_tsm_unregister(const struct tsm_pci_ops *ops)
> +{
> +	struct pci_dev *pdev;
> +	unsigned long index;
> +
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	if (ops != tsm_ops)
> +		return;
> +	xa_for_each(&pci_tsm_devs, index, pdev)
> +		pci_tsm_del(pdev);
> +	tsm_ops = NULL;
> +}
> +EXPORT_SYMBOL_GPL(pci_tsm_unregister);
> +
> +void pci_tsm_init(struct pci_dev *pdev)
> +{
> +	u16 ide_cap;
> +	int rc;
> +
> +	if (!pdev->cma_capable)
> +		return;
> +
> +	ide_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_IDE);
> +	if (!ide_cap)
> +		return;
> +
> +	struct pci_tsm *tsm __free(kfree) = kzalloc(sizeof(*tsm), GFP_KERNEL);
> +	if (!tsm)
> +		return;
> +
> +	tsm->ide_cap = ide_cap;
> +
> +	rc = xa_insert(&pci_tsm_devs, (unsigned long)pdev, pdev, GFP_KERNEL);
> +	if (rc) {
> +		pci_dbg(pdev, "failed to register tsm capable device\n");
> +		return;
> +	}
> +
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	pdev->tsm = no_free_ptr(tsm);
> +	pci_tsm_add(pdev);
> +}
> +
> +void pci_tsm_destroy(struct pci_dev *pdev)
> +{
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	pci_tsm_del(pdev);
> +	xa_erase(&pci_tsm_devs, (unsigned long)pdev);
> +	kfree(pdev->tsm);
> +	pdev->tsm = NULL;
> +}
> +
> +bool pci_tsm_authenticated(struct pci_dev *pdev)
> +{
> +	guard(rwsem_read)(&pci_tsm_rwsem);
> +	return pdev->tsm && pdev->tsm->state >= PCI_TSM_CONNECT;
> +}
> diff --git a/drivers/virt/coco/tsm/Makefile b/drivers/virt/coco/tsm/Makefile
> index f7561169faed..a4f0d07d7d97 100644
> --- a/drivers/virt/coco/tsm/Makefile
> +++ b/drivers/virt/coco/tsm/Makefile
> @@ -7,3 +7,4 @@ tsm_reports-y := reports.o
>   
>   obj-$(CONFIG_TSM) += tsm.o
>   tsm-y := class.o
> +tsm-$(CONFIG_PCI_TSM) += pci.o
> diff --git a/drivers/virt/coco/tsm/class.c b/drivers/virt/coco/tsm/class.c
> index a569fa6b09eb..a459e51c0892 100644
> --- a/drivers/virt/coco/tsm/class.c
> +++ b/drivers/virt/coco/tsm/class.c
> @@ -8,13 +8,11 @@
>   #include <linux/device.h>
>   #include <linux/module.h>
>   #include <linux/cleanup.h>
> +#include "tsm.h"
>   
>   static DECLARE_RWSEM(tsm_core_rwsem);
> -struct class *tsm_class;
> -struct tsm_subsys {
> -	struct device dev;
> -	const struct tsm_info *info;
> -} *tsm_subsys;
> +static struct class *tsm_class;
> +static struct tsm_subsys *tsm_subsys;
>   
>   int tsm_register(const struct tsm_info *info)
>   {
> @@ -52,6 +50,10 @@ int tsm_register(const struct tsm_info *info)
>   	dev = NULL;
>   	tsm_subsys = subsys;
>   
> +	rc = tsm_pci_init(info);
> +	if (rc)
> +		pr_err("PCI initialization failure: %d\n", rc);
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(tsm_register);
> @@ -65,6 +67,8 @@ void tsm_unregister(const struct tsm_info *info)
>   		return;
>   	}
>   
> +	tsm_pci_destroy(info);
> +
>   	if (info->host)
>   		sysfs_remove_link(&tsm_subsys->dev.kobj, "host");
>   	device_unregister(&tsm_subsys->dev);
> @@ -79,6 +83,13 @@ static void tsm_release(struct device *dev)
>   	kfree(subsys);
>   }
>   
> +static const struct attribute_group *tsm_attr_groups[] = {
> +#ifdef CONFIG_PCI_TSM
> +	&tsm_pci_attr_group,
> +#endif
> +	NULL,
> +};
> +
>   static int __init tsm_init(void)
>   {
>   	tsm_class = class_create("tsm");
> @@ -86,6 +97,7 @@ static int __init tsm_init(void)
>   		return PTR_ERR(tsm_class);
>   
>   	tsm_class->dev_release = tsm_release;
> +	tsm_class->dev_groups = tsm_attr_groups;
>   	return 0;
>   }
>   module_init(tsm_init)
> diff --git a/drivers/virt/coco/tsm/pci.c b/drivers/virt/coco/tsm/pci.c
> new file mode 100644
> index 000000000000..b3684ad7114f
> --- /dev/null
> +++ b/drivers/virt/coco/tsm/pci.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/tsm.h>
> +#include <linux/device.h>
> +#include "tsm.h"
> +
> +static ssize_t link_capable_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct tsm_subsys *subsys = container_of(dev, typeof(*subsys), dev);
> +
> +	return sysfs_emit(buf, "%u\n", subsys->info->link_stream_capable);
> +}
> +static DEVICE_ATTR_RO(link_capable);
> +
> +static ssize_t selective_streams_show(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct tsm_subsys *subsys = container_of(dev, typeof(*subsys), dev);
> +
> +	return sysfs_emit(buf, "%u\n", subsys->info->nr_selective_streams);
> +}
> +static DEVICE_ATTR_RO(selective_streams);
> +
> +static umode_t tsm_pci_attr_visible(struct kobject *kobj, struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct tsm_subsys *subsys = container_of(dev, typeof(*subsys), dev);
> +	const struct tsm_info *info = subsys->info;
> +
> +	if (a == &dev_attr_link_capable.attr) {
> +		if (info->link_stream_capable)
> +			return a->mode;
> +		return 0;
> +	}
> +
> +	if (a == &dev_attr_selective_streams.attr) {
> +		if (info->nr_selective_streams)
> +			return a->mode;
> +		return 0;
> +	}
> +
> +	return a->mode;
> +}
> +
> +static bool tsm_pci_group_visible(struct kobject *kobj)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct tsm_subsys *subsys = container_of(dev, typeof(*subsys), dev);
> +
> +	if (subsys->info->pci_ops)
> +		return true;
> +	return false;
> +}
> +DEFINE_SYSFS_GROUP_VISIBLE(tsm_pci);
> +
> +static struct attribute *tsm_pci_attrs[] = {
> +	&dev_attr_link_capable.attr,
> +	&dev_attr_selective_streams.attr,
> +	NULL,
> +};
> +
> +const struct attribute_group tsm_pci_attr_group = {
> +	.name = "pci",
> +	.attrs = tsm_pci_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(tsm_pci),
> +};
> +
> +int tsm_pci_init(const struct tsm_info *info)
> +{
> +	if (!info->pci_ops)
> +		return 0;
> +
> +	return pci_tsm_register(info->pci_ops);
> +}
> +
> +void tsm_pci_destroy(const struct tsm_info *info)
> +{
> +	pci_tsm_unregister(info->pci_ops);
> +}
> diff --git a/drivers/virt/coco/tsm/tsm.h b/drivers/virt/coco/tsm/tsm.h
> new file mode 100644
> index 000000000000..407c388a109b
> --- /dev/null
> +++ b/drivers/virt/coco/tsm/tsm.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __TSM_CORE_H
> +#define __TSM_CORE_H
> +
> +#include <linux/device.h>
> +
> +struct tsm_info;
> +struct tsm_subsys {
> +	struct device dev;
> +	const struct tsm_info *info;
> +};

Have not you just defined this in 3/5? :)

> +
> +#ifdef CONFIG_PCI_TSM
> +int tsm_pci_init(const struct tsm_info *info);
> +void tsm_pci_destroy(const struct tsm_info *info);
> +extern const struct attribute_group tsm_pci_attr_group;
> +#else
> +static inline int tsm_pci_init(const struct tsm_info *info)
> +{
> +	return 0;
> +}
> +static inline void tsm_pci_destroy(const struct tsm_info *info)
> +{
> +}
> +#endif
> +
> +#endif /* TSM_CORE_H */
> +
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 4a04ce7685e7..132962b21e04 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -522,6 +522,9 @@ struct pci_dev {
>   	struct spdm_state *spdm_state;	/* Security Protocol and Data Model */
>   	unsigned int	cma_capable:1;	/* Authentication supported */
>   	unsigned int	cma_init_failed:1;
> +#endif
> +#ifdef CONFIG_PCI_TSM
> +	struct pci_tsm *tsm;		/* TSM operation state */
>   #endif
>   	u16		acs_cap;	/* ACS Capability offset */
>   	phys_addr_t	rom;		/* Physical address if not from BAR */
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index 8cb8a661ba41..f5dbdfa65d8d 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -4,11 +4,15 @@
>   
>   #include <linux/sizes.h>
>   #include <linux/types.h>
> +#include <linux/mutex.h>
>   
>   struct tsm_info {
>   	const char *name;
>   	struct device *host;
>   	const struct attribute_group **groups;
> +	const struct tsm_pci_ops *pci_ops;
> +	unsigned int nr_selective_streams;
> +	unsigned int link_stream_capable:1;
>   };
>   
>   #define TSM_REPORT_INBLOB_MAX 64
> @@ -74,4 +78,77 @@ int tsm_report_register(const struct tsm_report_ops *ops, void *priv,
>   int tsm_report_unregister(const struct tsm_report_ops *ops);
>   int tsm_register(const struct tsm_info *info);
>   void tsm_unregister(const struct tsm_info *info);
> +
> +enum pci_tsm_op_status {
> +	PCI_TSM_FAIL = -1,
> +	PCI_TSM_OK,
> +	PCI_TSM_SPDM_REQ,

Secure SPDM is also needed here. In my toy TSM project [1] I am just 
using negatives for errors, 0 for "successfully finished" and positives 
for a DOE protocol (1 for SPDM, 2 for Secure SPDM), seems alright as it 
is all about PCI anyway (although "pci" is not always present in all 
these enums and structs).


> +};
> +
> +enum pci_tsm_op {
> +	PCI_TSM_OP_CONNECT,
> +	PCI_TSM_OP_DISCONNECT,
> +};
> +
> +struct pci_tsm_req {
> +	enum pci_tsm_op op;
> +	unsigned int seq;

@seq is not tested anywhere.

May be move (*req_free) here.


> +};
> +
> +struct pci_dev;
> +/**
> + * struct tsm_pci_ops - Low-level TSM-exported interface to the PCI core
> + * @add: accept device for tsm operation, locked
> + * @del: teardown tsm context for @pdev, locked
> + * @req_alloc: setup context for given operation, unlocked
> + * @req_free: teardown context for given request, unlocked
> + * @exec: run @req, may be invoked multiple times per @req, locked
> + * @lock: tsm work is one device and one op at a time
> + */
> +struct tsm_pci_ops {
> +	int (*add)(struct pci_dev *pdev);
> +	void (*del)(struct pci_dev *pdev);
> +	struct pci_tsm_req *(*req_alloc)(struct pci_dev *pdev,
> +					 enum pci_tsm_op op);
> +	struct pci_tsm_req *(*req_free)(struct pci_tsm_req *req);
> +	enum pci_tsm_op_status (*exec)(struct pci_dev *pdev,
> +				       struct pci_tsm_req *req);

The pci_tsm_req is just an @op, three hooks seems to be more than 
needed, could be just (*exec)(struct pci_dev *pdev, enum pci_tsm_op op).

Or the idea is to extend pci_tsm_req with some void *platform_req_data, 
is not it? There is one "op" in flight per a physical device allowed in 
SEV TIO, I suspect that is likely to be the case for others so such data 
can be managed by the platform code in the platform data of a TEE-IO device.


> +	struct mutex *lock;
> +};
> +
> +enum pci_tsm_state {
> +	PCI_TSM_IDLE,
> +	PCI_TSM_INIT,
> +	PCI_TSM_CONNECT,
> +};
> +
> +enum pci_tsm_mode {
> +	PCI_TSM_MODE_LINK,
> +	PCI_TSM_MODE_SELECTIVE,
> +};
> +
> +struct pci_tsm {
> +	enum pci_tsm_state state;
> +	enum pci_tsm_mode mode;

Does it have to be either mode and cannot be both?

> +	u16 ide_cap;
> +	unsigned int link_capable:1;
> +	unsigned int selective_capable:1;
> +	void *tsm_data;
> +};
> +
> +#ifdef CONFIG_PCI_TSM
> +int pci_tsm_register(const struct tsm_pci_ops *ops);
> +void pci_tsm_unregister(const struct tsm_pci_ops *ops);
> +void generic_pci_tsm_req_free(struct pci_tsm_req *req);
> +struct pci_tsm_req *generic_pci_tsm_req_alloc(struct pci_dev *pdev,
> +					      enum pci_tsm_op op);
> +#else
> +static inline int pci_tsm_register(const struct tsm_pci_ops *ops)
> +{
> +	return 0;
> +}
> +static inline void pci_tsm_unregister(const struct tsm_pci_ops *ops)
> +{
> +}
> +#endif
>   #endif /* __TSM_H */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index a39193213ff2..1219d50f8e89 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -742,7 +742,8 @@
>   #define PCI_EXT_CAP_ID_PL_16GT	0x26	/* Physical Layer 16.0 GT/s */
>   #define PCI_EXT_CAP_ID_PL_32GT  0x2A    /* Physical Layer 32.0 GT/s */
>   #define PCI_EXT_CAP_ID_DOE	0x2E	/* Data Object Exchange */
> -#define PCI_EXT_CAP_ID_MAX	PCI_EXT_CAP_ID_DOE
> +#define PCI_EXT_CAP_ID_IDE	0x30	/* Integrity and Data Encryption */
> +#define PCI_EXT_CAP_ID_MAX	PCI_EXT_CAP_ID_IDE
>   
>   #define PCI_EXT_CAP_DSN_SIZEOF	12
>   #define PCI_EXT_CAP_MCAST_ENDPOINT_SIZEOF 40
> 


[1] https://github.com/AMDESE/linux-kvm/commits/tio  <- this fella 
recently moved from my personal account

On 30/1/24 20:24, Dan Williams wrote:
> The PCIe 6.1 specification, section 11, introduces the Trusted
> Execution Environment (TEE) Device Interface Security Protocol (TDISP).
> This interface definition builds upon CMA, component measurement and
> authentication, and IDE, link integrity and data encryption. It adds
> support for establishing virtual functions within a device that can be
> assigned 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 device security manager (DSM) and
> system software in both a VMM and a VM. 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.
> 
> Similar to the PCI core extensions to support CONFIG_PCI_CMA,
> CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
> attribute, and add more properties + controls in a tsm/ subdirectory of
> the PCI device sysfs interface. Unlike CMA that can depend on a local to
> the PCI core implementation, PCI_TSM needs to be prepared for late
> loading of the platform TSM driver. Consider that the TSM driver may
> itself be a PCI driver. Userspace can depend on the common TSM device
> uevent to know when the PCI core has TSM services enabled. The PCI
> device tsm/ subdirectory is supplemented by the TSM device pci/
> directory for platform global TSM properties + controls.
> 
> All vendor TSM implementations share the property of asking the VMM to
> perform DOE mailbox operations on behalf of the TSM. That common
> capability is centralized in PCI core code that invokes an ->exec()
> operation callback potentially multiple times to service a given request
> (struct pci_tsm_req). Future operations / verbs will be handled
> similarly with the "request + exec" model. For now, only "connect" and
> "disconnect" are implemented which at a minimum is expected to establish
> IDE for the link.
> 
> In addition to requests the low-level TSM implementation is notified of
> device arrival and departure events so that it can filter devices that
> the TSM is not prepared to support, or otherwise setup and teardown
> per-device context.
> 
> Cc: Wu Hao <hao.wu@intel.com>
> Cc: Yilun Xu <yilun.xu@intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   Documentation/ABI/testing/sysfs-bus-pci   |   43 +++-
>   Documentation/ABI/testing/sysfs-class-tsm |   23 ++
>   drivers/pci/Kconfig                       |   15 +
>   drivers/pci/Makefile                      |    2
>   drivers/pci/cma.c                         |    5
>   drivers/pci/pci-sysfs.c                   |    3
>   drivers/pci/pci.h                         |   14 +
>   drivers/pci/probe.c                       |    1
>   drivers/pci/remove.c                      |    1
>   drivers/pci/tsm.c                         |  346 +++++++++++++++++++++++++++++
>   drivers/virt/coco/tsm/Makefile            |    1
>   drivers/virt/coco/tsm/class.c             |   22 +-
>   drivers/virt/coco/tsm/pci.c               |   83 +++++++
>   drivers/virt/coco/tsm/tsm.h               |   28 ++
>   include/linux/pci.h                       |    3
>   include/linux/tsm.h                       |   77 ++++++
>   include/uapi/linux/pci_regs.h             |    3
>   17 files changed, 662 insertions(+), 8 deletions(-)
>   create mode 100644 drivers/pci/tsm.c
>   create mode 100644 drivers/virt/coco/tsm/pci.c
>   create mode 100644 drivers/virt/coco/tsm/tsm.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 35b0e11fd0e6..0eef2128cf09 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -508,11 +508,16 @@ Description:
>   		This file contains "native" if the device authenticated
>   		successfully with CMA-SPDM (PCIe r6.1 sec 6.31). It contains
>   		"none" if the device failed authentication (and may thus be
> -		malicious).
> +		malicious). It transitions from "native" to "tsm" after
> +		successful connection to a tsm, see the "connect" attribute
> +		below.
>   
>   		Writing "native" to this file causes reauthentication with
>   		kernel-selected keys and the kernel's certificate chain.  That
> -		may be opportune after updating the .cma keyring.
> +		may be opportune after updating the .cma keyring. Note
> +		that once connected to a tsm this returns -EBUSY to attempts to
> +		write "native", i.e. first disconnect from the tsm to retrigger
> +		native authentication.
>   
>   		The file is not visible if authentication is unsupported
>   		by the device.
> @@ -529,3 +534,37 @@ Description:
>   		The reason why authentication support could not be determined
>   		is apparent from "dmesg".  To probe for authentication support
>   		again, exercise the "remove" and "rescan" attributes.
> +
> +What:		/sys/bus/pci/devices/.../tsm/
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		This directory only appears if a device supports CMA and IDE,
> +		and only after a TSM driver has loaded and accepted / setup this
> +		PCI device. Similar to the 'authenticated' attribute, trigger
> +		"remove" and "rescan" to retry the initialization. See
> +		Documentation/ABI/testing/sysfs-class-tsm for enumerating the
> +		platform's TSM capabilities.
> +
> +What:		/sys/bus/pci/devices/.../tsm/connect
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RW) Writing "1" to this file triggers the TSM to establish a
> +		secure connection with the device. This typically includes an
> +		SPDM (DMTF Security Protocols and Data Models) session over PCIe
> +		DOE (Data Object Exchange) and PCIe IDE (Integrity and Data
> +		Encryption) establishment. For TSMs and devices that support
> +		both modes of IDE ("link" and "selective") the "connect_mode"
> +		attribute selects the mode.
> +
> +What:		/sys/bus/pci/devices/.../tsm/connect_mode
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) Returns the available connection modes optionally with
> +		brackets around the currently active mode if the device is
> +		connected. For example it may show "link selective" for a
> +		disconnected device, "link [selective]" for a selective
> +		connected device, and it may hide a mode that is not supported
> +		by the device or TSM.
> diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm
> index 304b50b53e65..77957882738a 100644
> --- a/Documentation/ABI/testing/sysfs-class-tsm
> +++ b/Documentation/ABI/testing/sysfs-class-tsm
> @@ -10,3 +10,26 @@ Description:
>   		For software TSMs instantiated by a software module, @host is a
>   		directory with attributes for that TSM, and those attributes are
>   		documented below.
> +
> +
> +What:		/sys/class/tsm/tsm0/pci/link_capable
> +Date:		January, 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) When present this returns "1\n" to indicate that the TSM
> +		supports establishing Link IDE with a given root-port attached
> +		device. See "tsm/connect_mode" in
> +		Documentation/ABI/testing/sysfs-bus-pci
> +
> +
> +What:		/sys/class/tsm/tsm0/pci/selective_streams
> +Date:		January, 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) When present this returns the number of currently available
> +		selective IDE streams available to the TSM. When a stream id is
> +		allocated this number is decremented and a link to the PCI
> +		device(s) consuming the stream(s) appears alonside this
> +		attribute in the /sys/class/tsm/tsm0/pci/ directory. See
> +		"tsm/connect" and "tsm/connect_mode" in
> +		Documentation/ABI/testing/sysfs-bus-pci.
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index a5c3cadddd6f..11d788038d19 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -129,6 +129,21 @@ config PCI_CMA
>   	  A PCI DOE mailbox is used as transport for DMTF SPDM based
>   	  authentication, measurement and secure channel establishment.
>   
> +config PCI_TSM
> +	bool "TEE Security Manager for Device Security"
> +	depends on PCI_CMA
> +	depends on TSM
> +	help
> +	  The TEE (Trusted Execution Environment) Device Interface
> +	  Security Protocol (TDISP) defines a "TSM" as a platform agent
> +	  that manages device authentication, link encryption, link
> +	  integrity protection, and assignment of PCI device functions
> +	  (virtual or physical) to confidential computing VMs that can
> +	  access (DMA) guest private memory.
> +
> +	  Say Y to enable the PCI subsystem to enable the IDE and
> +	  TDISP capabilities of devices via TSM semantics.
> +
>   config PCI_DOE
>   	bool
>   
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index cc8b5d1d15b9..c4117d67ea83 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -38,6 +38,8 @@ obj-$(CONFIG_PCI_CMA)		+= cma.o cma.asn1.o
>   $(obj)/cma.o:			$(obj)/cma.asn1.h
>   $(obj)/cma.asn1.o:		$(obj)/cma.asn1.c $(obj)/cma.asn1.h
>   
> +obj-$(CONFIG_PCI_TSM)		+= tsm.o
> +
>   # Endpoint library must be initialized before its users
>   obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>   
> diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c
> index be7d2bb21b4c..5a69e9919589 100644
> --- a/drivers/pci/cma.c
> +++ b/drivers/pci/cma.c
> @@ -39,6 +39,9 @@ static ssize_t authenticated_store(struct device *dev,
>   	if (!sysfs_streq(buf, "native"))
>   		return -EINVAL;
>   
> +	if (pci_tsm_authenticated(pdev))
> +		return -EBUSY;
> +
>   	rc = pci_cma_reauthenticate(pdev);
>   	if (rc)
>   		return rc;

btw is this "native" CMA expected to migrate to tsm_pci_ops? Thanks,

On Tue, 30 Jan 2024 01:24:14 -0800
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 interface definition builds upon CMA, component
> measurement and authentication, and IDE, link integrity and data
> encryption. It adds support for establishing virtual functions within
> a device that can be assigned 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 device security
> manager (DSM) and system software in both a VMM and a VM. 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.
> 
> Similar to the PCI core extensions to support CONFIG_PCI_CMA,
> CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
> attribute, and add more properties + controls in a tsm/ subdirectory
> of the PCI device sysfs interface. Unlike CMA that can depend on a
> local to the PCI core implementation, PCI_TSM needs to be prepared
> for late loading of the platform TSM driver. Consider that the TSM
> driver may itself be a PCI driver. Userspace can depend on the common
> TSM device uevent to know when the PCI core has TSM services enabled.
> The PCI device tsm/ subdirectory is supplemented by the TSM device
> pci/ directory for platform global TSM properties + controls.
> 
> All vendor TSM implementations share the property of asking the VMM to
> perform DOE mailbox operations on behalf of the TSM. That common
> capability is centralized in PCI core code that invokes an ->exec()
> operation callback potentially multiple times to service a given
> request (struct pci_tsm_req). Future operations / verbs will be
> handled similarly with the "request + exec" model. For now, only
> "connect" and "disconnect" are implemented which at a minimum is
> expected to establish IDE for the link.
> 
> In addition to requests the low-level TSM implementation is notified
> of device arrival and departure events so that it can filter devices
> that the TSM is not prepared to support, or otherwise setup and
> teardown per-device context.
> 

Hey Dan,

1) What is the expectation of using the device connect and disconnect
in the guest-driven secure I/O enlightenment? In the last device
security meeting, you said the sysfs interface was mostly for higher
level software stacks, like virt-manager. I was wondering what would be
the picture looks like when coping these statement with the
guest-driven model. Are we expecting the device connect triggered by
QEMU when extracting the guest request from the secure channel in
this case?

2) How does the device-specific logic fit into the new TSM
services? E.g. when the TDISP connect is triggered by userspace, a
device needs to perform quirks before/after/inside the verbs, or a
device needs an interface to tell TSM service when it is able to
response to some verbs. Do you think we needs to have a set of
callbacks from the device side for the PCI TSM service to call?

Thanks,
Zhi.

> Cc: Wu Hao <hao.wu@intel.com>
> Cc: Yilun Xu <yilun.xu@intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci   |   43 +++-
>  Documentation/ABI/testing/sysfs-class-tsm |   23 ++
>  drivers/pci/Kconfig                       |   15 +
>  drivers/pci/Makefile                      |    2 
>  drivers/pci/cma.c                         |    5 
>  drivers/pci/pci-sysfs.c                   |    3 
>  drivers/pci/pci.h                         |   14 +
>  drivers/pci/probe.c                       |    1 
>  drivers/pci/remove.c                      |    1 
>  drivers/pci/tsm.c                         |  346
> +++++++++++++++++++++++++++++ drivers/virt/coco/tsm/Makefile
>   |    1 drivers/virt/coco/tsm/class.c             |   22 +-
>  drivers/virt/coco/tsm/pci.c               |   83 +++++++
>  drivers/virt/coco/tsm/tsm.h               |   28 ++
>  include/linux/pci.h                       |    3 
>  include/linux/tsm.h                       |   77 ++++++
>  include/uapi/linux/pci_regs.h             |    3 
>  17 files changed, 662 insertions(+), 8 deletions(-)
>  create mode 100644 drivers/pci/tsm.c
>  create mode 100644 drivers/virt/coco/tsm/pci.c
>  create mode 100644 drivers/virt/coco/tsm/tsm.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci
> b/Documentation/ABI/testing/sysfs-bus-pci index
> 35b0e11fd0e6..0eef2128cf09 100644 ---
> a/Documentation/ABI/testing/sysfs-bus-pci +++
> b/Documentation/ABI/testing/sysfs-bus-pci @@ -508,11 +508,16 @@
> Description: This file contains "native" if the device authenticated
>  		successfully with CMA-SPDM (PCIe r6.1 sec 6.31). It
> contains "none" if the device failed authentication (and may thus be
> -		malicious).
> +		malicious). It transitions from "native" to "tsm"
> after
> +		successful connection to a tsm, see the "connect"
> attribute
> +		below.
>  
>  		Writing "native" to this file causes
> reauthentication with kernel-selected keys and the kernel's
> certificate chain.  That
> -		may be opportune after updating the .cma keyring.
> +		may be opportune after updating the .cma keyring.
> Note
> +		that once connected to a tsm this returns -EBUSY to
> attempts to
> +		write "native", i.e. first disconnect from the tsm
> to retrigger
> +		native authentication.
>  
>  		The file is not visible if authentication is
> unsupported by the device.
> @@ -529,3 +534,37 @@ Description:
>  		The reason why authentication support could not be
> determined is apparent from "dmesg".  To probe for authentication
> support again, exercise the "remove" and "rescan" attributes.
> +
> +What:		/sys/bus/pci/devices/.../tsm/
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		This directory only appears if a device supports CMA
> and IDE,
> +		and only after a TSM driver has loaded and accepted
> / setup this
> +		PCI device. Similar to the 'authenticated'
> attribute, trigger
> +		"remove" and "rescan" to retry the initialization.
> See
> +		Documentation/ABI/testing/sysfs-class-tsm for
> enumerating the
> +		platform's TSM capabilities.
> +
> +What:		/sys/bus/pci/devices/.../tsm/connect
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RW) Writing "1" to this file triggers the TSM to
> establish a
> +		secure connection with the device. This typically
> includes an
> +		SPDM (DMTF Security Protocols and Data Models)
> session over PCIe
> +		DOE (Data Object Exchange) and PCIe IDE (Integrity
> and Data
> +		Encryption) establishment. For TSMs and devices that
> support
> +		both modes of IDE ("link" and "selective") the
> "connect_mode"
> +		attribute selects the mode.
> +
> +What:		/sys/bus/pci/devices/.../tsm/connect_mode
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) Returns the available connection modes
> optionally with
> +		brackets around the currently active mode if the
> device is
> +		connected. For example it may show "link selective"
> for a
> +		disconnected device, "link [selective]" for a
> selective
> +		connected device, and it may hide a mode that is not
> supported
> +		by the device or TSM.
> diff --git a/Documentation/ABI/testing/sysfs-class-tsm
> b/Documentation/ABI/testing/sysfs-class-tsm index
> 304b50b53e65..77957882738a 100644 ---
> a/Documentation/ABI/testing/sysfs-class-tsm +++
> b/Documentation/ABI/testing/sysfs-class-tsm @@ -10,3 +10,26 @@
> Description: For software TSMs instantiated by a software module,
> @host is a directory with attributes for that TSM, and those
> attributes are documented below.
> +
> +
> +What:		/sys/class/tsm/tsm0/pci/link_capable
> +Date:		January, 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) When present this returns "1\n" to indicate
> that the TSM
> +		supports establishing Link IDE with a given
> root-port attached
> +		device. See "tsm/connect_mode" in
> +		Documentation/ABI/testing/sysfs-bus-pci
> +
> +
> +What:		/sys/class/tsm/tsm0/pci/selective_streams
> +Date:		January, 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) When present this returns the number of
> currently available
> +		selective IDE streams available to the TSM. When a
> stream id is
> +		allocated this number is decremented and a link to
> the PCI
> +		device(s) consuming the stream(s) appears alonside
> this
> +		attribute in the /sys/class/tsm/tsm0/pci/ directory.
> See
> +		"tsm/connect" and "tsm/connect_mode" in
> +		Documentation/ABI/testing/sysfs-bus-pci.
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index a5c3cadddd6f..11d788038d19 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -129,6 +129,21 @@ config PCI_CMA
>  	  A PCI DOE mailbox is used as transport for DMTF SPDM based
>  	  authentication, measurement and secure channel
> establishment. 
> +config PCI_TSM
> +	bool "TEE Security Manager for Device Security"
> +	depends on PCI_CMA
> +	depends on TSM
> +	help
> +	  The TEE (Trusted Execution Environment) Device Interface
> +	  Security Protocol (TDISP) defines a "TSM" as a platform
> agent
> +	  that manages device authentication, link encryption, link
> +	  integrity protection, and assignment of PCI device
> functions
> +	  (virtual or physical) to confidential computing VMs that
> can
> +	  access (DMA) guest private memory.
> +
> +	  Say Y to enable the PCI subsystem to enable the IDE and
> +	  TDISP capabilities of devices via TSM semantics.
> +
>  config PCI_DOE
>  	bool
>  
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index cc8b5d1d15b9..c4117d67ea83 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -38,6 +38,8 @@ obj-$(CONFIG_PCI_CMA)		+= cma.o
> cma.asn1.o $(obj)/cma.o:			$(obj)/cma.asn1.h
>  $(obj)/cma.asn1.o:		$(obj)/cma.asn1.c $(obj)/cma.asn1.h
>  
> +obj-$(CONFIG_PCI_TSM)		+= tsm.o
> +
>  # Endpoint library must be initialized before its users
>  obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>  
> diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c
> index be7d2bb21b4c..5a69e9919589 100644
> --- a/drivers/pci/cma.c
> +++ b/drivers/pci/cma.c
> @@ -39,6 +39,9 @@ static ssize_t authenticated_store(struct device
> *dev, if (!sysfs_streq(buf, "native"))
>  		return -EINVAL;
>  
> +	if (pci_tsm_authenticated(pdev))
> +		return -EBUSY;
> +
>  	rc = pci_cma_reauthenticate(pdev);
>  	if (rc)
>  		return rc;
> @@ -55,6 +58,8 @@ static ssize_t authenticated_show(struct device
> *dev, (pdev->cma_init_failed || pdev->doe_init_failed))
>  		return -ENOTTY;
>  
> +	if (pci_tsm_authenticated(pdev))
> +		return sysfs_emit(buf, "tsm\n");
>  	if (spdm_authenticated(pdev->spdm_state))
>  		return sysfs_emit(buf, "native\n");
>  	return sysfs_emit(buf, "none\n");
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 368c4f71cc55..4327f8c2e6b5 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1654,6 +1654,9 @@ const struct attribute_group
> *pci_dev_attr_groups[] = { #endif
>  #ifdef CONFIG_PCI_CMA
>  	&pci_cma_attr_group,
> +#endif
> +#ifdef CONFIG_PCI_TSM
> +	&pci_tsm_attr_group,
>  #endif
>  	NULL,
>  };
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2b7d8d0b2e21..daa20866bc90 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -350,6 +350,20 @@ static inline int pci_cma_reauthenticate(struct
> pci_dev *pdev) }
>  #endif
>  
> +#ifdef CONFIG_PCI_TSM
> +void pci_tsm_init(struct pci_dev *pdev);
> +void pci_tsm_destroy(struct pci_dev *pdev);
> +extern const struct attribute_group pci_tsm_attr_group;
> +bool pci_tsm_authenticated(struct pci_dev *pdev);
> +#else
> +static inline void pci_tsm_init(struct pci_dev *pdev) { }
> +static inline void pci_tsm_destroy(struct pci_dev *pdev) { }
> +static inline bool pci_tsm_authenticated(struct pci_dev *pdev)
> +{
> +	return false;
> +}
> +#endif
> +
>  /**
>   * pci_dev_set_io_state - Set the new error state if possible.
>   *
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6b09c962c0b8..f60d6c3c8c48 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2542,6 +2542,7 @@ static void pci_init_capabilities(struct
> pci_dev *dev) pci_rcec_init(dev);		/* Root Complex
> Event Collector */ pci_doe_init(dev);		/* Data Object
> Exchange */ pci_cma_init(dev);		/* Component
> Measurement & Auth */
> +	pci_tsm_init(dev);		/* TEE Security Manager
> connection */ 
>  	pcie_report_downtraining(dev);
>  	pci_init_reset_methods(dev);
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index f009ac578997..228fa6ccf911 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -39,6 +39,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
>  	list_del(&dev->bus_list);
>  	up_write(&pci_bus_sem);
>  
> +	pci_tsm_destroy(dev);
>  	pci_cma_destroy(dev);
>  	pci_doe_destroy(dev);
>  	pcie_aspm_exit_link_state(dev);
> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> new file mode 100644
> index 000000000000..f74de0ee49a0
> --- /dev/null
> +++ b/drivers/pci/tsm.c
> @@ -0,0 +1,346 @@
> +// 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/pci.h>
> +#include <linux/tsm.h>
> +#include <linux/sysfs.h>
> +#include <linux/xarray.h>
> +#include "pci.h"
> +
> +/* collect tsm capable devices to rendezvous with the tsm driver */
> +static DEFINE_XARRAY(pci_tsm_devs);
> +
> +/*
> + * Provide a read/write lock against the init / exit of pdev tsm
> + * capabilities and arrival/departure of a tsm instance
> + */
> +static DECLARE_RWSEM(pci_tsm_rwsem);
> +static const struct tsm_pci_ops *tsm_ops;
> +
> +void generic_pci_tsm_req_free(struct pci_tsm_req *req)
> +{
> +	kfree(req);
> +}
> +EXPORT_SYMBOL_GPL(generic_pci_tsm_req_free);
> +
> +struct pci_tsm_req *generic_pci_tsm_req_alloc(struct pci_dev *pdev,
> enum pci_tsm_op op) +{
> +	struct pci_tsm_req *req = kzalloc(sizeof(*req), GFP_KERNEL);
> +
> +	if (!req)
> +		return NULL;
> +	req->op = op;
> +	return req;
> +}
> +EXPORT_SYMBOL_GPL(generic_pci_tsm_req_alloc);
> +
> +DEFINE_FREE(req_free, struct pci_tsm_req *, if (_T)
> tsm_ops->req_free(_T)) +
> +static int pci_tsm_disconnect(struct pci_dev *pdev)
> +{
> +	struct pci_tsm_req *req __free(req_free) = NULL;
> +
> +	/* opportunistic state checks to skip allocating a request */
> +	if (pdev->tsm->state < PCI_TSM_CONNECT)
> +		return 0;
> +
> +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_DISCONNECT);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
> +		enum pci_tsm_op_status status;
> +
> +		/* revalidate state */
> +		if (pdev->tsm->state < PCI_TSM_CONNECT)
> +			return 0;
> +		if (pdev->tsm->state < PCI_TSM_INIT)
> +			return -ENXIO;
> +
> +		do {
> +			status = tsm_ops->exec(pdev, req);
> +			req->seq++;
> +			/* TODO: marshal SPDM request */
> +		} while (status == PCI_TSM_SPDM_REQ);
> +
> +		if (status == PCI_TSM_FAIL)
> +			return -EIO;
> +		pdev->tsm->state = PCI_TSM_INIT;
> +	}
> +	return 0;
> +}
> +
> +static int pci_tsm_connect(struct pci_dev *pdev)
> +{
> +	struct pci_tsm_req *req __free(req_free) = NULL;
> +
> +	/* opportunistic state checks to skip allocating a request */
> +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +		return 0;
> +
> +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_CONNECT);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
> +		enum pci_tsm_op_status status;
> +
> +		/* revalidate state */
> +		if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +			return 0;
> +		if (pdev->tsm->state < PCI_TSM_INIT)
> +			return -ENXIO;
> +
> +		do {
> +			status = tsm_ops->exec(pdev, req);
> +			req->seq++;
> +		} while (status == PCI_TSM_SPDM_REQ);
> +
> +		if (status == PCI_TSM_FAIL)
> +			return -EIO;
> +		pdev->tsm->state = PCI_TSM_CONNECT;
> +	}
> +	return 0;
> +}
> +
> +static ssize_t connect_store(struct device *dev, struct
> device_attribute *attr,
> +			     const char *buf, size_t len)
> +{
> +	bool connect;
> +	int rc = kstrtobool(buf, &connect);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (rc)
> +		return rc;
> +
> +	if (connect) {
> +		if (!spdm_authenticated(pdev->spdm_state)) {
> +			pci_dbg(pdev, "SPDM authentication
> pre-requisite not met.\n");
> +			return -ENXIO;
> +		}
> +		rc = pci_tsm_connect(pdev);
> +		if (rc)
> +			return rc;
> +		return len;
> +	}
> +
> +	rc = pci_tsm_disconnect(pdev);
> +	if (rc)
> +		return rc;
> +	return len;
> +}
> +
> +static ssize_t connect_show(struct device *dev, struct
> device_attribute *attr,
> +			    char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sysfs_emit(buf, "%d\n", pdev->tsm->state >=
> PCI_TSM_CONNECT); +}
> +static DEVICE_ATTR_RW(connect);
> +
> +static const char *const pci_tsm_modes[] = {
> +	[PCI_TSM_MODE_LINK] = "link",
> +	[PCI_TSM_MODE_SELECTIVE] = "selective",
> +};
> +
> +static ssize_t connect_mode_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t len)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int i;
> +
> +	guard(mutex)(tsm_ops->lock);
> +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +		return -EBUSY;
> +	for (i = 0; i < ARRAY_SIZE(pci_tsm_modes); i++)
> +		if (sysfs_streq(buf, pci_tsm_modes[i]))
> +			break;
> +	if (i == PCI_TSM_MODE_LINK) {
> +		if (pdev->tsm->link_capable)
> +			pdev->tsm->mode = PCI_TSM_MODE_LINK;
> +		return -EOPNOTSUPP;
> +	} else if (i == PCI_TSM_MODE_SELECTIVE) {
> +		if (pdev->tsm->selective_capable)
> +			pdev->tsm->mode = PCI_TSM_MODE_SELECTIVE;
> +		return -EOPNOTSUPP;
> +	} else
> +		return -EINVAL;
> +	return len;
> +}
> +
> +static ssize_t connect_mode_show(struct device *dev,
> +				 struct device_attribute *attr, char
> *buf) +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	ssize_t count = 0;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pci_tsm_modes); i++) {
> +		if (i == PCI_TSM_MODE_LINK) {
> +			if (!pdev->tsm->link_capable)
> +				continue;
> +		} else if (i == PCI_TSM_MODE_SELECTIVE) {
> +			if (!pdev->tsm->selective_capable)
> +				continue;
> +		}
> +
> +		if (i == pdev->tsm->mode)
> +			count += sysfs_emit_at(buf, count, "[%s] ",
> +					       pci_tsm_modes[i]);
> +		else
> +			count += sysfs_emit_at(buf, count, "%s ",
> +					       pci_tsm_modes[i]);
> +	}
> +
> +	if (count)
> +		buf[count - 1] = '\n';
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(connect_mode);
> +
> +static umode_t pci_tsm_attr_visible(struct kobject *kobj, struct
> attribute *a, int n) +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (a == &dev_attr_connect_mode.attr) {
> +		if (pdev->tsm->link_capable ||
> pdev->tsm->selective_capable)
> +			return a->mode;
> +		return 0;
> +	}
> +
> +	return a->mode;
> +}
> +
> +static bool pci_tsm_group_visible(struct kobject *kobj)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (pdev->tsm && pdev->tsm->state > PCI_TSM_IDLE)
> +		return true;
> +	return false;
> +}
> +DEFINE_SYSFS_GROUP_VISIBLE(pci_tsm);
> +
> +static struct attribute *pci_tsm_attrs[] = {
> +	&dev_attr_connect.attr,
> +	&dev_attr_connect_mode.attr,
> +	NULL,
> +};
> +
> +const struct attribute_group pci_tsm_attr_group = {
> +	.name = "tsm",
> +	.attrs = pci_tsm_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(pci_tsm),
> +};
> +
> +static int pci_tsm_add(struct pci_dev *pdev)
> +{
> +	lockdep_assert_held(&pci_tsm_rwsem);
> +	if (!tsm_ops)
> +		return 0;
> +	scoped_guard(mutex, tsm_ops->lock) {
> +		if (pdev->tsm->state < PCI_TSM_INIT) {
> +			int rc = tsm_ops->add(pdev);
> +
> +			if (rc)
> +				return rc;
> +		}
> +		pdev->tsm->state = PCI_TSM_INIT;
> +	}
> +	return sysfs_update_group(&pdev->dev.kobj,
> &pci_tsm_attr_group); +}
> +
> +static void pci_tsm_del(struct pci_dev *pdev)
> +{
> +	lockdep_assert_held(&pci_tsm_rwsem);
> +	/* shutdown sysfs operations before tsm delete */
> +	pdev->tsm->state = PCI_TSM_IDLE;
> +	sysfs_update_group(&pdev->dev.kobj, &pci_tsm_attr_group);
> +	guard(mutex)(tsm_ops->lock);
> +	tsm_ops->del(pdev);
> +}
> +
> +int pci_tsm_register(const struct tsm_pci_ops *ops)
> +{
> +	struct pci_dev *pdev;
> +	unsigned long index;
> +
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	if (tsm_ops)
> +		return -EBUSY;
> +	tsm_ops = ops;
> +	xa_for_each(&pci_tsm_devs, index, pdev)
> +		pci_tsm_add(pdev);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_tsm_register);
> +
> +void pci_tsm_unregister(const struct tsm_pci_ops *ops)
> +{
> +	struct pci_dev *pdev;
> +	unsigned long index;
> +
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	if (ops != tsm_ops)
> +		return;
> +	xa_for_each(&pci_tsm_devs, index, pdev)
> +		pci_tsm_del(pdev);
> +	tsm_ops = NULL;
> +}
> +EXPORT_SYMBOL_GPL(pci_tsm_unregister);
> +
> +void pci_tsm_init(struct pci_dev *pdev)
> +{
> +	u16 ide_cap;
> +	int rc;
> +
> +	if (!pdev->cma_capable)
> +		return;
> +
> +	ide_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_IDE);
> +	if (!ide_cap)
> +		return;
> +
> +	struct pci_tsm *tsm __free(kfree) = kzalloc(sizeof(*tsm),
> GFP_KERNEL);
> +	if (!tsm)
> +		return;
> +
> +	tsm->ide_cap = ide_cap;
> +
> +	rc = xa_insert(&pci_tsm_devs, (unsigned long)pdev, pdev,
> GFP_KERNEL);
> +	if (rc) {
> +		pci_dbg(pdev, "failed to register tsm capable
> device\n");
> +		return;
> +	}
> +
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	pdev->tsm = no_free_ptr(tsm);
> +	pci_tsm_add(pdev);
> +}
> +
> +void pci_tsm_destroy(struct pci_dev *pdev)
> +{
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	pci_tsm_del(pdev);
> +	xa_erase(&pci_tsm_devs, (unsigned long)pdev);
> +	kfree(pdev->tsm);
> +	pdev->tsm = NULL;
> +}
> +
> +bool pci_tsm_authenticated(struct pci_dev *pdev)
> +{
> +	guard(rwsem_read)(&pci_tsm_rwsem);
> +	return pdev->tsm && pdev->tsm->state >= PCI_TSM_CONNECT;
> +}
> diff --git a/drivers/virt/coco/tsm/Makefile
> b/drivers/virt/coco/tsm/Makefile index f7561169faed..a4f0d07d7d97
> 100644 --- a/drivers/virt/coco/tsm/Makefile
> +++ b/drivers/virt/coco/tsm/Makefile
> @@ -7,3 +7,4 @@ tsm_reports-y := reports.o
>  
>  obj-$(CONFIG_TSM) += tsm.o
>  tsm-y := class.o
> +tsm-$(CONFIG_PCI_TSM) += pci.o
> diff --git a/drivers/virt/coco/tsm/class.c
> b/drivers/virt/coco/tsm/class.c index a569fa6b09eb..a459e51c0892
> 100644 --- a/drivers/virt/coco/tsm/class.c
> +++ b/drivers/virt/coco/tsm/class.c
> @@ -8,13 +8,11 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/cleanup.h>
> +#include "tsm.h"
>  
>  static DECLARE_RWSEM(tsm_core_rwsem);
> -struct class *tsm_class;
> -struct tsm_subsys {
> -	struct device dev;
> -	const struct tsm_info *info;
> -} *tsm_subsys;
> +static struct class *tsm_class;
> +static struct tsm_subsys *tsm_subsys;
>  
>  int tsm_register(const struct tsm_info *info)
>  {
> @@ -52,6 +50,10 @@ int tsm_register(const struct tsm_info *info)
>  	dev = NULL;
>  	tsm_subsys = subsys;
>  
> +	rc = tsm_pci_init(info);
> +	if (rc)
> +		pr_err("PCI initialization failure: %d\n", rc);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tsm_register);
> @@ -65,6 +67,8 @@ void tsm_unregister(const struct tsm_info *info)
>  		return;
>  	}
>  
> +	tsm_pci_destroy(info);
> +
>  	if (info->host)
>  		sysfs_remove_link(&tsm_subsys->dev.kobj, "host");
>  	device_unregister(&tsm_subsys->dev);
> @@ -79,6 +83,13 @@ static void tsm_release(struct device *dev)
>  	kfree(subsys);
>  }
>  
> +static const struct attribute_group *tsm_attr_groups[] = {
> +#ifdef CONFIG_PCI_TSM
> +	&tsm_pci_attr_group,
> +#endif
> +	NULL,
> +};
> +
>  static int __init tsm_init(void)
>  {
>  	tsm_class = class_create("tsm");
> @@ -86,6 +97,7 @@ static int __init tsm_init(void)
>  		return PTR_ERR(tsm_class);
>  
>  	tsm_class->dev_release = tsm_release;
> +	tsm_class->dev_groups = tsm_attr_groups;
>  	return 0;
>  }
>  module_init(tsm_init)
> diff --git a/drivers/virt/coco/tsm/pci.c b/drivers/virt/coco/tsm/pci.c
> new file mode 100644
> index 000000000000..b3684ad7114f
> --- /dev/null
> +++ b/drivers/virt/coco/tsm/pci.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/tsm.h>
> +#include <linux/device.h>
> +#include "tsm.h"
> +
> +static ssize_t link_capable_show(struct device *dev,
> +				 struct device_attribute *attr, char
> *buf) +{
> +	struct tsm_subsys *subsys = container_of(dev,
> typeof(*subsys), dev); +
> +	return sysfs_emit(buf, "%u\n",
> subsys->info->link_stream_capable); +}
> +static DEVICE_ATTR_RO(link_capable);
> +
> +static ssize_t selective_streams_show(struct device *dev,
> +				      struct device_attribute *attr,
> char *buf) +{
> +	struct tsm_subsys *subsys = container_of(dev,
> typeof(*subsys), dev); +
> +	return sysfs_emit(buf, "%u\n",
> subsys->info->nr_selective_streams); +}
> +static DEVICE_ATTR_RO(selective_streams);
> +
> +static umode_t tsm_pci_attr_visible(struct kobject *kobj, struct
> attribute *a, int n) +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct tsm_subsys *subsys = container_of(dev,
> typeof(*subsys), dev);
> +	const struct tsm_info *info = subsys->info;
> +
> +	if (a == &dev_attr_link_capable.attr) {
> +		if (info->link_stream_capable)
> +			return a->mode;
> +		return 0;
> +	}
> +
> +	if (a == &dev_attr_selective_streams.attr) {
> +		if (info->nr_selective_streams)
> +			return a->mode;
> +		return 0;
> +	}
> +
> +	return a->mode;
> +}
> +
> +static bool tsm_pci_group_visible(struct kobject *kobj)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct tsm_subsys *subsys = container_of(dev,
> typeof(*subsys), dev); +
> +	if (subsys->info->pci_ops)
> +		return true;
> +	return false;
> +}
> +DEFINE_SYSFS_GROUP_VISIBLE(tsm_pci);
> +
> +static struct attribute *tsm_pci_attrs[] = {
> +	&dev_attr_link_capable.attr,
> +	&dev_attr_selective_streams.attr,
> +	NULL,
> +};
> +
> +const struct attribute_group tsm_pci_attr_group = {
> +	.name = "pci",
> +	.attrs = tsm_pci_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(tsm_pci),
> +};
> +
> +int tsm_pci_init(const struct tsm_info *info)
> +{
> +	if (!info->pci_ops)
> +		return 0;
> +
> +	return pci_tsm_register(info->pci_ops);
> +}
> +
> +void tsm_pci_destroy(const struct tsm_info *info)
> +{
> +	pci_tsm_unregister(info->pci_ops);
> +}
> diff --git a/drivers/virt/coco/tsm/tsm.h b/drivers/virt/coco/tsm/tsm.h
> new file mode 100644
> index 000000000000..407c388a109b
> --- /dev/null
> +++ b/drivers/virt/coco/tsm/tsm.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __TSM_CORE_H
> +#define __TSM_CORE_H
> +
> +#include <linux/device.h>
> +
> +struct tsm_info;
> +struct tsm_subsys {
> +	struct device dev;
> +	const struct tsm_info *info;
> +};
> +
> +#ifdef CONFIG_PCI_TSM
> +int tsm_pci_init(const struct tsm_info *info);
> +void tsm_pci_destroy(const struct tsm_info *info);
> +extern const struct attribute_group tsm_pci_attr_group;
> +#else
> +static inline int tsm_pci_init(const struct tsm_info *info)
> +{
> +	return 0;
> +}
> +static inline void tsm_pci_destroy(const struct tsm_info *info)
> +{
> +}
> +#endif
> +
> +#endif /* TSM_CORE_H */
> +
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 4a04ce7685e7..132962b21e04 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -522,6 +522,9 @@ struct pci_dev {
>  	struct spdm_state *spdm_state;	/* Security Protocol
> and Data Model */ unsigned int	cma_capable:1;	/*
> Authentication supported */ unsigned int	cma_init_failed:1;
> +#endif
> +#ifdef CONFIG_PCI_TSM
> +	struct pci_tsm *tsm;		/* TSM operation state */
>  #endif
>  	u16		acs_cap;	/* ACS Capability offset
> */ phys_addr_t	rom;		/* Physical address if not
> from BAR */ diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index 8cb8a661ba41..f5dbdfa65d8d 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -4,11 +4,15 @@
>  
>  #include <linux/sizes.h>
>  #include <linux/types.h>
> +#include <linux/mutex.h>
>  
>  struct tsm_info {
>  	const char *name;
>  	struct device *host;
>  	const struct attribute_group **groups;
> +	const struct tsm_pci_ops *pci_ops;
> +	unsigned int nr_selective_streams;
> +	unsigned int link_stream_capable:1;
>  };
>  
>  #define TSM_REPORT_INBLOB_MAX 64
> @@ -74,4 +78,77 @@ int tsm_report_register(const struct
> tsm_report_ops *ops, void *priv, int tsm_report_unregister(const
> struct tsm_report_ops *ops); int tsm_register(const struct tsm_info
> *info); void tsm_unregister(const struct tsm_info *info);
> +
> +enum pci_tsm_op_status {
> +	PCI_TSM_FAIL = -1,
> +	PCI_TSM_OK,
> +	PCI_TSM_SPDM_REQ,
> +};
> +
> +enum pci_tsm_op {
> +	PCI_TSM_OP_CONNECT,
> +	PCI_TSM_OP_DISCONNECT,
> +};
> +
> +struct pci_tsm_req {
> +	enum pci_tsm_op op;
> +	unsigned int seq;
> +};
> +
> +struct pci_dev;
> +/**
> + * struct tsm_pci_ops - Low-level TSM-exported interface to the PCI
> core
> + * @add: accept device for tsm operation, locked
> + * @del: teardown tsm context for @pdev, locked
> + * @req_alloc: setup context for given operation, unlocked
> + * @req_free: teardown context for given request, unlocked
> + * @exec: run @req, may be invoked multiple times per @req, locked
> + * @lock: tsm work is one device and one op at a time
> + */
> +struct tsm_pci_ops {
> +	int (*add)(struct pci_dev *pdev);
> +	void (*del)(struct pci_dev *pdev);
> +	struct pci_tsm_req *(*req_alloc)(struct pci_dev *pdev,
> +					 enum pci_tsm_op op);
> +	struct pci_tsm_req *(*req_free)(struct pci_tsm_req *req);
> +	enum pci_tsm_op_status (*exec)(struct pci_dev *pdev,
> +				       struct pci_tsm_req *req);
> +	struct mutex *lock;
> +};
> +
> +enum pci_tsm_state {
> +	PCI_TSM_IDLE,
> +	PCI_TSM_INIT,
> +	PCI_TSM_CONNECT,
> +};
> +
> +enum pci_tsm_mode {
> +	PCI_TSM_MODE_LINK,
> +	PCI_TSM_MODE_SELECTIVE,
> +};
> +
> +struct pci_tsm {
> +	enum pci_tsm_state state;
> +	enum pci_tsm_mode mode;
> +	u16 ide_cap;
> +	unsigned int link_capable:1;
> +	unsigned int selective_capable:1;
> +	void *tsm_data;
> +};
> +
> +#ifdef CONFIG_PCI_TSM
> +int pci_tsm_register(const struct tsm_pci_ops *ops);
> +void pci_tsm_unregister(const struct tsm_pci_ops *ops);
> +void generic_pci_tsm_req_free(struct pci_tsm_req *req);
> +struct pci_tsm_req *generic_pci_tsm_req_alloc(struct pci_dev *pdev,
> +					      enum pci_tsm_op op);
> +#else
> +static inline int pci_tsm_register(const struct tsm_pci_ops *ops)
> +{
> +	return 0;
> +}
> +static inline void pci_tsm_unregister(const struct tsm_pci_ops *ops)
> +{
> +}
> +#endif
>  #endif /* __TSM_H */
> diff --git a/include/uapi/linux/pci_regs.h
> b/include/uapi/linux/pci_regs.h index a39193213ff2..1219d50f8e89
> 100644 --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -742,7 +742,8 @@
>  #define PCI_EXT_CAP_ID_PL_16GT	0x26	/* Physical Layer
> 16.0 GT/s */ #define PCI_EXT_CAP_ID_PL_32GT  0x2A    /* Physical
> Layer 32.0 GT/s */ #define PCI_EXT_CAP_ID_DOE	0x2E	/*
> Data Object Exchange */ -#define PCI_EXT_CAP_ID_MAX
> PCI_EXT_CAP_ID_DOE +#define PCI_EXT_CAP_ID_IDE	0x30	/*
> Integrity and Data Encryption */ +#define PCI_EXT_CAP_ID_MAX
> PCI_EXT_CAP_ID_IDE 
>  #define PCI_EXT_CAP_DSN_SIZEOF	12
>  #define PCI_EXT_CAP_MCAST_ENDPOINT_SIZEOF 40
> 
>

Alexey Kardashevskiy wrote:
> On 30/1/24 20:24, Dan Williams wrote:
> > The PCIe 6.1 specification, section 11, introduces the Trusted
> > Execution Environment (TEE) Device Interface Security Protocol (TDISP).
> > This interface definition builds upon CMA, component measurement and
> > authentication, and IDE, link integrity and data encryption. It adds
> > support for establishing virtual functions within a device that can be
> > assigned 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 device security manager (DSM) and
> > system software in both a VMM and a VM. 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.
> > 
> > Similar to the PCI core extensions to support CONFIG_PCI_CMA,
> > CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
> > attribute, and add more properties + controls in a tsm/ subdirectory of
> > the PCI device sysfs interface. Unlike CMA that can depend on a local to
> > the PCI core implementation, PCI_TSM needs to be prepared for late
> > loading of the platform TSM driver. Consider that the TSM driver may
> > itself be a PCI driver. Userspace can depend on the common TSM device
> > uevent to know when the PCI core has TSM services enabled. The PCI
> > device tsm/ subdirectory is supplemented by the TSM device pci/
> > directory for platform global TSM properties + controls.
> > 
> > All vendor TSM implementations share the property of asking the VMM to
> > perform DOE mailbox operations on behalf of the TSM. That common
> > capability is centralized in PCI core code that invokes an ->exec()
> > operation callback potentially multiple times to service a given request
> > (struct pci_tsm_req). Future operations / verbs will be handled
> > similarly with the "request + exec" model. For now, only "connect" and
> > "disconnect" are implemented which at a minimum is expected to establish
> > IDE for the link.
> > 
> > In addition to requests the low-level TSM implementation is notified of
> > device arrival and departure events so that it can filter devices that
> > the TSM is not prepared to support, or otherwise setup and teardown
> > per-device context.
> 
> 
> It's a good start but I am still digesting this scaffolding.
> 
[..]
> > diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm
> > index 304b50b53e65..77957882738a 100644
> > --- a/Documentation/ABI/testing/sysfs-class-tsm
> > +++ b/Documentation/ABI/testing/sysfs-class-tsm
> > @@ -10,3 +10,26 @@ Description:
> >   		For software TSMs instantiated by a software module, @host is a
> >   		directory with attributes for that TSM, and those attributes are
> >   		documented below.
> > +
> > +
> > +What:		/sys/class/tsm/tsm0/pci/link_capable
> > +Date:		January, 2024
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(RO) When present this returns "1\n" to indicate that the TSM
> > +		supports establishing Link IDE with a given root-port attached
> > +		device. See "tsm/connect_mode" in
> > +		Documentation/ABI/testing/sysfs-bus-pci
> 
> I am struggling to make sense of "a given root-port attached device".
> There is one CCP device on AMD SEV and therefore one /sys/class/tsm/tsmX 
> but still many root ports. How do root ports relate to /sys/class/tsm/tsm0 ?

This is a property of the TSM for whether it supports Link IDE,
Selective IDE, or both. Since Link IDE is a point-to-point protocol, the
TSM can only report whether Link IDE is available for Link IDE capable
root-ports.

For example, I believe some platform vendors are only supporting
Selective IDE with their TSM, while others additionally support Link
IDE.

For simplicity, I will likely drop Link IDE details out of this proposal
as most known use cases specify Selective IDE. Link IDE complexity can
always be added back later.


> > +
> > +
> > +What:		/sys/class/tsm/tsm0/pci/selective_streams
> > +Date:		January, 2024
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(RO) When present this returns the number of currently available
> > +		selective IDE streams available to the TSM. When a stream id is
> > +		allocated this number is decremented and a link to the PCI
> > +		device(s) consuming the stream(s) appears alonside this
> 
> s/alonside/alongside/

thx.

> > +		attribute in the /sys/class/tsm/tsm0/pci/ directory. See
> > +		"tsm/connect" and "tsm/connect_mode" in
> > +		Documentation/ABI/testing/sysfs-bus-pci.
> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > index a5c3cadddd6f..11d788038d19 100644
> > --- a/drivers/pci/Kconfig
> > +++ b/drivers/pci/Kconfig
> > @@ -129,6 +129,21 @@ config PCI_CMA
> >   	  A PCI DOE mailbox is used as transport for DMTF SPDM based
> >   	  authentication, measurement and secure channel establishment.
> >   
> > +config PCI_TSM
> > +	bool "TEE Security Manager for Device Security"
> 
> (discussed elsewhere, I'll rant here once more and then will shut up)
> 
> It is bool and not tristate :(

Yes.

> CMA, DOE are the same, quite annoying (as in these early days I am 
> adding printks here and there and rmmod+modpbobe saves time but builtins 
> mean reboot) and imho no really necessary as (from 4/5) "only next 
> generation server hosts will start to include a platform TSM".

A couple observations:

* A significant portion of the overall TSM driver complexity lies in
  low-level the platform vendor specific init paths. Those
  initializations paths happen on demand via a loadable module.
  Additionally the successful outcome of cross-vendor  collaboration is
  that the vendor specific runtime paths stay thin, and those will also be
  included the demand loaded vendor platform module.

* The vmlinux .text size increase to turn on CONFIG_PCI_CMA and
  CONFIG_PCI_TSM is in the noise (0.01% increase). The runtime memory
  utilization when the feature is not used is just a pointer in a 'struct
  pci_dev', i.e. only a few hundred bytes for a reasonable laptop.

So the cost of maintaining this facility in the PCI core is negligible
from a memory overhead overhead perspective and the bulk of the
complexity that would benefit from being runtime replaceable as a module
(low level TSM driver) remains modular.

For that small cost we gain a facility that is easy for the rest of the
kernel to reason about and audit. All authentication state linked off of
the PCI device, all sysfs attributes centrally defined. The fundamental
object lifetime and sysfs lifetime is still pci_init_capabilities() and
pci_destroy_dev(), and TSM facilities can be tightly coupled with CMA
which had zero concerns being added to the PCI core.

[..]
> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> > new file mode 100644
> > index 000000000000..f74de0ee49a0
> > --- /dev/null
> > +++ b/drivers/pci/tsm.c
> > @@ -0,0 +1,346 @@
> > +// 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/pci.h>
> > +#include <linux/tsm.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/xarray.h>
> > +#include "pci.h"
> > +
> > +/* collect tsm capable devices to rendezvous with the tsm driver */
> > +static DEFINE_XARRAY(pci_tsm_devs);
> 
> Not used anywhere.

pci_tsm_register(), pci_tsm_unregister(), pci_tsm_init()

[..]
> > +static int pci_tsm_add(struct pci_dev *pdev)
> 
> Nothing checks the returned value.

Yeah, it turns out that if the tsm rejects the device for whatever
reason then its state will never advance past PCI_TSM_INIT. Likely I
will make pci_tsm_add() return void in the next round.

[..]
> > diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> > index 8cb8a661ba41..f5dbdfa65d8d 100644
> > --- a/include/linux/tsm.h
> > +++ b/include/linux/tsm.h
[..]
> > @@ -74,4 +78,77 @@ int tsm_report_register(const struct tsm_report_ops *ops, void *priv,
> >   int tsm_report_unregister(const struct tsm_report_ops *ops);
> >   int tsm_register(const struct tsm_info *info);
> >   void tsm_unregister(const struct tsm_info *info);
> > +
> > +enum pci_tsm_op_status {
> > +	PCI_TSM_FAIL = -1,
> > +	PCI_TSM_OK,
> > +	PCI_TSM_SPDM_REQ,
> 
> Secure SPDM is also needed here. In my toy TSM project [1] I am just 
> using negatives for errors, 0 for "successfully finished" and positives 
> for a DOE protocol (1 for SPDM, 2 for Secure SPDM), seems alright as it 
> is all about PCI anyway (although "pci" is not always present in all 
> these enums and structs).

Oh, I thought the SPDM marshaling was opaque so the kernel would not
know or care if it was transporting clear-text or cipher text, I will
take a closer look at your implementation.

> > +};
> > +
> > +enum pci_tsm_op {
> > +	PCI_TSM_OP_CONNECT,
> > +	PCI_TSM_OP_DISCONNECT,
> > +};
> > +
> > +struct pci_tsm_req {
> > +	enum pci_tsm_op op;
> > +	unsigned int seq;
> 
> @seq is not tested anywhere.
> 
> May be move (*req_free) here.

Say more, why does each request needs its own custom way to free a
request? I am following the lead of proven mechanisms like bio_put() and
bio_free() where the bio does not have its own ->bio_free() callback.

> > +};
> > +
> > +struct pci_dev;
> > +/**
> > + * struct tsm_pci_ops - Low-level TSM-exported interface to the PCI core
> > + * @add: accept device for tsm operation, locked
> > + * @del: teardown tsm context for @pdev, locked
> > + * @req_alloc: setup context for given operation, unlocked
> > + * @req_free: teardown context for given request, unlocked
> > + * @exec: run @req, may be invoked multiple times per @req, locked
> > + * @lock: tsm work is one device and one op at a time
> > + */
> > +struct tsm_pci_ops {
> > +	int (*add)(struct pci_dev *pdev);
> > +	void (*del)(struct pci_dev *pdev);
> > +	struct pci_tsm_req *(*req_alloc)(struct pci_dev *pdev,
> > +					 enum pci_tsm_op op);
> > +	struct pci_tsm_req *(*req_free)(struct pci_tsm_req *req);
> > +	enum pci_tsm_op_status (*exec)(struct pci_dev *pdev,
> > +				       struct pci_tsm_req *req);
> 
> The pci_tsm_req is just an @op, three hooks seems to be more than 
> needed, could be just (*exec)(struct pci_dev *pdev, enum pci_tsm_op op).

The rationale for other ops beyond exec is to keep the locking context
clear.

That said I do need to rework the locking to make it finer grained, and
allow multiple physical functions to be acted upon at the same time.

> Or the idea is to extend pci_tsm_req with some void *platform_req_data, 
> is not it?

Exactly. req_alloc() is where the low level TSM driver gets to append
its data to the request before execution, and req_free() lets the low
level TSM driver unwind that.

> There is one "op" in flight per a physical device allowed in 
> SEV TIO, I suspect that is likely to be the case for others so such data 
> can be managed by the platform code in the platform data of a TEE-IO device.

Yes, I think one-op per device is a common trait that can be managed by
the core.

[..]
> > +struct pci_tsm {
> > +	enum pci_tsm_state state;
> > +	enum pci_tsm_mode mode;
> 
> Does it have to be either mode and cannot be both?

Yes, mode is "selective" vs "link" IDE. The TDISP spec also allows for a
third option of "TSM deems no IDE needed". However, in terms of what the
user can pick it needs to be "selective" if they want TEE I/O operation.

...and a TSM is free to not implement support for the Link IDE so that
capability can be hidden.

Alexey Kardashevskiy wrote:
[..]
> > diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c
> > index be7d2bb21b4c..5a69e9919589 100644
> > --- a/drivers/pci/cma.c
> > +++ b/drivers/pci/cma.c
> > @@ -39,6 +39,9 @@ static ssize_t authenticated_store(struct device *dev,
> >   	if (!sysfs_streq(buf, "native"))
> >   		return -EINVAL;
> >   
> > +	if (pci_tsm_authenticated(pdev))
> > +		return -EBUSY;
> > +
> >   	rc = pci_cma_reauthenticate(pdev);
> >   	if (rc)
> >   		return rc;
> 
> btw is this "native" CMA expected to migrate to tsm_pci_ops? Thanks,

No, CMA is independent from TSM enabling. So you have the option to only
ever authenticate devices via kernel-native CMA, and ignore TEE I/O and
the platform TSM completely. Or, once CMA authentication succeeds then
the kernel additionally allows transitioning the device to be TSM
authenticated / connected.

This keeps the Linux device-attestation ecosystem healthy, standards
compliant devices with managed certificate distribution.

Zhi Wang wrote:
[..]
> Hey Dan,
> 
> 1) What is the expectation of using the device connect and disconnect
> in the guest-driven secure I/O enlightenment?

"Connect" is state of the link that can be automatically maintained over
events like reset and error recovery. The guest is responsible for Bind
/ Unbind.

Now, the host can optionally "Connect" in response to a "Bind" event,
but it is not clear that such a mechanism is needed. It likely is going
to depend on how error handling is implemented, and whether an event
that causes disconnect can be recovered. We may get there, but likely
not in the initial phases of the implementation.

> In the last device security meeting, you said the sysfs interface was
> mostly for higher level software stacks, like virt-manager. I was
> wondering what would be the picture looks like when coping these
> statement with the guest-driven model. Are we expecting the device
> connect triggered by QEMU when extracting the guest request from the
> secure channel in this case?

I think it is simplest for now if "Connect" is a pre-requisite for
guest-triggered "Bind".

> 2) How does the device-specific logic fit into the new TSM
> services? E.g. when the TDISP connect is triggered by userspace, a
> device needs to perform quirks before/after/inside the verbs, or a
> device needs an interface to tell TSM service when it is able to
> response to some verbs. Do you think we needs to have a set of
> callbacks from the device side for the PCI TSM service to call?

True "quirks" would be driven by bug reports. Outside of that likely the
attestation information needs to have multiple validation entry points
for userspace, PCI core, and endpoint drivers to each have a chance to
deploy some attestation policy. Some of these questions will need have
common answers developed between the native CMA implementation and the
TSM implementation since CMA needs to solve some of ABI issues of making
measurements available to attestation agents.

At Plumbers I had been thinking "golden measurements" injected into the
kernel ahead of interface validation gives the kernel the most autonomy,
but questions about measurement nonce and other concerns tempered my
thinking there. Plenty to still talk about and navigate.

On Mon, 26 Feb 2024 22:34:54 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Zhi Wang wrote:
> [..]
> > Hey Dan,
> > 
> > 1) What is the expectation of using the device connect and
> > disconnect in the guest-driven secure I/O enlightenment?
> 
> "Connect" is state of the link that can be automatically maintained
> over events like reset and error recovery. The guest is responsible
> for Bind / Unbind.
> 
> Now, the host can optionally "Connect" in response to a "Bind" event,
> but it is not clear that such a mechanism is needed. It likely is
> going to depend on how error handling is implemented, and whether an
> event that causes disconnect can be recovered. We may get there, but
> likely not in the initial phases of the implementation.
> 
> > In the last device security meeting, you said the sysfs interface
> > was mostly for higher level software stacks, like virt-manager. I
> > was wondering what would be the picture looks like when coping these
> > statement with the guest-driven model. Are we expecting the device
> > connect triggered by QEMU when extracting the guest request from the
> > secure channel in this case?
> 
> I think it is simplest for now if "Connect" is a pre-requisite for
> guest-triggered "Bind".
>

Thanks so much for the reply. 

IIUC, it means a guest assumes the device is in "connect" state when
noticing a TDI, and it has the awareness that the "connect" state will
be taken care by host until it needs to step in error recovery?

I am more thinking of how device driver talks with the PCI core.

+static int pci_tsm_connect(struct pci_dev *pdev)
+{
+	struct pci_tsm_req *req __free(req_free) = NULL;
+
+	/* opportunistic state checks to skip allocating a request */
+	if (pdev->tsm->state >= PCI_TSM_CONNECT)
+		return 0;
+

As this patch is triggered by userspace through sysfs, I am wondering
would it be a good idea to let the device driver step in around the
device connect/disconnect flow in the future? as a device might needs to
be switched to different states before it is ready to handle SPDM and
IDE.

Maybe the PCI core (pci_tsm_{connect, disconnect, error_handling}())
should broadcast the event through a notifier when checking the connect
state. An example kernel user of that notifier can forward the event to
the userspace as udev events via PF_NETLINK.

+	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_CONNECT);
+	if (!req)
+		return -ENOMEM;
+
+	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
+		enum pci_tsm_op_status status;
+
+		/* revalidate state */
+		if (pdev->tsm->state >= PCI_TSM_CONNECT)
+			return 0;
+		if (pdev->tsm->state < PCI_TSM_INIT)
+			return -ENXIO;
+
+		do {
+			status = tsm_ops->exec(pdev, req);
+			req->seq++;
+		} while (status == PCI_TSM_SPDM_REQ);
+
+		if (status == PCI_TSM_FAIL)
+			return -EIO;
+		pdev->tsm->state = PCI_TSM_CONNECT;
+	}
+	return 0;
+}
 
> > 2) How does the device-specific logic fit into the new TSM
> > services? E.g. when the TDISP connect is triggered by userspace, a
> > device needs to perform quirks before/after/inside the verbs, or a
> > device needs an interface to tell TSM service when it is able to
> > response to some verbs. Do you think we needs to have a set of
> > callbacks from the device side for the PCI TSM service to call?
> 
> True "quirks" would be driven by bug reports. Outside of that likely

Yup, I was just thinking another approach for the device specific code
to step in for pci_tsm_{connect,disconnect}() without a driver and
pci-quirks just popped up. :)

> the attestation information needs to have multiple validation entry
> points for userspace, PCI core, and endpoint drivers to each have a
> chance to deploy some attestation policy. Some of these questions
> will need have common answers developed between the native CMA
> implementation and the TSM implementation since CMA needs to solve
> some of ABI issues of making measurements available to attestation
> agents.
> 
> At Plumbers I had been thinking "golden measurements" injected into
> the kernel ahead of interface validation gives the kernel the most
> autonomy, but questions about measurement nonce and other concerns
> tempered my thinking there. Plenty to still talk about and navigate.

Yes. We had been discussing that a lot. :) I also carefully watched the
playback of LPC Confidential MC, quite a lot nice discussions. Out
of curiosity, were folks think eBPF a good fit or an overkill here? :)

Thanks,
Zhi.

Zhi Wang wrote:
> On Mon, 26 Feb 2024 22:34:54 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Zhi Wang wrote:
> > [..]
> > > Hey Dan,
> > > 
> > > 1) What is the expectation of using the device connect and
> > > disconnect in the guest-driven secure I/O enlightenment?
> > 
> > "Connect" is state of the link that can be automatically maintained
> > over events like reset and error recovery. The guest is responsible
> > for Bind / Unbind.
> > 
> > Now, the host can optionally "Connect" in response to a "Bind" event,
> > but it is not clear that such a mechanism is needed. It likely is
> > going to depend on how error handling is implemented, and whether an
> > event that causes disconnect can be recovered. We may get there, but
> > likely not in the initial phases of the implementation.
> > 
> > > In the last device security meeting, you said the sysfs interface
> > > was mostly for higher level software stacks, like virt-manager. I
> > > was wondering what would be the picture looks like when coping these
> > > statement with the guest-driven model. Are we expecting the device
> > > connect triggered by QEMU when extracting the guest request from the
> > > secure channel in this case?
> > 
> > I think it is simplest for now if "Connect" is a pre-requisite for
> > guest-triggered "Bind".
> >
> 
> Thanks so much for the reply. 
> 
> IIUC, it means a guest assumes the device is in "connect" state when
> noticing a TDI, and it has the awareness that the "connect" state will
> be taken care by host until it needs to step in error recovery?
> 
> I am more thinking of how device driver talks with the PCI core.
> 
> +static int pci_tsm_connect(struct pci_dev *pdev)
> +{
> +	struct pci_tsm_req *req __free(req_free) = NULL;
> +
> +	/* opportunistic state checks to skip allocating a request */
> +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +		return 0;
> +
> 
> As this patch is triggered by userspace through sysfs, I am wondering
> would it be a good idea to let the device driver step in around the
> device connect/disconnect flow in the future? as a device might needs to
> be switched to different states before it is ready to handle SPDM and
> IDE.

This capability needs a definite use case for the kernel to onboard the
complexity, and notifiers in general increase the maintenance burden.
This PCI/TSM proposal is that "connect" can happen independent of any
driver being loaded. If a driver wants to trigger reconnect after it
loads it can, but it is not clear why it would need to get its fingers
into the "connect" process itself. I.e. I would hope that at maximum an
endpoint driver just needs to see the results of "connect", or trigger
reconnect, not intercept the connect flow.

If the connect state is treated like a link state then it is reasonable
to expect that the PCI core restores the link after reset. If that is
the case then the driver's mechanism to trigger reconnect is to call
pci_reset_function() which implicitly handles reconnecting the device.

> Maybe the PCI core (pci_tsm_{connect, disconnect, error_handling}())
> should broadcast the event through a notifier when checking the connect
> state. An example kernel user of that notifier can forward the event to
> the userspace as udev events via PF_NETLINK.

Drivers can already register for ->reset_prepare() and ->reset_done()
notifiers. It would be great to not need to invent new notification
infrastructure, at least without an exceedingly good reason.

> > the attestation information needs to have multiple validation entry
> > points for userspace, PCI core, and endpoint drivers to each have a
> > chance to deploy some attestation policy. Some of these questions
> > will need have common answers developed between the native CMA
> > implementation and the TSM implementation since CMA needs to solve
> > some of ABI issues of making measurements available to attestation
> > agents.
> > 
> > At Plumbers I had been thinking "golden measurements" injected into
> > the kernel ahead of interface validation gives the kernel the most
> > autonomy, but questions about measurement nonce and other concerns
> > tempered my thinking there. Plenty to still talk about and navigate.
> 
> Yes. We had been discussing that a lot. :) I also carefully watched the
> playback of LPC Confidential MC, quite a lot nice discussions. Out
> of curiosity, were folks think eBPF a good fit or an overkill here? :)

Probably too early to settle that, we are still trying to figure out how
to log native measurements [1] and the logging mechanism [2]. eBPF only
comes after memcmp() is deemed insufficient, but even memcmp() needs to
plumb the measurements through an attestation mechanism.

[1]: http://lore.kernel.org/r/20240222154529.GA9465@wunner.de
[2]: http://lore.kernel.org/r/20240128212532.2754325-1-sameo@rivosinc.com

On Tue, 30 Jan 2024 01:24:14 -0800
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 interface definition builds upon CMA, component measurement and
> authentication, and IDE, link integrity and data encryption. It adds
> support for establishing virtual functions within a device that can be
> assigned 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 device security manager (DSM) and
> system software in both a VMM and a VM. 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.
> 
> Similar to the PCI core extensions to support CONFIG_PCI_CMA,
> CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
> attribute, and add more properties + controls in a tsm/ subdirectory of
> the PCI device sysfs interface. Unlike CMA that can depend on a local to
> the PCI core implementation, PCI_TSM needs to be prepared for late
> loading of the platform TSM driver. Consider that the TSM driver may
> itself be a PCI driver. Userspace can depend on the common TSM device
> uevent to know when the PCI core has TSM services enabled. The PCI
> device tsm/ subdirectory is supplemented by the TSM device pci/
> directory for platform global TSM properties + controls.
> 
> All vendor TSM implementations share the property of asking the VMM to
> perform DOE mailbox operations on behalf of the TSM. That common
> capability is centralized in PCI core code that invokes an ->exec()
> operation callback potentially multiple times to service a given request
> (struct pci_tsm_req). Future operations / verbs will be handled
> similarly with the "request + exec" model. For now, only "connect" and
> "disconnect" are implemented which at a minimum is expected to establish
> IDE for the link.
> 
> In addition to requests the low-level TSM implementation is notified of
> device arrival and departure events so that it can filter devices that
> the TSM is not prepared to support, or otherwise setup and teardown
> per-device context.
> 
> Cc: Wu Hao <hao.wu@intel.com>
> Cc: Yilun Xu <yilun.xu@intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Superficial comments inline - noticed whilst getting my head
around the basic structure.




> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> new file mode 100644
> index 000000000000..f74de0ee49a0
> --- /dev/null
> +++ b/drivers/pci/tsm.c
> @@ -0,0 +1,346 @@

> +
> +DEFINE_FREE(req_free, struct pci_tsm_req *, if (_T) tsm_ops->req_free(_T))
> +
> +static int pci_tsm_disconnect(struct pci_dev *pdev)
> +{
> +	struct pci_tsm_req *req __free(req_free) = NULL;

As below. I'll stop commenting on these.

> +
> +	/* opportunistic state checks to skip allocating a request */
> +	if (pdev->tsm->state < PCI_TSM_CONNECT)
> +		return 0;
> +
> +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_DISCONNECT);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
> +		enum pci_tsm_op_status status;
> +
> +		/* revalidate state */
> +		if (pdev->tsm->state < PCI_TSM_CONNECT)
> +			return 0;
> +		if (pdev->tsm->state < PCI_TSM_INIT)
> +			return -ENXIO;
> +
> +		do {
> +			status = tsm_ops->exec(pdev, req);
> +			req->seq++;
> +			/* TODO: marshal SPDM request */
> +		} while (status == PCI_TSM_SPDM_REQ);
> +
> +		if (status == PCI_TSM_FAIL)
> +			return -EIO;
> +		pdev->tsm->state = PCI_TSM_INIT;
> +	}
> +	return 0;
> +}
> +
> +static int pci_tsm_connect(struct pci_dev *pdev)
> +{
> +	struct pci_tsm_req *req __free(req_free) = NULL;

Push down a few lines to put it where the allocation happens.

> +
> +	/* opportunistic state checks to skip allocating a request */
> +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +		return 0;
> +
> +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_CONNECT);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {

What could possibly go wrong? Everyone loves scoped_cond_guard ;)

> +		enum pci_tsm_op_status status;
> +
> +		/* revalidate state */
> +		if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +			return 0;
> +		if (pdev->tsm->state < PCI_TSM_INIT)
> +			return -ENXIO;
> +
> +		do {
> +			status = tsm_ops->exec(pdev, req);
> +			req->seq++;
> +		} while (status == PCI_TSM_SPDM_REQ);
> +
> +		if (status == PCI_TSM_FAIL)
> +			return -EIO;
> +		pdev->tsm->state = PCI_TSM_CONNECT;
> +	}
> +	return 0;
> +}

...

> + size_t connect_mode_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t len)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int i;
> +
> +	guard(mutex)(tsm_ops->lock);
> +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> +		return -EBUSY;
> +	for (i = 0; i < ARRAY_SIZE(pci_tsm_modes); i++)
> +		if (sysfs_streq(buf, pci_tsm_modes[i]))
> +			break;
> +	if (i == PCI_TSM_MODE_LINK) {
> +		if (pdev->tsm->link_capable)
> +			pdev->tsm->mode = PCI_TSM_MODE_LINK;

Error in all real paths paths?
Also, maybe a switch will be more sensible here.

> +		return -EOPNOTSUPP;
> +	} else if (i == PCI_TSM_MODE_SELECTIVE) {
> +		if (pdev->tsm->selective_capable)
> +			pdev->tsm->mode = PCI_TSM_MODE_SELECTIVE;
> +		return -EOPNOTSUPP;
> +	} else
> +		return -EINVAL;
> +	return len;
> +}


> +
> +void pci_tsm_init(struct pci_dev *pdev)
> +{
> +	u16 ide_cap;
> +	int rc;
> +
> +	if (!pdev->cma_capable)
> +		return;
> +
> +	ide_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_IDE);
> +	if (!ide_cap)
> +		return;
> +
> +	struct pci_tsm *tsm __free(kfree) = kzalloc(sizeof(*tsm), GFP_KERNEL);
> +	if (!tsm)
> +		return;
> +
> +	tsm->ide_cap = ide_cap;
> +
> +	rc = xa_insert(&pci_tsm_devs, (unsigned long)pdev, pdev, GFP_KERNEL);

This is an unusual pattern with the key and the value matching.
Feels like xarray might for once not be the best choice of structure.

> +	if (rc) {
> +		pci_dbg(pdev, "failed to register tsm capable device\n");
> +		return;
> +	}
> +
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	pdev->tsm = no_free_ptr(tsm);
> +	pci_tsm_add(pdev);
> +}
> +
> +void pci_tsm_destroy(struct pci_dev *pdev)
> +{
> +	guard(rwsem_write)(&pci_tsm_rwsem);
> +	pci_tsm_del(pdev);
> +	xa_erase(&pci_tsm_devs, (unsigned long)pdev);
> +	kfree(pdev->tsm);
> +	pdev->tsm = NULL;
> +}



> diff --git a/drivers/virt/coco/tsm/class.c b/drivers/virt/coco/tsm/class.c
> index a569fa6b09eb..a459e51c0892 100644
> --- a/drivers/virt/coco/tsm/class.c
> +++ b/drivers/virt/coco/tsm/class.c

> +static const struct attribute_group *tsm_attr_groups[] = {
> +#ifdef CONFIG_PCI_TSM
> +	&tsm_pci_attr_group,
> +#endif
> +	NULL,
Trivial but, no point in that comma as we will never chase it with anything.
> +};
> +
>  static int __init tsm_init(void)
>  {
>  	tsm_class = class_create("tsm");
> @@ -86,6 +97,7 @@ static int __init tsm_init(void)
>  		return PTR_ERR(tsm_class);
>  
>  	tsm_class->dev_release = tsm_release;
> +	tsm_class->dev_groups = tsm_attr_groups;
>  	return 0;
>  }
>  module_init(tsm_init)
> diff --git a/drivers/virt/coco/tsm/pci.c b/drivers/virt/coco/tsm/pci.c
> new file mode 100644
> index 000000000000..b3684ad7114f
> --- /dev/null
> +++ b/drivers/virt/coco/tsm/pci.c

> +
> +static bool tsm_pci_group_visible(struct kobject *kobj)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct tsm_subsys *subsys = container_of(dev, typeof(*subsys), dev);
Give this a macro probably.
 dev_to_tsm_subsys(kobj_to_dev(kobj));

> +
> +	if (subsys->info->pci_ops)
> +		return true;

	return subsys->info->pci->ops;
maybe

> +	return false;
> +}
> +DEFINE_SYSFS_GROUP_VISIBLE(tsm_pci);
> +
> +static struct attribute *tsm_pci_attrs[] = {
> +	&dev_attr_link_capable.attr,
> +	&dev_attr_selective_streams.attr,
> +	NULL,
> +};
> +
> +const struct attribute_group tsm_pci_attr_group = {
> +	.name = "pci",
> +	.attrs = tsm_pci_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(tsm_pci),
> +};
> +
> +int tsm_pci_init(const struct tsm_info *info)
> +{
> +	if (!info->pci_ops)
> +		return 0;
> +
> +	return pci_tsm_register(info->pci_ops);
> +}
> +
> +void tsm_pci_destroy(const struct tsm_info *info)
> +{
> +	pci_tsm_unregister(info->pci_ops);
> +}
> diff --git a/drivers/virt/coco/tsm/tsm.h b/drivers/virt/coco/tsm/tsm.h
> new file mode 100644
> index 000000000000..407c388a109b
> --- /dev/null
> +++ b/drivers/virt/coco/tsm/tsm.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __TSM_CORE_H
> +#define __TSM_CORE_H
> +
> +#include <linux/device.h>
> +
> +struct tsm_info;
> +struct tsm_subsys {
> +	struct device dev;
> +	const struct tsm_info *info;
> +};

I know people like to build up patch sets, but I think defining this here
from patch 3 would just reduce noise.

> +
> +#ifdef CONFIG_PCI_TSM
> +int tsm_pci_init(const struct tsm_info *info);
> +void tsm_pci_destroy(const struct tsm_info *info);
> +extern const struct attribute_group tsm_pci_attr_group;
> +#else
> +static inline int tsm_pci_init(const struct tsm_info *info)
> +{
> +	return 0;
> +}
> +static inline void tsm_pci_destroy(const struct tsm_info *info)
> +{
> +}
> +#endif
> +
> +#endif /* TSM_CORE_H */

> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index 8cb8a661ba41..f5dbdfa65d8d 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -4,11 +4,15 @@
>  
>  #include <linux/sizes.h>
>  #include <linux/types.h>
> +#include <linux/mutex.h>

struct mutex;
instead given you only have a pointer I think.


>  
>  struct tsm_info {
>  	const char *name;
>  	struct device *host;
>  	const struct attribute_group **groups;
> +	const struct tsm_pci_ops *pci_ops;
> +	unsigned int nr_selective_streams;
> +	unsigned int link_stream_capable:1;
>  };


> +struct pci_dev;
> +/**
> + * struct tsm_pci_ops - Low-level TSM-exported interface to the PCI core
> + * @add: accept device for tsm operation, locked
> + * @del: teardown tsm context for @pdev, locked
> + * @req_alloc: setup context for given operation, unlocked
> + * @req_free: teardown context for given request, unlocked
> + * @exec: run @req, may be invoked multiple times per @req, locked
> + * @lock: tsm work is one device and one op at a time
> + */
> +struct tsm_pci_ops {
> +	int (*add)(struct pci_dev *pdev);
> +	void (*del)(struct pci_dev *pdev);
> +	struct pci_tsm_req *(*req_alloc)(struct pci_dev *pdev,
> +					 enum pci_tsm_op op);
> +	struct pci_tsm_req *(*req_free)(struct pci_tsm_req *req);
> +	enum pci_tsm_op_status (*exec)(struct pci_dev *pdev,
> +				       struct pci_tsm_req *req);
> +	struct mutex *lock;

Mixing data with what would otherwise be likely to be constant pointers
probably best avoided.  Maybe wrap the lock in another op instead?


> +};

Jonathan Cameron wrote:
> On Tue, 30 Jan 2024 01:24:14 -0800
> 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 interface definition builds upon CMA, component measurement and
> > authentication, and IDE, link integrity and data encryption. It adds
> > support for establishing virtual functions within a device that can be
> > assigned 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 device security manager (DSM) and
> > system software in both a VMM and a VM. 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.
> > 
> > Similar to the PCI core extensions to support CONFIG_PCI_CMA,
> > CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
> > attribute, and add more properties + controls in a tsm/ subdirectory of
> > the PCI device sysfs interface. Unlike CMA that can depend on a local to
> > the PCI core implementation, PCI_TSM needs to be prepared for late
> > loading of the platform TSM driver. Consider that the TSM driver may
> > itself be a PCI driver. Userspace can depend on the common TSM device
> > uevent to know when the PCI core has TSM services enabled. The PCI
> > device tsm/ subdirectory is supplemented by the TSM device pci/
> > directory for platform global TSM properties + controls.
> > 
> > All vendor TSM implementations share the property of asking the VMM to
> > perform DOE mailbox operations on behalf of the TSM. That common
> > capability is centralized in PCI core code that invokes an ->exec()
> > operation callback potentially multiple times to service a given request
> > (struct pci_tsm_req). Future operations / verbs will be handled
> > similarly with the "request + exec" model. For now, only "connect" and
> > "disconnect" are implemented which at a minimum is expected to establish
> > IDE for the link.
> > 
> > In addition to requests the low-level TSM implementation is notified of
> > device arrival and departure events so that it can filter devices that
> > the TSM is not prepared to support, or otherwise setup and teardown
> > per-device context.
> > 
> > Cc: Wu Hao <hao.wu@intel.com>
> > Cc: Yilun Xu <yilun.xu@intel.com>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Cc: Alexey Kardashevskiy <aik@amd.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Superficial comments inline - noticed whilst getting my head
> around the basic structure.
> 
> 
> 
> 
> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> > new file mode 100644
> > index 000000000000..f74de0ee49a0
> > --- /dev/null
> > +++ b/drivers/pci/tsm.c
> > @@ -0,0 +1,346 @@
> 
> > +
> > +DEFINE_FREE(req_free, struct pci_tsm_req *, if (_T) tsm_ops->req_free(_T))
> > +
> > +static int pci_tsm_disconnect(struct pci_dev *pdev)
> > +{
> > +	struct pci_tsm_req *req __free(req_free) = NULL;
> 
> As below. I'll stop commenting on these.

Hey, they are fair game, will find a way to rework this and not use the
confusing pattern.

> > +
> > +	/* opportunistic state checks to skip allocating a request */
> > +	if (pdev->tsm->state < PCI_TSM_CONNECT)
> > +		return 0;
> > +
> > +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_DISCONNECT);
> > +	if (!req)
> > +		return -ENOMEM;
> > +
> > +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
> > +		enum pci_tsm_op_status status;
> > +
> > +		/* revalidate state */
> > +		if (pdev->tsm->state < PCI_TSM_CONNECT)
> > +			return 0;
> > +		if (pdev->tsm->state < PCI_TSM_INIT)
> > +			return -ENXIO;
> > +
> > +		do {
> > +			status = tsm_ops->exec(pdev, req);
> > +			req->seq++;
> > +			/* TODO: marshal SPDM request */
> > +		} while (status == PCI_TSM_SPDM_REQ);
> > +
> > +		if (status == PCI_TSM_FAIL)
> > +			return -EIO;
> > +		pdev->tsm->state = PCI_TSM_INIT;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int pci_tsm_connect(struct pci_dev *pdev)
> > +{
> > +	struct pci_tsm_req *req __free(req_free) = NULL;
> 
> Push down a few lines to put it where the allocation happens.
> 
> > +
> > +	/* opportunistic state checks to skip allocating a request */
> > +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> > +		return 0;
> > +
> > +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_CONNECT);
> > +	if (!req)
> > +		return -ENOMEM;
> > +
> > +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
> 
> What could possibly go wrong? Everyone loves scoped_cond_guard ;)

Indeed.

> 
> > +		enum pci_tsm_op_status status;
> > +
> > +		/* revalidate state */
> > +		if (pdev->tsm->state >= PCI_TSM_CONNECT)
> > +			return 0;
> > +		if (pdev->tsm->state < PCI_TSM_INIT)
> > +			return -ENXIO;
> > +
> > +		do {
> > +			status = tsm_ops->exec(pdev, req);
> > +			req->seq++;
> > +		} while (status == PCI_TSM_SPDM_REQ);
> > +
> > +		if (status == PCI_TSM_FAIL)
> > +			return -EIO;
> > +		pdev->tsm->state = PCI_TSM_CONNECT;
> > +	}
> > +	return 0;
> > +}
> 
> ...
> 
> > + size_t connect_mode_store(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  const char *buf, size_t len)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	int i;
> > +
> > +	guard(mutex)(tsm_ops->lock);
> > +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> > +		return -EBUSY;
> > +	for (i = 0; i < ARRAY_SIZE(pci_tsm_modes); i++)
> > +		if (sysfs_streq(buf, pci_tsm_modes[i]))
> > +			break;
> > +	if (i == PCI_TSM_MODE_LINK) {
> > +		if (pdev->tsm->link_capable)
> > +			pdev->tsm->mode = PCI_TSM_MODE_LINK;
> 
> Error in all real paths paths?

Uh... yeah, did I mention that this untested. Will fix.

> Also, maybe a switch will be more sensible here.

Sure.

> 
> > +		return -EOPNOTSUPP;
> > +	} else if (i == PCI_TSM_MODE_SELECTIVE) {
> > +		if (pdev->tsm->selective_capable)
> > +			pdev->tsm->mode = PCI_TSM_MODE_SELECTIVE;
> > +		return -EOPNOTSUPP;
> > +	} else
> > +		return -EINVAL;
> > +	return len;
> > +}
> 
> 
> > +
> > +void pci_tsm_init(struct pci_dev *pdev)
> > +{
> > +	u16 ide_cap;
> > +	int rc;
> > +
> > +	if (!pdev->cma_capable)
> > +		return;
> > +
> > +	ide_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_IDE);
> > +	if (!ide_cap)
> > +		return;
> > +
> > +	struct pci_tsm *tsm __free(kfree) = kzalloc(sizeof(*tsm), GFP_KERNEL);
> > +	if (!tsm)
> > +		return;
> > +
> > +	tsm->ide_cap = ide_cap;
> > +
> > +	rc = xa_insert(&pci_tsm_devs, (unsigned long)pdev, pdev, GFP_KERNEL);
> 
> This is an unusual pattern with the key and the value matching.
> Feels like xarray might for once not be the best choice of structure.

This is the "use xarray instead of a linked list patterni". It would be
useful to maybe make the key be the Segment+BDF, but I did not take the
time to figure out if that can fit in an unsigned long. In the meantime
this saves needing to embed a linked list node in 'struct pci_dev'.

[..]
> > diff --git a/drivers/virt/coco/tsm/class.c b/drivers/virt/coco/tsm/class.c
> > index a569fa6b09eb..a459e51c0892 100644
> > --- a/drivers/virt/coco/tsm/class.c
> > +++ b/drivers/virt/coco/tsm/class.c
> 
> > +static const struct attribute_group *tsm_attr_groups[] = {
> > +#ifdef CONFIG_PCI_TSM
> > +	&tsm_pci_attr_group,
> > +#endif
> > +	NULL,
> Trivial but, no point in that comma as we will never chase it with anything.
> > +};
> > +
> >  static int __init tsm_init(void)
> >  {
> >  	tsm_class = class_create("tsm");
> > @@ -86,6 +97,7 @@ static int __init tsm_init(void)
> >  		return PTR_ERR(tsm_class);
> >  
> >  	tsm_class->dev_release = tsm_release;
> > +	tsm_class->dev_groups = tsm_attr_groups;
> >  	return 0;
> >  }
> >  module_init(tsm_init)
> > diff --git a/drivers/virt/coco/tsm/pci.c b/drivers/virt/coco/tsm/pci.c
> > new file mode 100644
> > index 000000000000..b3684ad7114f
> > --- /dev/null
> > +++ b/drivers/virt/coco/tsm/pci.c
> 
> > +
> > +static bool tsm_pci_group_visible(struct kobject *kobj)
> > +{
> > +	struct device *dev = kobj_to_dev(kobj);
> > +	struct tsm_subsys *subsys = container_of(dev, typeof(*subsys), dev);
> Give this a macro probably.
>  dev_to_tsm_subsys(kobj_to_dev(kobj));

Sure.

> 
> > +
> > +	if (subsys->info->pci_ops)
> > +		return true;
> 
> 	return subsys->info->pci->ops;
> maybe

True, maybe with a !! for good measure.

[..]
> > diff --git a/drivers/virt/coco/tsm/tsm.h b/drivers/virt/coco/tsm/tsm.h
> > new file mode 100644
> > index 000000000000..407c388a109b
> > --- /dev/null
> > +++ b/drivers/virt/coco/tsm/tsm.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __TSM_CORE_H
> > +#define __TSM_CORE_H
> > +
> > +#include <linux/device.h>
> > +
> > +struct tsm_info;
> > +struct tsm_subsys {
> > +	struct device dev;
> > +	const struct tsm_info *info;
> > +};
> 
> I know people like to build up patch sets, but I think defining this here
> from patch 3 would just reduce noise.

Ok.

> 
> > +
> > +#ifdef CONFIG_PCI_TSM
> > +int tsm_pci_init(const struct tsm_info *info);
> > +void tsm_pci_destroy(const struct tsm_info *info);
> > +extern const struct attribute_group tsm_pci_attr_group;
> > +#else
> > +static inline int tsm_pci_init(const struct tsm_info *info)
> > +{
> > +	return 0;
> > +}
> > +static inline void tsm_pci_destroy(const struct tsm_info *info)
> > +{
> > +}
> > +#endif
> > +
> > +#endif /* TSM_CORE_H */
> 
> > diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> > index 8cb8a661ba41..f5dbdfa65d8d 100644
> > --- a/include/linux/tsm.h
> > +++ b/include/linux/tsm.h
> > @@ -4,11 +4,15 @@
> >  
> >  #include <linux/sizes.h>
> >  #include <linux/types.h>
> > +#include <linux/mutex.h>
> 
> struct mutex;
> instead given you only have a pointer I think.

True, but see below I expect this lock to move somewhere else in the
next version.

> 
> 
> >  
> >  struct tsm_info {
> >  	const char *name;
> >  	struct device *host;
> >  	const struct attribute_group **groups;
> > +	const struct tsm_pci_ops *pci_ops;
> > +	unsigned int nr_selective_streams;
> > +	unsigned int link_stream_capable:1;
> >  };
> 
> 
> > +struct pci_dev;
> > +/**
> > + * struct tsm_pci_ops - Low-level TSM-exported interface to the PCI core
> > + * @add: accept device for tsm operation, locked
> > + * @del: teardown tsm context for @pdev, locked
> > + * @req_alloc: setup context for given operation, unlocked
> > + * @req_free: teardown context for given request, unlocked
> > + * @exec: run @req, may be invoked multiple times per @req, locked
> > + * @lock: tsm work is one device and one op at a time
> > + */
> > +struct tsm_pci_ops {
> > +	int (*add)(struct pci_dev *pdev);
> > +	void (*del)(struct pci_dev *pdev);
> > +	struct pci_tsm_req *(*req_alloc)(struct pci_dev *pdev,
> > +					 enum pci_tsm_op op);
> > +	struct pci_tsm_req *(*req_free)(struct pci_tsm_req *req);
> > +	enum pci_tsm_op_status (*exec)(struct pci_dev *pdev,
> > +				       struct pci_tsm_req *req);
> > +	struct mutex *lock;
> 
> Mixing data with what would otherwise be likely to be constant pointers
> probably best avoided.  Maybe wrap the lock in another op instead?

After chatting with Alexey this lock is too coarse and will move to a
per device lock rather than locking the entire interface.