Re: [PATCH 08/11] PCI/IDE: Add IDE establishment helpers

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

Bjorn Helgaas wrote:
> On Thu, Dec 05, 2024 at 02:24:02PM -0800, Dan Williams wrote:
> > There are two components to establishing an encrypted link, provisioning
> > the stream in config-space, and programming the keys into the link layer
> > via the IDE_KM (key management) protocol. These helpers enable the
> > former, and are in support of TSM coordinated IDE_KM. When / if native
> > IDE establishment arrives it will share this same config-space
> > provisioning flow, but for now IDE_KM, in any form, is saved for a
> > follow-on change.
> > 
> > With the TSM implementations of SEV-TIO and TDX Connect in mind this
> > abstracts small differences in those implementations. For example, TDX
> > Connect handles Root Port registers updates while SEV-TIO expects System
> > Software to update the Root Port registers. This is the rationale for
> > the PCI_IDE_SETUP_ROOT_PORT flag.
> > 
> > The other design detail for TSM-coordinated IDE establishment is that
> > the TSM manages allocation of stream-ids, this is why the stream_id is
> > passed in to pci_ide_stream_setup().
> 
> s/stream-ids/Stream IDs/ to match spec usage (also several other
> places)

Sure. Fixed up this one, the print statements that were using "stream
id", the code comments, and all the other patches in this set using
lowercase "stream id".

> > The flow is:
> > 
> > pci_ide_stream_probe()
> >   Gather stream settings (devid and address filters)
> > pci_ide_stream_setup()
> >   Program the stream settings into the endpoint, and optionally Root Port)
> > pci_ide_enable_stream()
> >   Run the stream after IDE_KM
> 
> Not sure what "devid" is.  Requester ID?  I suppose it's from
> PCI_DEVID(), which does turn out to be the PCIe Requester ID.  I don't
> think the spec uses a "devid" term, so I'd prefer the term used in the
> spec.

Indeed my thought process, was "well, Linux has long called Requester ID
'DEVID' so just pick 'DEVID' to keep with that momentum". I am fine to
use "Requester ID" in all comments etc.

I'll also switch any usage of "devid" to "rid" for variable names and
just note somewhere that PCI_DEVID() is the "rid" as far as IDE and TSM
code paths are concerned.

> > In support of system administrators auditing where platform IDE stream
> > resources are being spent, the allocated stream is reflected as a
> > symlink from the host-bridge to the endpoint.
> 
> s/host-bridge/host bridge/ to match typical usage (also elsewhere)

Fixed here, found an additional one code comments, and the documentation
file

> 
> > +++ b/Documentation/ABI/testing/sysfs-devices-pci-host-bridge
> > @@ -0,0 +1,28 @@
> > +What:		/sys/devices/pciDDDDD:BB
> > +		/sys/devices/.../pciDDDDD:BB
> > +Date:		December, 2024
> > +Contact:	linux-pci@vger.kernel.org
> > +Description:
> > +		A PCI host bridge device parents a PCI bus device topology. PCI
> > +		controllers may also parent host bridges. The DDDDD:BB format
> > +		convey the PCI domain number and the bus number for root ports
> > +		of the host bridge.
> 
> "Root ports" doesn't seem quite right here; BB is the root bus number,
> and makes sense even for conventional PCI.
> 
> I know IDE etc is PCIe-specific, but I think saying "the PCI domain
> number and root bus number of the host bridge" would be more accurate
> since there can be things other than Root Ports on the root bus, e.g.,
> conventional PCI devices, RCiEPs, RCECs, etc.

Makes sense, adopted your wording.

> 
> Typical formatting of domain is %04x.

Oh, whoops, fixed that by doing:

    s/DDDDD/DDDD/

...throughout the set.

> 
> > +What:		pciDDDDD:BB/streamN:DDDDD:BB:DD:F
> > +Date:		December, 2024
> > +Contact:	linux-pci@vger.kernel.org
> > +Description:
> > +		(RO) When a host-bridge has established a secure connection,
> > +		typically PCIe IDE, between a host-bridge an endpoint, this
> > +		symlink appears. The primary function is to account how many
> > +		streams can be returned to the available secure streams pool by
> > +		invoking the tsm/disconnect flow. The link points to the
> > +		endpoint PCI device at domain:DDDDD bus:BB device:DD function:F.
> 
> s/host-bridge an endpoint/host bridge and an endpoint/

Fixed.

> > + * Retrieve stream association parameters for devid (RID) and resources
> > + * (device address ranges)
> 
> This and other exported functions probably should have kernel-doc.
> Document only the implemented parts.
> 
> > +void pci_ide_stream_probe(struct pci_dev *pdev, struct pci_ide *ide)
> 
> > +void pci_ide_stream_teardown(struct pci_dev *pdev, struct pci_ide *ide,
> > +			     enum pci_ide_flags flags)
> > +{
> > +	struct pci_host_bridge *hb = pci_find_host_bridge(pdev->bus);
> > +	struct pci_dev *rp = pcie_find_root_port(pdev);
> > +
> > +	__pci_ide_stream_teardown(pdev, ide);
> > +	if (flags & PCI_IDE_SETUP_ROOT_PORT)
> > +		__pci_ide_stream_teardown(rp, ide);
> 
> Looks like this relies on the caller to supply the same flags as they
> previously supplied to pci_ide_stream_setup()?  Could/should we
> remember the flags to remove the possibility of leaking the RP setup
> or trying to teardown RP setup that wasn't done?

I addressed this by decomposing this function into a "register" step and
a "setup" step according to Alexey's feedback. That removes the
responsibility of the core to remember the flags and puts the onus on
the leaf driver to remember to program the RP if its TSM implementation
requires that (some do not).

In other words the leak risk is contained to pairing "stream register"
with "stream unregister" calls, and that is independent of this now
deleted PCI_IDE_SETUP_ROOT_PORT detail.

> > +++ b/include/linux/pci.h
> > @@ -601,6 +601,10 @@ struct pci_host_bridge {
> >  	int		domain_nr;
> >  	struct list_head windows;	/* resource_entry */
> >  	struct list_head dma_ranges;	/* dma ranges resource list */
> > +#ifdef CONFIG_PCI_IDE			/* track IDE stream id allocation */
> > +	DECLARE_BITMAP(ide_stream_ids, PCI_IDE_SEL_CTL_ID_MAX + 1);
> > +	struct resource ide_stream_res; /* track ide stream address association */
> 
> Seems like overkill to repeat this.  Probably remove the comment on
> the #ifdef and s/ide/IDE/ here.

Done.