[RFC,3/5] coco/tsm: Introduce a shared class device for TSMs

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>
---
 Documentation/ABI/testing/sysfs-class-tsm |   12 +++
 drivers/virt/coco/tsm/Kconfig             |    7 ++
 drivers/virt/coco/tsm/Makefile            |    3 +
 drivers/virt/coco/tsm/class.c             |  100 +++++++++++++++++++++++++++++
 include/linux/tsm.h                       |    8 ++
 5 files changed, 130 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-tsm
 create mode 100644 drivers/virt/coco/tsm/class.c

On 30/1/24 20:24, Dan Williams 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>
> ---
>   Documentation/ABI/testing/sysfs-class-tsm |   12 +++
>   drivers/virt/coco/tsm/Kconfig             |    7 ++
>   drivers/virt/coco/tsm/Makefile            |    3 +
>   drivers/virt/coco/tsm/class.c             |  100 +++++++++++++++++++++++++++++
>   include/linux/tsm.h                       |    8 ++
>   5 files changed, 130 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-class-tsm
>   create mode 100644 drivers/virt/coco/tsm/class.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm
> new file mode 100644
> index 000000000000..304b50b53e65
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-tsm
> @@ -0,0 +1,12 @@
> +What:		/sys/class/tsm/tsm0/host
> +Date:		January 2024
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) For hardware TSMs represented by a device in /sys/devices,
> +		@host is a link to that device.
> +		Links to hardware TSM sysfs ABIs:
> +		- Documentation/ABI/testing/sysfs-driver-ccp
> +
> +		For software TSMs instantiated by a software module, @host is a
> +		directory with attributes for that TSM, and those attributes are
> +		documented below.
> diff --git a/drivers/virt/coco/tsm/Kconfig b/drivers/virt/coco/tsm/Kconfig
> index 69f04461c83e..595d86917462 100644
> --- a/drivers/virt/coco/tsm/Kconfig
> +++ b/drivers/virt/coco/tsm/Kconfig
> @@ -5,3 +5,10 @@
>   config TSM_REPORTS
>   	select CONFIGFS_FS
>   	tristate
> +
> +config ARCH_HAS_TSM
> +	bool
> +
> +config TSM
> +	depends on ARCH_HAS_TSM && SYSFS
> +	tristate
> diff --git a/drivers/virt/coco/tsm/Makefile b/drivers/virt/coco/tsm/Makefile
> index b48504a3ccfd..f7561169faed 100644
> --- a/drivers/virt/coco/tsm/Makefile
> +++ b/drivers/virt/coco/tsm/Makefile
> @@ -4,3 +4,6 @@
>   
>   obj-$(CONFIG_TSM_REPORTS) += tsm_reports.o
>   tsm_reports-y := reports.o
> +
> +obj-$(CONFIG_TSM) += tsm.o
> +tsm-y := class.o
> 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;
> +	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;


no kfree(subsys) ? Thanks,

> +
> +	if (info->host) {
> +		rc = sysfs_create_link(&dev->kobj, &info->host->kobj, "host");
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* don't auto-free @dev */
> +	dev = NULL;
> +	tsm_subsys = subsys;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_register);
> +
> +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);
> +
> +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("Trusted Security Module core device model");
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index 28753608fcf5..8cb8a661ba41 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -5,6 +5,12 @@
>   #include <linux/sizes.h>
>   #include <linux/types.h>
>   
> +struct tsm_info {
> +	const char *name;
> +	struct device *host;
> +	const struct attribute_group **groups;
> +};
> +
>   #define TSM_REPORT_INBLOB_MAX 64
>   #define TSM_REPORT_OUTBLOB_MAX SZ_32K
>   
> @@ -66,4 +72,6 @@ extern const struct config_item_type tsm_report_extra_type;
>   int tsm_report_register(const struct tsm_report_ops *ops, void *priv,
>   			const struct config_item_type *type);
>   int tsm_report_unregister(const struct tsm_report_ops *ops);
> +int tsm_register(const struct tsm_info *info);
> +void tsm_unregister(const struct tsm_info *info);
>   #endif /* __TSM_H */
>

