Re: [PATCH v4 04/10] PCI/TSM: Authenticate devices via platform TSM

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

Bjorn Helgaas wrote:
> On Thu, Jul 17, 2025 at 11:33:52AM -0700, Dan Williams wrote:
> > The PCIe 6.1 specification, section 11, introduces the Trusted Execution
> > Environment (TEE) Device Interface Security Protocol (TDISP).  This
> > protocol definition builds upon Component Measurement and Authentication
> > (CMA), and link Integrity and Data Encryption (IDE). It adds support for
> > assigning devices (PCI physical or virtual function) to a confidential
> > VM such that the assigned device is enabled to access guest private
> > memory protected by technologies like Intel TDX, AMD SEV-SNP, RISCV
> > COVE, or ARM CCA.
> 
> Previous patches reference PCIe r6.2.  Personally I would change them
> all the citations to r7.0, since that's out now and (I assume)
> includes everything.  I guess you said "introduced in r6.1," which is
> not the same as "introduced in r7.0," but I'm not sure how relevant it
> is to know that very first revision.

Ack, looks like the section numbers have not changed which makes it easier.

> > The operations that can be executed against a PCI device are split into
> > 2 mutually exclusive operation sets, "Link" and "Security" (struct
> 
> s/2/two/  Old skool, but you obviously pay attention to details like
> that :)

I only recently gave up the fight against 2^H^H two spaces after a
period, fixed.

> > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > +What:		/sys/bus/pci/devices/.../tsm/
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		This directory only appears if a physical device function
> > +		supports authentication (PCIe CMA-SPDM), interface security
> > +		(PCIe TDISP), and is accepted for secure operation by the
> > +		platform TSM driver. This attribute directory appears
> > +		dynamically after the platform TSM driver loads. So, only after
> > +		the /sys/class/tsm/tsm0 device arrives can tools assume that
> > +		devices without a tsm/ attribute directory will never have one,
> > +		before that, the security capabilities of the device relative to
> > +		the platform TSM are unknown. See
> > +		Documentation/ABI/testing/sysfs-class-tsm.
> 
> s/never have one,/never have one;/

yes.

> 
> > +++ b/drivers/pci/tsm.c
> > +#define dev_fmt(fmt) "TSM: " fmt
> 
> Include "PCI" for context?

Sure.

> 
> > + * Provide a read/write lock against the init / exit of pdev tsm
> > + * capabilities and arrival/departure of a tsm instance
> 
> s/tsm/TSM/ in comments.

Got it.

> > +static void pci_tsm_walk_fns(struct pci_dev *pdev,
> > +			     int (*cb)(struct pci_dev *pdev, void *data),
> > +			     void *data)
> > +{
> > +	struct pci_dev *fn;
> > +	int i;
> > +
> > +	/* walk virtual functions */
> > +        for (i = 0; i < pci_num_vf(pdev); i++) {
> > +		fn = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
> > +						 pci_iov_virtfn_bus(pdev, i),
> > +						 pci_iov_virtfn_devfn(pdev, i));
> > +		if (call_cb_put(fn, data, cb))
> > +			return;
> > +        }
> > +
> > +	/* walk subordinate physical functions */
> > +	for (i = 1; i < 8; i++) {
> > +		fn = pci_get_slot(pdev->bus,
> > +				  PCI_DEVFN(PCI_SLOT(pdev->devfn), i));
> > +		if (call_cb_put(fn, data, cb))
> > +			return;
> > +	}
> > +
> > +	/* walk downstream devices */
> > +        if (pci_pcie_type(pdev) != PCI_EXP_TYPE_UPSTREAM)
> > +                return;
> > +
> > +        if (!is_dsm(pdev))
> > +                return;
> > +
> > +        pci_walk_bus(pdev->subordinate, cb, data);
> 
> What's the difference between all this and just pci_walk_bus() of
> pdev->subordinate?  Are VFs not included in that walk?  Maybe a
> hint here would be useful.

