On Tue, 30 Jan 2024 01:24:02 -0800
Dan Williams <dan.j.williams@xxxxxxxxx> 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@xxxxxxxxx>
> Cc: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> Cc: Alexey Kardashevskiy <aik@xxxxxxx>
> Cc: Wu Hao <hao.wu@xxxxxxxxx>
> Cc: Yilun Xu <yilun.xu@xxxxxxxxx>
> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Cc: John Allen <john.allen@xxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
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);
