Message-ID: <6896a9ec60926_cff99100ee@dwillia2-xfh.jf.intel.com.notmuch>
Date: Fri, 8 Aug 2025 18:52:44 -0700
From: <dan.j.williams@...el.com>
To: Bjorn Helgaas <helgaas@...nel.org>, Dan Williams
	<dan.j.williams@...el.com>
CC: <linux-coco@...ts.linux.dev>, <linux-pci@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>, <aik@....com>,
	<lukas@...ner.de>, Samuel Ortiz <sameo@...osinc.com>, Yilun Xu
	<yilun.xu@...ux.intel.com>
Subject: Re: [PATCH v4 07/10] PCI/IDE: Add IDE establishment helpers

Bjorn Helgaas wrote:
> On Thu, Jul 17, 2025 at 11:33:55AM -0700, Dan Williams wrote:
> > There are two components to establishing an encrypted link, provisioning
> > the stream in Partner Port config-space, and programming the keys into
> > the link layer via IDE_KM (IDE Key Management). This new library,
> > drivers/pci/ide.c, enables the former. IDE_KM, via a TSM low-level
> > driver, is saved for later.
> > 
> > With the platform TSM implementations of SEV-TIO and TDX Connect in mind
> > this library abstracts small differences in those implementations. For
> > example, TDX Connect handles Root Port register setup while SEV-TIO
> > expects System Software to update the Root Port registers. This is the
> > rationale for fine-grained 'setup' + 'enable' verbs.
> > 
> > The other design detail for TSM-coordinated IDE establishment is that
> > the TSM may manage allocation of Stream IDs, this is why the Stream ID
> > value is passed in to pci_ide_stream_setup().
> > 
> > The flow is:
> > 
> > pci_ide_stream_alloc()
> >   Allocate a Selective IDE Stream Register Block in each Partner Port
> >   (Endpoint + Root Port), and reserve a host bridge / platform stream
> >   slot. Gather Partner Port specific stream settings like Requester ID.
> > pci_ide_stream_register()
> >   Publish the stream in sysfs after allocating a Stream ID. In the TSM
> >   case the TSM allocates the Stream ID for the Partner Port pair.
> > pci_ide_stream_setup()
> >   Program the stream settings to a Partner Port. Caller is responsible
> >   for optionally calling this for the Root Port as well if the TSM
> >   implementation requires it.
> > pci_ide_stream_enable()
> >   Try to run the stream after IDE_KM.
> 
> IIUC this patch doesn't actually add this as a "flow"; it adds these
> interfaces, and I guess it's up to callers to use them in a way that
> establishes this flow.

Right, common helpers for low-level TSM drivers to use with an example
of such a driver (without all the arch specific complexities) in
samples/devsec/.

> Maybe indent a couple spaces and add blank lines between them?

Ok.

> 
> > In support of system administrators auditing where platform, Root Port,
> > and Endpoint IDE stream resources are being spent, the allocated stream
> > is reflected as a symlink from the host bridge to the endpoint with the
> > name:
> > 
> >     stream%d.%d.%d
> > 
> > Where the tuple of integers reflects the allocated platform, Root Port,
> > and Endpoint stream index (Selective IDE Stream Register Block) values.
> 
> > +++ b/Documentation/ABI/testing/sysfs-devices-pci-host-bridge
> > +What:		pciDDDD:BB/streamH.R.E
> > +Contact:	linux-pci@...r.kernel.org
> > +Description:
> > +		(RO) When a platform has established a secure connection, PCIe
> > +		IDE, between two Partner Ports, this symlink appears. The
> > +		primary function is to account the stream slot / resources
> > +		consumed in each of the (H)ost bridge, (R)oot Port and
> > +		(E)ndpoint that will be freed when invoking the tsm/disconnect
> > +		flow. The link points to the endpoint PCI device in the
> > +		Selective IDE Stream. "R" and "E" represent the assigned
> > +		Selective IDE Stream Register Block in the Root Port and
> > +		Endpoint, and "H" represents a platform specific pool of stream
> > +		resources shared by the Root Ports in a host bridge. See
> > +		/sys/devices/pciDDDD:BB entry for details about the DDDD:BB
> > +		format.
> 
> s/tsm/TSM/
> s/endpoint/Endpoint/
> 
> For "(H)ost bridge", "(R)oot Port",
> 
>   - Could use "Host bridge (H)", etc, which makes spell checkers work
>     better (trivial, I know)
> 
>   - What's the format of these parts?  From the patch (and the commit
>     log), it looks like they're decimal stream index values?  (I don't
>     know enough to know what stream index values are, but presumably
>     users will.)

