On Thu, 05 Dec 2024 14:23:33 -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. The
> name originates from the PCI specification for platform agent that
> carries out operations for PCIe TDISP (TEE Device Interface Security
> Protocol).
>
> Instances of this class device are parented by a device representing the
> platform security capability like CONFIG_CRYPTO_DEV_CCP or
> CONFIG_INTEL_TDX_HOST.
>
> This class device interface is a frontend to 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
> (Device Security Manager (TDISP)).
>
> For now this is just the scaffolding for registering a TSM device sysfs
> interface.
>
> Cc: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> Cc: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> Cc: Alexey Kardashevskiy <aik@xxxxxxx>
> 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 couple of generic comments inline.
> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index 819a97e8ba99..14e7cf145d85 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -14,3 +14,5 @@ source "drivers/virt/coco/tdx-guest/Kconfig"
> source "drivers/virt/coco/arm-cca-guest/Kconfig"
>
> source "drivers/virt/coco/guest/Kconfig"
> +
> +source "drivers/virt/coco/host/Kconfig"
> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index 885c9ef4e9fc..73f1b7bc5b11 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_SEV_GUEST) += sev-guest/
> obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
> obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest/
> obj-$(CONFIG_TSM_REPORTS) += guest/
> +obj-y += host/
> diff --git a/drivers/virt/coco/host/Kconfig b/drivers/virt/coco/host/Kconfig
> new file mode 100644
> index 000000000000..4fbc6ef34f12
> --- /dev/null
> +++ b/drivers/virt/coco/host/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# TSM (TEE Security Manager) Common infrastructure and host drivers
> +#
> +config TSM
> + tristate
> diff --git a/drivers/virt/coco/host/Makefile b/drivers/virt/coco/host/Makefile
> new file mode 100644
> index 000000000000..be0aba6007cd
> --- /dev/null
> +++ b/drivers/virt/coco/host/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# TSM (TEE Security Manager) Common infrastructure and host drivers
> +
> +obj-$(CONFIG_TSM) += tsm.o
> +tsm-y := tsm-core.o
> diff --git a/drivers/virt/coco/host/tsm-core.c b/drivers/virt/coco/host/tsm-core.c
> new file mode 100644
> index 000000000000..0ee738fc40ed
> --- /dev/null
> +++ b/drivers/virt/coco/host/tsm-core.c
> @@ -0,0 +1,113 @@
> +// 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);
> +static struct class *tsm_class;
> +static struct tsm_subsys {
> + struct device dev;
> +} *tsm_subsys;
This naming seems a bit confusing. To me tsm_sybsys could be:
a) The subsystem itself. So something we'd expect to remove only alongside
class destroy.
b) A subsystem of a tsm (confusing here in a subsystem for tsms). Expectation
being that a given tsm would register more than one of these.
c) What I think it is which is, which is the device added to register with
the tsm subsystem.
Mind you I'm not immediately sure what a better naming is.
tsm_class_dev maybe? Though that sounds like it should be a struct device.
> +
> +static struct tsm_subsys *
> +alloc_tsm_subsys(struct device *parent, const struct attribute_group **groups)
> +{
> + struct tsm_subsys *subsys = kzalloc(sizeof(*subsys), GFP_KERNEL);
> + struct device *dev;
> +
> + if (!subsys)
> + return ERR_PTR(-ENOMEM);
> + dev = &subsys->dev;
> + dev->parent = parent;
> + dev->groups = groups;
> + dev->class = tsm_class;
> + device_initialize(dev);
> + return subsys;
> +}
> +
> +static void put_tsm_subsys(struct tsm_subsys *subsys)
> +{
> + if (!IS_ERR_OR_NULL(subsys))
If you are calling this with it as an error or null then
that smells like a bad bug we shouldn't paper over.
The only case I can think of is the define free you have
below which correctly has the same check.
> + put_device(&subsys->dev);
> +}
> +
> +DEFINE_FREE(put_tsm_subsys, struct tsm_subsys *,
> + if (!IS_ERR_OR_NULL(_T)) put_tsm_subsys(_T))
> +struct tsm_subsys *tsm_register(struct device *parent,
> + const struct attribute_group **groups)
> +{
> + struct device *dev;
> + int rc;
> +
> + guard(rwsem_write)(&tsm_core_rwsem);
> + if (tsm_subsys) {
> + dev_warn(parent, "failed to register: %s already registered\n",
> + dev_name(tsm_subsys->dev.parent));
> + return ERR_PTR(-EBUSY);
> + }
> +
> + struct tsm_subsys *subsys __free(put_tsm_subsys) =
> + alloc_tsm_subsys(parent, groups);
> + if (IS_ERR(subsys))
> + return subsys;
> +
> + dev = &subsys->dev;
> + rc = dev_set_name(dev, "tsm0");
> + if (rc)
> + return ERR_PTR(rc);
> +
> + rc = device_add(dev);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + tsm_subsys = no_free_ptr(subsys);
> +
> + return tsm_subsys;
> +}
> +EXPORT_SYMBOL_GPL(tsm_register);
> +
> +void tsm_unregister(struct tsm_subsys *subsys)
> +{
> + guard(rwsem_write)(&tsm_core_rwsem);
> + if (!tsm_subsys || subsys != tsm_subsys) {
> + pr_warn("failed to unregister, not currently registered\n");
> + return;
> + }
> +
> + device_unregister(&subsys->dev);
> + tsm_subsys = NULL;
> +}
> +EXPORT_SYMBOL_GPL(tsm_unregister);
> +
> +static void tsm_release(struct device *dev)
> +{
> + struct tsm_subsys *subsys = container_of(dev, typeof(*subsys), dev);
> +
> + kfree(subsys);
> +}
> +
> +static int __init tsm_init(void)
> +{
> + tsm_class = class_create("tsm");
> + if (IS_ERR(tsm_class))
> + return PTR_ERR(tsm_class);
> +
> + tsm_class->dev_release = tsm_release;
> + return 0;
> +}
> +module_init(tsm_init)
> +
> +static void __exit tsm_exit(void)
> +{
> + class_destroy(tsm_class);
> +}
> +module_exit(tsm_exit)
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("TEE Security Manager core");