Right, ->subordinate is only managed for actual bridge devices. PFs do
use one or more 'struct pci_bus *' instances for their VFs, but do not
set ->subordinate I assume becuase of that "or more" case. See the NULL
@bridge parameter to pci_add_new_bus() in virtfn_add_bus(). With that
there is no clean way I see to walk all the virtfn buses of a PF, so
fall back to a pci_get_domain_bus_and_slot() walk.

I will add a note to this effect as I had to do some digging here to be
sure.

> > +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_pci_ops(tsm_dev);
> > +	struct pci_tsm *pci_tsm __free(tsm_remove) = ops->probe(pdev);
> > +
> > +	if (!pci_tsm)
> > +		return -ENXIO;
> > +
> > +	pdev->tsm = pci_tsm;
> > +	tsm_pf0 = to_pci_tsm_pf0(pdev->tsm);
> > +
> > +	ACQUIRE(mutex_intr, lock)(&tsm_pf0->lock);
> > +	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
> > +		return rc;
> > +
> > +	rc = ops->connect(pdev);
> > +	if (rc)
> > +		return rc;
> > +
> > +	pdev->tsm = no_free_ptr(pci_tsm);
> > +
> > +	/*
> > +	 * Now that the DSM is established, probe() all the potential
> > +	 * dependent functions. Failure to probe a function is not fatal
> > +	 * to connect(), it just disables subsequent security operations
> > +	 * for that function.
> > +	 */
> > +	pci_tsm_probe_fns(pdev);
> 
> Makes me wonder what happens if a device is hot-added in the
> hierarchy.  I guess nothing.  Is that what we want?  What would be the
> flow if we *did* want to do something?  I guess disconnect and connect
> again?

If a subfunction is found after the 'connect' event, like late enable of
SR-IOV capability, then the resulting pci_device_add() for that should
lookup and perform the ->probe() at that time.

> > + * Find the PCI Device instance that serves as the Device Security
> > + * Manger (DSM) for @pdev. Note that no additional reference is held for
> 
> s/Manger/Manager/

...could have swore I ran checkpatch, but indeed it flags this.

Fixed, along with the others.
 
> > +struct pci_tsm_ops {
> > +	/*
> > +	 * struct pci_tsm_link_ops - Manage physical link and the TSM/DSM session
> > +	 * @probe: probe device for tsm link operation readiness, setup
> > +	 *	   DSM context
> 
> s/tsm link/TSM link/
> 
> > +	 * struct pci_tsm_security_ops - Manage the security state of the function
> > +	 * @sec_probe: probe device for tsm security operation
> > +	 *	       readiness, setup security context
> 
> s/for tsm/for TSM/
> 
> > + * struct pci_tsm - Core TSM context for a given PCIe endpoint
> > + * @pdev: Back ref to device function, distinguishes type of pci_tsm context
> > + * @dsm: PCI Device Security Manager for link operations on @pdev.
> 
> Extra period at end, unlike others.

Fixed the above.

> 
> > + * @ops: Link Confidentiality or Device Function Security operations
> 
> > +static inline bool is_pci_tsm_pf0(struct pci_dev *pdev)
> > +{
> > +	if (!pci_is_pcie(pdev))
> > +		return false;
> > +
> > +	if (pdev->is_virtfn)
> > +		return false;
> > +
> > +	/*
> > +	 * Allow for a Device Security Manager (DSM) associated with function0
> > +	 * of an Endpoint to coordinate TDISP requests for other functions
> > +	 * (physical or virtual) of the device, or allow for an Upstream Port
> > +	 * DSM to accept TDISP requests for switch Downstream Endpoints.
> 
> What exactly is a "switch Downstream Endpoint"?  Do you mean a "Switch
> Downstream Port"?  Or an Endpoint that is downstream of a Switch?

Endpoint that is downstream of a Switch. I will clarify the comment.