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

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

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)