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

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

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.