Re: [PATCH v4 07/10] PCI/IDE: Add IDE establishment helpers

[Bjorn Helgaas <helgaas@kernel.org>]

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.

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

> 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@vger.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.)

> +++ 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.

> +/**
> + * pci_ide_stream_enable() - try to enable a Selective IDE Stream

Do or do not.  There is no try.

> + * @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.

> + */
> +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".

> + * @enable: flag whether to run pci_ide_stream_disable for this parnter slot