From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: <linux-coco@lists.linux.dev>, Wu Hao <hao.wu@intel.com>,
Yilun Xu <yilun.xu@intel.com>, Lukas Wunner <lukas@wunner.de>,
Samuel Ortiz <sameo@rivosinc.com>,
Alexey Kardashevskiy <aik@amd.com>,
Bjorn Helgaas <bhelgaas@google.com>, <linux-pci@vger.kernel.org>,
<gregkh@linuxfoundation.org>
Subject: Re: [RFC PATCH 5/5] PCI/TSM: Authenticate devices via platform TSM
Date: Thu, 7 Mar 2024 17:18:10 +0000 [thread overview]
Message-ID: <20240307171810.0000396c@Huawei.com> (raw)
In-Reply-To: <170660665391.224441.13963835575448844460.stgit@dwillia2-xfh.jf.intel.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?
> +};
next prev parent reply other threads:[~2024-03-07 17:18 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-30 9:23 [RFC PATCH 0/5] Towards a shared TSM sysfs-ABI for Confidential Computing Dan Williams
2024-01-30 9:23 ` [RFC PATCH 1/5] PCI/CMA: Prepare to interoperate with TSM authentication Dan Williams
2024-02-08 22:09 ` Bjorn Helgaas
2024-01-30 9:23 ` [RFC PATCH 2/5] coco/tsm: Establish a new coco/tsm subdirectory Dan Williams
2024-02-09 2:24 ` Kuppuswamy Sathyanarayanan
2024-02-27 1:39 ` Dan Williams
2024-01-30 9:24 ` [RFC PATCH 3/5] coco/tsm: Introduce a shared class device for TSMs Dan Williams
2024-02-16 11:29 ` Alexey Kardashevskiy
2024-02-27 1:47 ` Dan Williams
2024-03-07 16:41 ` Jonathan Cameron
2024-03-07 19:33 ` Dan Williams
2024-01-30 9:24 ` [RFC PATCH 4/5] sysfs: Introduce a mechanism to hide static attribute_groups Dan Williams
2024-01-30 16:44 ` Greg KH
2024-01-30 16:48 ` Dan Williams
2024-01-30 17:31 ` Greg KH
2024-02-19 8:57 ` Greg KH
2024-02-22 13:22 ` Greg KH
2024-01-30 9:24 ` [RFC PATCH 5/5] PCI/TSM: Authenticate devices via platform TSM Dan Williams
2024-02-08 22:13 ` Bjorn Helgaas
2024-02-09 5:51 ` Dan Williams
2024-02-16 11:29 ` Alexey Kardashevskiy
2024-02-27 5:52 ` Dan Williams
2024-02-16 21:38 ` Alexey Kardashevskiy
2024-02-27 5:59 ` Dan Williams
2024-02-26 11:37 ` Zhi Wang
2024-02-27 6:34 ` Dan Williams
2024-02-27 19:53 ` Zhi Wang
2024-03-01 0:32 ` Dan Williams
2024-03-07 17:18 ` Jonathan Cameron [this message]
2024-03-07 19:51 ` 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=20240307171810.0000396c@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=aik@amd.com \
--cc=bhelgaas@google.com \
--cc=dan.j.williams@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=hao.wu@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 \
/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.