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