Re: [PATCH v7 1/9] coco/tsm: Introduce a core device for TEE Security Managers

dan.j.williams
2025-10-29T23:47:32Z

Jonathan Cameron wrote:
> On Thu, 23 Oct 2025 19:04:10 -0700
> 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. 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 core device are parented by a device representing the
> > platform security function like CONFIG_CRYPTO_DEV_CCP or
> > CONFIG_INTEL_TDX_HOST.
> >
> > This 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: Alexey Kardashevskiy <aik@amd.com>
> > Cc: Xu Yilun <yilun.xu@linux.intel.com>
> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Co-developed-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> > Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Dan,
>
> My usual problem of having forgotten all the details since I last
> looked applies, so I'll take another look at the lot.
>
> One trivial comment below.

Too late, you already added a review tag. </joking>

> > diff --git a/drivers/virt/coco/tsm-core.c b/drivers/virt/coco/tsm-core.c
> > new file mode 100644
> > index 000000000000..a64b776642cf
> > --- /dev/null
> > +++ b/drivers/virt/coco/tsm-core.c
> > @@ -0,0 +1,109 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> Maybe worth updating as in general this has evolved a bit this year
> I think.

Sure.

> > +
> > +static void put_tsm_dev(struct tsm_dev *tsm_dev)
> > +{
> > + if (!IS_ERR_OR_NULL(tsm_dev))
> > + put_device(&tsm_dev->dev);
> > +}
> > +
> > +DEFINE_FREE(put_tsm_dev, struct tsm_dev *,
> > + if (!IS_ERR_OR_NULL(_T)) put_tsm_dev(_T))
>
> I'm entirely on board with the normal argument behind the !IS_ERR_OR_NULL()
> check and the fact it lets the compiler remove an indirect call in some
> cases. However, here you have the protection here and in put_tsm_dev()
> that is only called via this path. That seems excessive.

I think if there were open coded callers of put_tsm_dev() I would keep
the excessive form, but since this only for __free() no need for the
explicit helper.

range-diff result:

1: c2ad31ce3803 ! 1: 448addc31b86 coco/tsm: Introduce a core device for TEE Security Managers
@@ include/linux/tsm.h: struct tsm_report_ops {
## drivers/virt/coco/tsm-core.c (new) ##
@@
+// SPDX-License-Identifier: GPL-2.0-only
-+/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
++/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
@@ drivers/virt/coco/tsm-core.c (new)
+ return no_free_ptr(tsm_dev);
+}
+
-+static void put_tsm_dev(struct tsm_dev *tsm_dev)
-+{
-+ if (!IS_ERR_OR_NULL(tsm_dev))
-+ put_device(&tsm_dev->dev);
-+}
-+
+DEFINE_FREE(put_tsm_dev, struct tsm_dev *,
-+ if (!IS_ERR_OR_NULL(_T)) put_tsm_dev(_T))
++ if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev))
+
+struct tsm_dev *tsm_register(struct device *parent)
+{