From: Dan Williams <dan.j.williams@intel.com>
To: Alexey Kardashevskiy <aik@amd.com>,
Dan Williams <dan.j.williams@intel.com>,
<linux-coco@lists.linux.dev>
Cc: Wu Hao <hao.wu@intel.com>, Yilun Xu <yilun.xu@intel.com>,
Lukas Wunner <lukas@wunner.de>, Samuel Ortiz <sameo@rivosinc.com>,
Bjorn Helgaas <bhelgaas@google.com>,
Xu Yilun <yilun.xu@linux.intel.com>, <kevin.tian@intel.com>,
<gregkh@linuxfoundation.org>, <linux-pci@vger.kernel.org>
Subject: Re: [RFC PATCH v2 5/6] PCI/TSM: Authenticate devices via platform TSM
Date: Fri, 26 Apr 2024 19:58:51 -0700 [thread overview]
Message-ID: <662c69eb6dbf1_b6e0294d1@dwillia2-mobl3.amr.corp.intel.com.notmuch> (raw)
In-Reply-To: <fc201452-080e-4942-b5a0-0c64d023ac6b@amd.com>
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@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>
> > Co-developed-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
[..]
> > 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.
next prev parent reply other threads:[~2024-04-27 2:58 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-12 8:51 [RFC PATCH v2 0/6] Towards a shared TSM sysfs-ABI for Confidential Computing Dan Williams
2024-04-12 8:51 ` [RFC PATCH v2 1/6] configfs-tsm: Namespace TSM report symbols Dan Williams
2024-04-12 8:51 ` [RFC PATCH v2 2/6] coco/guest: Move shared guest CC infrastructure to drivers/virt/coco/guest/ Dan Williams
2024-04-12 8:52 ` [RFC PATCH v2 3/6] x86/tdx: Introduce a "tdx" subsystem and "tsm" device Dan Williams
2024-04-12 8:52 ` [RFC PATCH v2 4/6] coco/tsm: Introduce a class device for TEE Security Managers Dan Williams
2024-04-12 8:52 ` [RFC PATCH v2 5/6] PCI/TSM: Authenticate devices via platform TSM Dan Williams
2024-04-13 3:14 ` kernel test robot
2024-04-13 7:34 ` kernel test robot
2024-04-13 11:11 ` kernel test robot
2024-04-19 22:07 ` Bjorn Helgaas
2024-04-27 1:27 ` Dan Williams
2024-04-22 2:21 ` Alexey Kardashevskiy
2024-04-27 2:58 ` Dan Williams [this message]
2024-05-06 15:14 ` Xu Yilun
2024-05-07 18:21 ` Dan Williams
2024-05-08 2:21 ` Xu Yilun
2024-05-07 8:46 ` Xu Yilun
2024-05-07 18:28 ` Dan Williams
2024-05-14 17:13 ` Zhi Wang
2024-04-12 8:52 ` [RFC PATCH v2 6/6] tdx_tsm: TEE Security Manager driver for TDX Dan Williams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=662c69eb6dbf1_b6e0294d1@dwillia2-mobl3.amr.corp.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=aik@amd.com \
--cc=bhelgaas@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=hao.wu@intel.com \
--cc=kevin.tian@intel.com \
--cc=linux-coco@lists.linux.dev \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=sameo@rivosinc.com \
--cc=yilun.xu@intel.com \
--cc=yilun.xu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.