From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: <linux-coco@lists.linux.dev>, Xiaoyao Li <xiaoyao.li@intel.com>,
"Isaku Yamahata" <isaku.yamahata@intel.com>,
Alexey Kardashevskiy <aik@amd.com>, "Wu Hao" <hao.wu@intel.com>,
Yilun Xu <yilun.xu@intel.com>,
Tom Lendacky <thomas.lendacky@amd.com>,
John Allen <john.allen@amd.com>, <linux-pci@vger.kernel.org>,
<gregkh@linuxfoundation.org>
Subject: Re: [RFC PATCH 3/5] coco/tsm: Introduce a shared class device for TSMs
Date: Thu, 7 Mar 2024 16:41:39 +0000 [thread overview]
Message-ID: <20240307164139.000049aa@Huawei.com> (raw)
In-Reply-To: <170660664287.224441.947018257307932138.stgit@dwillia2-xfh.jf.intel.com>
On Tue, 30 Jan 2024 01:24:02 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> A "tsm" is a platform component that provides an API for securely
> provisioning resources for a confidential guest (TVM) to consume. "TSM"
> also happens to be the acronym the PCI specification uses to define the
> platform agent that carries out device-security operations. That
> platform capability is commonly called TEE I/O. It is this arrival of
> TEE I/O platforms that requires the "tsm" concept to grow from a
> low-level arch-specific detail of TVM instantiation, to a frontend
> interface to mediate device setup and interact with general purpose
> kernel subsystems outside of arch/ like the PCI core.
>
> Provide a virtual (as in /sys/devices/virtual) class device interface to
> front all of the aspects of a TSM and TEE I/O that are
> cross-architecture common. This includes mechanisms like enumerating
> available platform TEE I/O capabilities and provisioning connections
> between the platform TSM and device DSMs.
>
> It is expected to handle hardware TSMs, like AMD SEV-SNP and ARM CCA
> where there is a physical TEE coprocessor device running firmware, as
> well as software TSMs like Intel TDX and RISC-V COVE, where there is a
> privileged software module loaded at runtime.
>
> For now this is just the scaffolding for registering a TSM device and/or
> TSM-specific attribute groups.
>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: Isaku Yamahata <isaku.yamahata@intel.com>
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Wu Hao <hao.wu@intel.com>
> Cc: Yilun Xu <yilun.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: John Allen <john.allen@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
A few drive by comments because I was curious.
> diff --git a/drivers/virt/coco/tsm/class.c b/drivers/virt/coco/tsm/class.c
> new file mode 100644
> index 000000000000..a569fa6b09eb
> --- /dev/null
> +++ b/drivers/virt/coco/tsm/class.c
> @@ -0,0 +1,100 @@
> +// 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/rwsem.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/cleanup.h>
> +
> +static DECLARE_RWSEM(tsm_core_rwsem);
> +struct class *tsm_class;
> +struct tsm_subsys {
> + struct device dev;
> + const struct tsm_info *info;
> +} *tsm_subsys;
> +
> +int tsm_register(const struct tsm_info *info)
> +{
> + struct device *dev __free(put_device) = NULL;
I think it would be appropriate to move this down to where
you set dev so it is moderately obvious where it comes from.
This only becomes valid cleanup after the call to device_register()
with it's device_initialize - which is ugly.
Maybe we should just use split device_initialize() / device_add()
when combining with __free(put_device);
The ideal would be something like.
struct device *dev __free(put_device) = device_initialize(&subsys->dev);
with device_initialize() returning the dev parameter.
For the really adventurous maybe even the overkill option of the following
(I know it's currently pointless complexity - given no error paths between
the kzalloc and device_initialize())
struct tsm_subsys *subsys __free(kfree) = kzalloc(sizeof(*subsys), GFP_KERNEL);
//Now safe to exit here.
struct device *dev __free(put_device) = device_initialize(&no_free_ptr(subsys)->dev);
// Also good to exit here with no double free etc though subsys is now inaccessible breaking
the one place it's used later ;)
Maybe I'm over thinking things and I doubt cleanup.h discussions
was really what you wanted from this RFC :)
> + struct tsm_subsys *subsys;
> + int rc;
> +
> + guard(rwsem_write)(&tsm_core_rwsem);
> + if (tsm_subsys) {
> + pr_warn("failed to register: \"%s\", \"%s\" already registered\n",
> + info->name, tsm_subsys->info->name);
> + return -EBUSY;
> + }
> +
> + subsys = kzalloc(sizeof(*subsys), GFP_KERNEL);
> + if (!subsys)
> + return -ENOMEM;
> +
> + subsys->info = info;
> + dev = &subsys->dev;
> + dev->class = tsm_class;
> + dev->groups = info->groups;
> + dev_set_name(dev, "tsm0");
> + rc = device_register(dev);
> + if (rc)
> + return rc;
> +
> + if (info->host) {
> + rc = sysfs_create_link(&dev->kobj, &info->host->kobj, "host");
Why not parent it from there? If it has a logical parent, that would be
nicer than using a virtual device.
> + if (rc)
> + return rc;
> + }
> +
> + /* don't auto-free @dev */
> + dev = NULL;
> + tsm_subsys = subsys;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_register);
Seems appropriate to namespace these from the start.
> +
> +void tsm_unregister(const struct tsm_info *info)
> +{
> + guard(rwsem_write)(&tsm_core_rwsem);
> + if (!tsm_subsys || info != tsm_subsys->info) {
> + pr_warn("failed to unregister: \"%s\", not currently registered\n",
> + info->name);
> + return;
> + }
> +
> + if (info->host)
> + sysfs_remove_link(&tsm_subsys->dev.kobj, "host");
> + device_unregister(&tsm_subsys->dev);
> + tsm_subsys = NULL;
> +}
> +EXPORT_SYMBOL_GPL(tsm_unregister);
next prev parent reply other threads:[~2024-03-07 16:41 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 [this message]
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
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=20240307164139.000049aa@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=aik@amd.com \
--cc=dan.j.williams@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=hao.wu@intel.com \
--cc=isaku.yamahata@intel.com \
--cc=john.allen@amd.com \
--cc=linux-coco@lists.linux.dev \
--cc=linux-pci@vger.kernel.org \
--cc=thomas.lendacky@amd.com \
--cc=xiaoyao.li@intel.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.