Re: [PATCH v2 05/11] PCI/TSM: Authenticate devices via platform TSM

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Dan Williams <dan.j.williams@intel.com> writes:

....

> diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> new file mode 100644
> index 000000000000..17657b7ef66c
> --- /dev/null
> +++ b/include/linux/pci-tsm.h
> @@ -0,0 +1,135 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __PCI_TSM_H
> +#define __PCI_TSM_H
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +
> +struct pci_dev;
> +
> +enum pci_tsm_state {
> +	PCI_TSM_ERR = -1,
> +	PCI_TSM_INIT,
> +	PCI_TSM_CONNECT,
> +};
> +
> +enum pci_tsm_type {
> +	PCI_TSM_INVALID,
> +	PCI_TSM_PF0,
> +	PCI_TSM_VIRTFN,
> +	PCI_TSM_MFD,
> +};
> +
> +/**
> + * struct pci_tsm - Core TSM context for a given PCIe endpoint
> + * @pdev: indicates the type of pci_tsm object
> + * @type: debug validation of the pci_tsm object type inferred by @pdev
> + *
> + * This structure is wrapped by a low level TSM driver and returned by
> + * tsm_ops.probe(), it is freed by tsm_ops.remove(). Depending on
> + * whether @pdev is physical function 0, another physical function, or a
> + * virtual function determines the pci_tsm object type. E.g. see 'struct
> + * pci_tsm_pf0'.
> + */
> +struct pci_tsm {
> +	struct pci_dev *pdev;
> +#ifdef CONFIG_PCI_TSM_DEBUG
> +	enum pci_tsm_type type;
> +#endif
> +};
> +
> +#ifdef CONFIG_PCI_TSM_DEBUG
> +static inline void pci_tsm_set_type(struct pci_tsm *pci_tsm, enum pci_tsm_type type)
> +{
> +	pci_tsm->type = type;
> +}
> +static inline bool pci_tsm_check_type(struct pci_tsm *pci_tsm, enum pci_tsm_type type)
> +{
> +	return pci_tsm->type == type;
> +}
> +#else
> +static inline void pci_tsm_set_type(struct pci_tsm *pci_tsm, enum pci_tsm_type type)
> +{
> +}
> +
> +static inline bool pci_tsm_check_type(struct pci_tsm *pci_tsm, enum pci_tsm_type type)
> +{
> +	return true;
> +}
> +#endif
> +
> +/**
> + * struct pci_tsm_pf0 - Physical Function 0 TDISP context
> + * @state: reflect device initialized, connected, or bound
> + * @lock: protect @state vs pci_tsm_ops invocation
> + * @doe_mb: PCIe Data Object Exchange mailbox
> + */
> +struct pci_tsm_pf0 {
> +	struct pci_tsm tsm;
> +	enum pci_tsm_state state;
> +	struct mutex lock;
> +	struct pci_doe_mb *doe_mb;
> +};
>

While working with a multifunction device, I found that adding lock and
state to struct pci_tsm simplified the code considerably.

In multifunction setups, it’s possible that multiple functions may
expose DOE capabilities. In this case, when sending SPDM messages for a
TDI, should we always use the DOE mailbox of function 0, or should we
address the mailbox of the specific function involved?

If the latter is preferred, would it make sense to rename the current
structure—currently representing the base pci_tsm plus the DOE
mailbox—to something more generic? because it is not more tied to
physical function 0

