Alexey Kardashevskiy wrote:
> On 12/4/24 18:52, 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 Component Measurement and
> > Authentication (CMA), and link Integrity and Data Encryption (IDE). It
> > adds support for assigning devices (PCI physical or virtual function) to
> > a confidential VM such that the assigned device is enabled to access
> > guest private memory protected by technologies like Intel TDX, AMD
> > SEV-SNP, RISCV COVE, or ARM CCA.
> >
> > The "TSM" (TEE Security Manager) is a concept in the TDISP specification
> > of an agent that mediates between a "DSM" (Device Security Manager) and
> > system software in both a VMM and a confidential VM. A VMM uses TSM ABIs
> > to setup link security and assign devices. A confidential VM uses TSM
> > ABIs to transition an assigned device into the TDISP "RUN" state and
> > validate its configuration. From a Linux perspective the TSM abstracts
> > many of the details of TDISP, IDE, and CMA. Some of those details leak
> > through at times, but for the most part TDISP is an internal
> > implementation detail of the TSM.
> >
> > 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.
> >
> > The common verbs that the low-level TSM drivers implement are defined by
> > 'enum pci_tsm_cmd'. For now only connect and disconnect are defined for
> > establishing a trust relationship between the host and the device,
> > securing the interconnect (optionally establishing IDE), and tearing
> > that down.
> >
> > The locking allows for multiple devices to be executing commands
> > simultaneously, one outstanding command per-device and an rwsem flushes
> > all inflight commands when a TSM low-level driver/device is removed.
> >
> > In addition to commands submitted through an 'exec' operation the
> > low-level TSM driver is notified of device arrival and departure events
> > via 'add' and 'del' operations. With those it can setup per-device
> > context, or filter devices that the TSM is not prepared to support.
> >
> > Cc: Wu Hao <hao.wu@xxxxxxxxx>
> > Cc: Yilun Xu <yilun.xu@xxxxxxxxx>
> > Cc: Lukas Wunner <lukas@xxxxxxxxx>
> > Cc: Samuel Ortiz <sameo@xxxxxxxxxxxx>
> > Cc: Alexey Kardashevskiy <aik@xxxxxxx>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Co-developed-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
[..]
> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> > new file mode 100644
> > index 000000000000..9c5fb2c46662
> > --- /dev/null
> > +++ b/drivers/pci/tsm.c
> > @@ -0,0 +1,270 @@
> > +// 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/pci-doe.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/xarray.h>
> > +#include <linux/pci-tsm.h>
> > +#include <linux/bitfield.h>
> > +#include "pci.h"
> > +
> > +/* collect TSM capable devices to rendezvous with the tsm driver */
> > +static DEFINE_XARRAY(pci_tsm_devs);
>
> imho either this or pci_dev::tsm is enough but not necessarily both.
You mean:
s/pci_tsm_devs/tsm_devs/
?
[..]
> > +void pci_tsm_init(struct pci_dev *pdev)
> > +{
> > + bool tee_cap;
> > + u16 ide_cap;
> > + int rc;
> > +
> > + ide_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_IDE);
> > + tee_cap = pdev->devcap & PCI_EXP_DEVCAP_TEE;
> > + if (ide_cap || tee_cap)
>
> I'd swap if with else.
Oh, you mean:
if (!(ide_cap || tee_cap))
return;
?
>
> > + pci_dbg(pdev,
> > + "Device security capabailities detected (%s%s ), init TSM\n",
>
> capabailities
Incapabail of spelling correctly apparently. checkpatch spell check let
me down.
Fixed.
>
> > + ide_cap ? " ide" : "", tee_cap ? " tee" : "");
> > + else
> > + return;
>
>
> If (!ide_cap && tee_cap), we get here but doing the below does not make
> sense for TEE (which are likely to be VFs).
The "!ide_cap && tee_cap" case may also be the "TSM wants to setup IDE
without TDISP flow".
[..]
> > +struct pci_dev;
> > +/**
> > + * struct pci_tsm_ops - Low-level TSM-exported interface to the PCI core
> > + * @add: accept device for tsm operation
>
>
> What does "accept" means here? "Accept" sounds like some action needed
> from something but this is what exec() for.
At the lowest level an "accepted" device, one that returns successfully
from "->add()" and has its tsm/ attribute group in sysfs enabled.
> So far I have not noticed that allocating platform-specific structures
> prior calling a verb is any useful, i.e. having a separate state -
> PCI_TSM_INIT - is just an extra noise in the "painfully simple first
> TSM implementation".
As far as I know, not all TSM implementations care about the "!ide_cap
&& tee_cap" case. That said, regarding painfully simple, if the only
difference is some TSMs support IDE without TDISP and some do not, that
standalone-IDE support can be added later.
> > + * @del: teardown tsm context for @pdev
> > + * @exec: synchronously execute @cmd
> > + *
> > + * Note that @add, and @del run in down_write(&pci_tsm_rswem) context to
> > + * synchronize with TSM driver bind/unbind events and
> > + * pci_device_add()/pci_destroy_dev(). @exec runs in
> > + * @pdev->tsm->exec_lock context to synchronize @exec results with
> > + * @pdev->tsm->state
> > + */
> > +struct pci_tsm_ops {
> > + int (*add)(struct pci_dev *pdev);
> > + void (*del)(struct pci_dev *pdev);
> > + int (*exec)(struct pci_dev *pdev, enum pci_tsm_cmd cmd);
>
>
> A nit: the verbs seem working (especially reducing the amount of
> cut-n-paste of all this spdm forwarding) until I get to things like
> "get_status" which returns a structure to dump in the sysfs. Doing it
> like this means adding a structure in pci_tsm and manage its state
> (valid/partial/empty/...). Or we might want to generalize some input
> parameters for the verbs, adding u64 params is meh.
If it is just for sysfs cases then I would just have a facility for low
level TSM drivers to publish some attributes directly at
pci_tsm_register time. Something like the following, and just require
low level TSM implementations to agree on filenames and formats, but
otherwise avoid complicating ->exec().
diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
index 44b998a1c824..d34c3477ddc3 100644
--- a/drivers/pci/tsm.c
+++ b/drivers/pci/tsm.c
@@ -113,9 +113,9 @@ static struct attribute *pci_tsm_attrs[] = {
NULL,
};
-const struct attribute_group pci_tsm_attr_group = {
+struct attribute_group pci_tsm_attr_group = {
.name = "tsm",
- .attrs = pci_tsm_attrs,
+ .attrs = pci_tsm_default_attrs,
.is_visible = SYSFS_GROUP_VISIBLE(pci_tsm),
};
@@ -169,7 +169,7 @@ static void pci_tsm_del(struct pci_dev *pdev)
tsm_ops->del(pdev);
}
-int pci_tsm_register(const struct pci_tsm_ops *ops)
+int pci_tsm_register(const struct pci_tsm_ops *ops, struct attribute **attrs)
{
struct pci_dev *pdev;
unsigned long index;
@@ -180,6 +180,7 @@ int pci_tsm_register(const struct pci_tsm_ops *ops)
if (tsm_ops)
return -EBUSY;
tsm_ops = ops;
+ pci_tsm_attr_group.attrs = attrs;
xa_for_each(&pci_tsm_devs, index, pdev)
pci_tsm_add(pdev);
return 0;
@@ -198,6 +199,7 @@ void pci_tsm_unregister(const struct pci_tsm_ops *ops)
return;
xa_for_each(&pci_tsm_devs, index, pdev)
pci_tsm_del(pdev);
+ pci_tsm_attr_group.attrs = pci_tsm_default_attrs;
tsm_ops = NULL;
}
EXPORT_SYMBOL_GPL(pci_tsm_unregister);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 16493426a04f..dd4dc8719c5c 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -515,6 +515,9 @@ struct pci_dev {
> > #endif
> > #ifdef CONFIG_PCI_DOE
> > struct xarray doe_mbs; /* Data Object Exchange mailboxes */
> > +#endif
> > +#ifdef CONFIG_PCI_TSM
> > + struct pci_tsm *tsm; /* TSM operation state */
>
>
> I am wondering if pdev->dev.platform_data can be used for this.
No, platform_data is for ACPI or OF to populate.
For example, see sst_acpi_probe().
>
>
> > #endif
> > u16 acs_cap; /* ACS Capability offset */
> > phys_addr_t rom; /* Physical address if not from BAR */
> > @@ -550,6 +553,12 @@ static inline int pci_channel_offline(struct pci_dev *pdev)
> > return (pdev->error_state != pci_channel_io_normal);
> > }
> >
> > +/* id resources that may be shared across host-bridges */
> > +struct pci_hb_id_pool {
> > + int nr_stream_ids;
> > + int nr_cxl_cache_ids;
> > +};
> > +
> > /*
> > * Currently in ACPI spec, for each PCI host bridge, PCI Segment
> > * Group number is limited to a 16-bit value, therefore (int)-1 is
> > @@ -568,6 +577,8 @@ struct pci_host_bridge {
> > void *sysdata;
> > int busnr;
> > int domain_nr;
> > + struct pci_hb_id_pool __pool;
> > + struct pci_hb_id_pool *pool; /* &self->__pool, unless shared */
>
>
> What are these for? Something for IDE (which I also have in the works,
> very basic set of wrappers)? Thanks,
Yes, this is something I had sitting in my tree as a rough draft, but
did not intend to send out, but I guess a useful accident. Hao Wu has
taken this concept further. @Hao, lets post what we are thinking here.
The concept is that stream-ids are a limited resource. Host bridges, at
least Intel ones, might share their stream-id pool with another
host-bridge. Do AMD platforms have similar constraints?
The end goal is to be able to convey to a system owner which devices are
consuming which stream-ids and which host-bridges have available
stream-ids to allocate.