I clarified that a bit:

"A stream consumes a Stream ID slot in each of the Host bridge (H), Root
Port (R) and Endpoint (E)"

Presumably users that are debugging why they are unable to establish any
more streams can use this to discover, for example, "oh, I have resources available
in my Host Bridge and Endpoint, but the Root Port is out of Stream
slots".

> 
> > +++ b/drivers/pci/ide.c
> > +int pci_ide_domain(struct pci_dev *pdev)
> > +{
> > +	if (pdev->fm_enabled)
> > +		return pci_domain_nr(pdev->bus);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_ide_domain);
> 
> Not mentioned in commit log.  Maybe it doesn't need to be.  The only
> call I see is in this file, so it looks like it could even be static.

True, not sure why I thought this would be consumed by TSM drivers.
Fixed.

> 
> > +/**
> > + * pci_ide_stream_enable() - try to enable a Selective IDE Stream
> 
> Do or do not.  There is no try.

Ha! It does always enable, it just may immediately transition to the
error state if one of the partners is upset about something.

> > + * @pdev: PCIe device object for either a Root Port or Endpoint Partner Port
> > + * @ide: registered and setup IDE settings descriptor
> > + *
> > + * Activate the stream by writing to the Selective IDE Stream Control
> > + * Register, report whether the state successfully transitioned to
> > + * secure mode. Note that the state may go "insecure" at any point after
> > + * this check, but that is handled via asynchronous error reporting.
> 
> Maybe recast this as "Return:" instead of "report whether ..."  At
> least, I assume this reporting is done via the return value.

Yup, that is better.

> 
> > + */
> > +int pci_ide_stream_enable(struct pci_dev *pdev, struct pci_ide *ide)
> > +{
> > +	struct pci_ide_partner *settings = pci_ide_to_settings(pdev, ide);
> > +	int pos;
> > +	u32 val;
> > +
> > +	if (!settings)
> > +		return -ENXIO;
> > +
> > +	pos = sel_ide_offset(pdev, settings);
> > +
> > +	set_ide_sel_ctl(pdev, ide, pos, true);
> > +
> > +	pci_read_config_dword(pdev, pos + PCI_IDE_SEL_STS, &val);
> > +	if (FIELD_GET(PCI_IDE_SEL_STS_STATE_MASK, val) !=
> > +	    PCI_IDE_SEL_STS_STATE_SECURE) {
> > +		set_ide_sel_ctl(pdev, ide, pos, false);
> > +		return -ENXIO;
> > +	}
> > +
> > +	settings->enable = 1;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_ide_stream_enable);
> 
> > +++ b/include/linux/pci-ide.h
> > + * struct pci_ide_partner - Per port pair Selective IDE Stream settings
> > + * @rid_start: Partner Port Requester ID range start
> > + * @rid_start: Partner Port Requester ID range end
> > + * @stream_index: Selective IDE Stream Register Block selection
> > + * @setup: flag to track whether to run pci_ide_stream_teardown for this parnter slot
> 
> Wrap to fit in 80 columns like the rest of the file.  Add "()" after
> function name (below too).  Jonathan mentioned the "parnter".

Done.

Powered by blists - more mailing lists