> +
> +static inline bool is_pci_tsm_pf0(struct pci_dev *pdev)
> +{
> +	if (!pci_is_pcie(pdev))
> +		return false;
> +
> +	if (pdev->is_virtfn)
> +		return false;
> +
> +	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT)
> +		return false;
> +
> +	return PCI_FUNC(pdev->devfn) == 0;
> +}
> +
> +/**
> + * struct pci_tsm_ops - Low-level TSM-exported interface to the PCI core
> + * @probe: probe/accept device for tsm operation, setup DSM context
> + * @remove: destroy DSM context
> + * @connect: establish / validate a secure connection (e.g. IDE) with the device
> + * @disconnect: teardown the secure connection
> + *
> + * @probe and @remove run in pci_tsm_rwsem held for write context. All
> + * other ops run under the @pdev->tsm->lock mutex and pci_tsm_rwsem held
> + * for read.
> + */
> +struct pci_tsm_ops {
> +	struct pci_tsm *(*probe)(struct pci_dev *pdev);
> +	void (*remove)(struct pci_tsm *tsm);
> +	int (*connect)(struct pci_dev *pdev);
> +	void (*disconnect)(struct pci_dev *pdev);
> +};
> +
> +enum pci_doe_proto {
> +	PCI_DOE_PROTO_CMA = 1,
> +	PCI_DOE_PROTO_SSESSION = 2,
> +};
> +
> +#ifdef CONFIG_PCI_TSM
> +int pci_tsm_core_register(const struct pci_tsm_ops *ops,
> +			  const struct attribute_group *grp);
> +void pci_tsm_core_unregister(const struct pci_tsm_ops *ops);
> +int pci_tsm_doe_transfer(struct pci_dev *pdev, enum pci_doe_proto type,
> +			 const void *req, size_t req_sz, void *resp,
> +			 size_t resp_sz);
> +int pci_tsm_pf0_initialize(struct pci_dev *pdev, struct pci_tsm_pf0 *tsm);
> +#else
> +static inline int pci_tsm_core_register(const struct pci_tsm_ops *ops,
> +					const struct attribute_group *grp)
> +{
> +	return 0;
> +}
> +static inline void pci_tsm_core_unregister(const struct pci_tsm_ops *ops)
> +{
> +}
> +static inline int pci_tsm_doe_transfer(struct pci_dev *pdev,
> +				       enum pci_doe_proto type, const void *req,
> +				       size_t req_sz, void *resp,
> +				       size_t resp_sz)
> +{
> +	return -ENOENT;
> +}
> +#endif
> +#endif /*__PCI_TSM_H */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 628f9f5fdac9..57cfa0e3f2bd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -537,6 +537,9 @@ struct pci_dev {
>  	u8		nr_link_ide;	/* Link Stream count (Selective Stream offset) */
>  	unsigned int	ide_cfg:1;	/* Config cycles over IDE */
>  	unsigned int	ide_tee_limit:1; /* Disallow T=0 traffic over IDE */
> +#endif
> +#ifdef CONFIG_PCI_TSM
> +	struct pci_tsm *tsm;		/* TSM operation state */
>  #endif
>  	u16		acs_cap;	/* ACS Capability offset */
>  	u8		supported_speeds; /* Supported Link Speeds Vector */
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index 9253b79b8582..59d3848404e1 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -111,7 +111,9 @@ struct tsm_report_ops {
>  int tsm_report_register(const struct tsm_report_ops *ops, void *priv);
>  int tsm_report_unregister(const struct tsm_report_ops *ops);
>  struct tsm_core_dev;
> +struct pci_tsm_ops;
>  struct tsm_core_dev *tsm_register(struct device *parent,
> -				  const struct attribute_group **groups);
> +				  const struct attribute_group **groups,
> +				  const struct pci_tsm_ops *ops);
>  void tsm_unregister(struct tsm_core_dev *tsm_core);
>  #endif /* __TSM_H */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 000258cd93c8..713588a29813 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -499,6 +499,7 @@
>  #define  PCI_EXP_DEVCAP_PWR_VAL	0x03fc0000 /* Slot Power Limit Value */
>  #define  PCI_EXP_DEVCAP_PWR_SCL	0x0c000000 /* Slot Power Limit Scale */
>  #define  PCI_EXP_DEVCAP_FLR     0x10000000 /* Function Level Reset */
> +#define  PCI_EXP_DEVCAP_TEE     0x40000000 /* TEE I/O (TDISP) Support */
>  #define PCI_EXP_DEVCTL		0x08	/* Device Control */
>  #define  PCI_EXP_DEVCTL_CERE	0x0001	/* Correctable Error Reporting En. */
>  #define  PCI_EXP_DEVCTL_NFERE	0x0002	/* Non-Fatal Error Reporting Enable */