Alexey Kardashevskiy wrote:
> On 30/1/24 20:24, Dan Williams 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>
> > ---
> >   Documentation/ABI/testing/sysfs-class-tsm |   12 +++
> >   drivers/virt/coco/tsm/Kconfig             |    7 ++
> >   drivers/virt/coco/tsm/Makefile            |    3 +
> >   drivers/virt/coco/tsm/class.c             |  100 +++++++++++++++++++++++++++++
> >   include/linux/tsm.h                       |    8 ++
> >   5 files changed, 130 insertions(+)
> >   create mode 100644 Documentation/ABI/testing/sysfs-class-tsm
> >   create mode 100644 drivers/virt/coco/tsm/class.c
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm
> > new file mode 100644
> > index 000000000000..304b50b53e65
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-tsm
> > @@ -0,0 +1,12 @@
> > +What:		/sys/class/tsm/tsm0/host
> > +Date:		January 2024
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(RO) For hardware TSMs represented by a device in /sys/devices,
> > +		@host is a link to that device.
> > +		Links to hardware TSM sysfs ABIs:
> > +		- Documentation/ABI/testing/sysfs-driver-ccp
> > +
> > +		For software TSMs instantiated by a software module, @host is a
> > +		directory with attributes for that TSM, and those attributes are
> > +		documented below.
> > diff --git a/drivers/virt/coco/tsm/Kconfig b/drivers/virt/coco/tsm/Kconfig
> > index 69f04461c83e..595d86917462 100644
> > --- a/drivers/virt/coco/tsm/Kconfig
> > +++ b/drivers/virt/coco/tsm/Kconfig
> > @@ -5,3 +5,10 @@
> >   config TSM_REPORTS
> >   	select CONFIGFS_FS
> >   	tristate
> > +
> > +config ARCH_HAS_TSM
> > +	bool
> > +
> > +config TSM
> > +	depends on ARCH_HAS_TSM && SYSFS
> > +	tristate
> > diff --git a/drivers/virt/coco/tsm/Makefile b/drivers/virt/coco/tsm/Makefile
> > index b48504a3ccfd..f7561169faed 100644
> > --- a/drivers/virt/coco/tsm/Makefile
> > +++ b/drivers/virt/coco/tsm/Makefile
> > @@ -4,3 +4,6 @@
> >   
> >   obj-$(CONFIG_TSM_REPORTS) += tsm_reports.o
> >   tsm_reports-y := reports.o
> > +
> > +obj-$(CONFIG_TSM) += tsm.o
> > +tsm-y := class.o
> > 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;
> > +	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;
> 
> 
> no kfree(subsys) ? Thanks,

No, that's handled by __free(put_device), but I want to make this
pattern easier to read with something like.

	struct tsm_subsys *subsys __free(put_tsm_subsys) = tsm_subsys_alloc();
	if (!subsys)
		return -ENOMEM;
	rc = device_register(&subsys->dev);
	cond_no_free_ptr(rc == 0, return rc, subsys)

...stay tuned for whether Linus and/or Peter like the cond_no_free_ptr()
suggestion, will address that on a separate thread.

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);

Jonathan Cameron wrote:
> 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 :)

Heh, useful review is always welcome, and yeah, I think every instance
of the "... __free(x) = NULL" pattern deserves to be scrutinized. Likely
what I will do is add a helper function which was the main takeaway
I got from the Linus review of cond_no_free_ptr(). Something like:

diff --git a/drivers/virt/coco/tsm/class.c b/drivers/virt/coco/tsm/class.c
index a569fa6b09eb..a60087905c72 100644
--- a/drivers/virt/coco/tsm/class.c
+++ b/drivers/virt/coco/tsm/class.c
@@ -16,10 +16,29 @@ struct tsm_subsys {
 	const struct tsm_info *info;
 } *tsm_subsys;
 
+static struct tsm_subsys *tsm_subsys_alloc(const struct tsm_info *info)
+{
+	struct device *dev;
+
+	struct tsm_subsys *subsys __free(kfree) =
+		kzalloc(sizeof(*subsys), GFP_KERNEL);
+	if (!subsys)
+		return NULL;
+
+	subsys->info = info;
+	dev = &subsys->dev;
+	dev->class = tsm_class;
+	dev->groups = info->groups;
+	if (dev_set_name(dev, "tsm0"))
+		return NULL;
+	device_initialize(dev);
+
+	return no_free_ptr(subsys);
+}
+DEFINE_FREE(tsm_subsys_put, struct tsm_subsys *, if (_T) put_device(&_T->dev))
+
 int tsm_register(const struct tsm_info *info)
 {
-	struct device *dev __free(put_device) = NULL;
-	struct tsm_subsys *subsys;
 	int rc;
 
 	guard(rwsem_write)(&tsm_core_rwsem);
@@ -29,16 +48,11 @@ int tsm_register(const struct tsm_info *info)
 		return -EBUSY;
 	}
 
-	subsys = kzalloc(sizeof(*subsys), GFP_KERNEL);
+	struct tsm_subsys *subsys __free(tsm_subsys_put) = tsm_subsys_alloc(info);
 	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);
+	rc = device_add(&subsys->dev);
 	if (rc)
 		return rc;
 
@@ -48,9 +62,7 @@ int tsm_register(const struct tsm_info *info)
 			return rc;
 	}
 
-	/* don't auto-free @dev */
-	dev = NULL;
-	tsm_subsys = subsys;
+	tsm_subsys = no_free_ptr(subsys);
 
 	return 0;
 }

> 
> 
> > +	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.

Yeah at the time I was drafting this I was working around the lack of a
ready device parent for Intel TDX which can not lean on a PCI device TSM
like other archs. However, yes, that would be nice and it implies that
Intel TDX just needs to build a virtual device abstraction to hang this
interface.

> 
> > +		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.

Sure.