Jonathan Cameron wrote:
> 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.
Yeah, subsys is awkward because even though this device is a singleton,
it is not a 'bus'. I will switch to 'struct tsm_core_dev' unless someone
comes up with a better name.
>
>
> > +
> > +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.
So, I do not think it matters in the end because there is no expectation
that anything but __free() invokes this call, and @subsys will be NULL
after no_free_ptr(). In general I expect that __free() callbacks should
mirror the "skip free()" condition in their DEFINE_FREE()
implementation. In this case though, the usage is so small and obvious
that we can pre-elide that code and just drop the duplicate check.
Going forward though I think this is something that deserves
clarification in cleanup.h documentation and I would argue that
DEFINE_FREE() and the __free handler duplicate the "skip free()"
condition check.
...or otherwise clarify the complication introduced by mixing ERR_PTR()
with no_free_ptr().
