Re: [PATCH v7 4/9] PCI/TSM: Establish Secure Sessions and Link Encryption

dan.j.williams
2025-10-30T19:56:58Z

Jonathan Cameron wrote:
[..]
> > Only "link", Secure Session and physical Link Encryption, operations are
> > defined at this stage with placeholders for the device security, Trusted
> > Computing Base entry / exit, operations.
>
> That list probably needs an 'and'

No, but went ahead and added parentheses to make it clearer.

> > The locking allows for multiple devices to be executing commands
> > simultaneously, one outstanding command per-device and an rwsem
> > synchronizes the implementation relative to TSM registration/unregistration
> > events.
> >
> > Thanks to Wu Hao for his work on an early draft of this support.
> >
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Cc: Alexey Kardashevskiy <aik@amd.com>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Co-developed-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Some comments on comments/documentation inline. With those addressed
> (which should be straight forward)
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>
> > diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> > new file mode 100644
> > index 000000000000..e3107ede2a0f
> > --- /dev/null
> > +++ b/include/linux/pci-tsm.h
> > @@ -0,0 +1,159 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __PCI_TSM_H
> > +#define __PCI_TSM_H
> > +#include <linux/mutex.h>
> > +#include <linux/pci.h>
> > +
> > +struct pci_tsm;
> See below for note on a duplicate of this.
>
> > +struct tsm_dev;
> > +
> > +/*
> > + * struct pci_tsm_ops - manage confidential links and security state
> > + * @link_ops: Coordinate PCIe SPDM and IDE establishment via a platform TSM.
> > + * Provide a secure session transport for TDISP state management
> > + * (typically bare metal physical function operations).
> > + * @sec_ops: Lock, unlock, and interrogate the security state of the
>
> devsec_ops?

Yes.

>
> > + * function via the platform TSM (typically virtual function
> > + * operations).
> > + * @owner: Back reference to the TSM device that owns this instance.
> Not seeing this below.

It is gone now. The reason kernel-doc is not complaining is because this
not actually a kernel-doc formatted comment. In any event, now cleaned
up.

> > + *
> > + * This operations are mutually exclusive either a tsm_dev instance
> > + * manages physical link properties or it manages function security
> > + * states like TDISP lock/unlock.
> > + */
> > +struct pci_tsm_ops {
> > + /*
> > + * struct pci_tsm_link_ops - Manage physical link and the TSM/DSM session
> > + * @probe: establish context with the TSM (allocate / wrap 'struct
> > + * pci_tsm') for follow-on link operations
> > + * @remove: destroy link operations context
> > + * @connect: establish / validate a secure connection (e.g. IDE)
> > + * with the device
> > + * @disconnect: teardown the secure link
> > + *
> > + * Context: @probe, @remove, @connect, and @disconnect run under
> > + * pci_tsm_rwsem held for write to sync with TSM unregistration and
> > + * mutual exclusion of @connect and @disconnect. @connect and
> > + * @disconnect additionally run under the DSM lock (struct
> > + * pci_tsm_pf0::lock) as well as @probe and @remove of the subfunctions.
> > + */
> > + struct_group_tagged(pci_tsm_link_ops, link_ops,
> > + struct pci_tsm *(*probe)(struct tsm_dev *tsm_dev,
> > + struct pci_dev *pdev);
> > + void (*remove)(struct pci_tsm *tsm);
> > + int (*connect)(struct pci_dev *pdev);
> > + void (*disconnect)(struct pci_dev *pdev);
> > + );
> > +
> > + /*
> > + * struct pci_tsm_devsec_ops - Manage the security state of the function
> > + * @lock: establish context with the TSM (allocate / wrap 'struct
> > + * pci_tsm') for follow-on security state transitions from the
> > + * LOCKED state
> > + * @unlock: destroy TSM context and return device to UNLOCKED state
> > + *
> > + * Context: @lock and @unlock run under pci_tsm_rwsem held for write to
> > + * sync with TSM unregistration and each other
> > + */
> > + struct_group_tagged(pci_tsm_devsec_ops, devsec_ops,
> > + struct pci_tsm *(*lock)(struct tsm_dev *tsm_dev,
> > + struct pci_dev *pdev);
> > + void (*unlock)(struct pci_tsm *tsm);
> > + );
> > +};
>
>
> > +#ifdef CONFIG_PCI_TSM
> > +struct tsm_dev;
>
> Seems to be declared already (not under an ifdef) above.

Yes, the Alexey request to drop owner and only have @tsm_dev in 'struct
pci_tsm' moved the need for this declaration earlier.

> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> > new file mode 100644
> > index 000000000000..094650454aa7
> > --- /dev/null
> > +++ b/drivers/pci/tsm.c
>
> > +static int pci_tsm_connect(struct pci_dev *pdev, struct tsm_dev *tsm_dev)
> > +{
> > + int rc;
> > + struct pci_tsm_pf0 *tsm_pf0;
> > + const struct pci_tsm_ops *ops = tsm_dev->pci_ops;
> > + struct pci_tsm *pci_tsm __free(tsm_remove) = ops->probe(tsm_dev, pdev);
> > +
> > + /* connect() mutually exclusive with subfunction pci_tsm_init() */
>
> Extra space after ()

Got it.

[..]
> > +/*
> > + * Find the PCI Device instance that serves as the Device Security Manager (DSM)
> > + * for @pdev. Note that no additional reference is held for the resulting device
> > + * because @pdev always has a longer registered lifetime than its DSM by virtue
> > + * of being a child of, or identical to, its DSM.
>
> This comment has me confused. I would normally expect parent to have the guaranteed
> longer life span than the child. This seems to say the opposite.
> Code itself is fine.

Right, comment flipped the polarity of the lifetime. It was meant to say
that registered children always extend the life of their DSM ancestor.

4: 722f1b0155cf ! 4: 77789285d040 PCI/TSM: Establish Secure Sessions and Link Encryption
@@ Commit message
"Security" operations coordinate the security state of the assigned
virtual device (TDI). These are the guest side operations in TDISP.

- Only "link", Secure Session and physical Link Encryption, operations are
- defined at this stage with placeholders for the device security, Trusted
- Computing Base entry / exit, operations.
+ Only "link" (Secure Session and physical Link Encryption) operations are
+ defined at this stage. There are placeholders for the device security
+ (Trusted Computing Base entry / exit) operations.

The locking allows for multiple devices to be executing commands
simultaneously, one outstanding command per-device and an rwsem
@@ Commit message
Cc: Samuel Ortiz <sameo@rivosinc.com>
Cc: Alexey Kardashevskiy <aik@amd.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
+ Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Co-developed-by: Xu Yilun <yilun.xu@linux.intel.com>
Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
@@ include/linux/pci-tsm.h (new)
+ * @link_ops: Coordinate PCIe SPDM and IDE establishment via a platform TSM.
+ * Provide a secure session transport for TDISP state management
+ * (typically bare metal physical function operations).
-+ * @sec_ops: Lock, unlock, and interrogate the security state of the
-+ * function via the platform TSM (typically virtual function
-+ * operations).
-+ * @owner: Back reference to the TSM device that owns this instance.
++ * @devsec_ops: Lock, unlock, and interrogate the security state of the
++ * function via the platform TSM (typically virtual function
++ * operations).
+ *
+ * This operations are mutually exclusive either a tsm_dev instance
+ * manages physical link properties or it manages function security
@@ include/linux/pci-tsm.h (new)
+}
+
+#ifdef CONFIG_PCI_TSM
-+struct tsm_dev;
+int pci_tsm_register(struct tsm_dev *tsm_dev);
+void pci_tsm_unregister(struct tsm_dev *tsm_dev);
+int pci_tsm_link_constructor(struct pci_dev *pdev, struct pci_tsm *tsm,
@@ drivers/pci/tsm.c (new)
+ const struct pci_tsm_ops *ops = tsm_dev->pci_ops;
+ struct pci_tsm *pci_tsm __free(tsm_remove) = ops->probe(tsm_dev, pdev);
+
-+ /* connect() mutually exclusive with subfunction pci_tsm_init() */
++ /* connect() mutually exclusive with subfunction pci_tsm_init() */
+ lockdep_assert_held_write(&pci_tsm_rwsem);
+
+ if (!pci_tsm)
@@ drivers/pci/tsm.c (new)
+/*
+ * Find the PCI Device instance that serves as the Device Security Manager (DSM)
+ * for @pdev. Note that no additional reference is held for the resulting device
-+ * because @pdev always has a longer registered lifetime than its DSM by virtue
-+ * of being a child of, or identical to, its DSM.
++ * because that resulting object always has a registered lifetime
++ * greater-than-or-equal to that of the @pdev argument. This is by virtue of
++ * @pdev being a descendant of, or identical to, the returned DSM device.
+ */
+static struct pci_dev *find_dsm_dev(struct pci_dev *pdev)
+{
@@ drivers/virt/coco/tsm-core.c: static struct tsm_dev *alloc_tsm_dev(struct device
+ return tsm_dev;
+}
+
- static void put_tsm_dev(struct tsm_dev *tsm_dev)
- {
- if (!IS_ERR_OR_NULL(tsm_dev))
-@@ drivers/virt/coco/tsm-core.c: static void put_tsm_dev(struct tsm_dev *tsm_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)
+struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops)