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

[Dan Williams <dan.j.williams@intel.com>]

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.