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

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

Jonathan Cameron wrote:
> On Thu, 15 May 2025 22:47:27 -0700
> Dan Williams <dan.j.williams@intel.com> 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.
> > 
> > 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:%s
> > 
> > Where the tuple of integers reflects the allocated platform, Root Port,
> > and Endpoint stream index (Selective IDE Stream Register Block) values,
> > and the %s is the endpoint device name.
> > 
> > Thanks to Wu Hao for a draft implementation of this infrastructure.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
> > Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> > Co-developed-by: Yilun Xu <yilun.xu@linux.intel.com>
> > Signed-off-by: Yilun Xu <yilun.xu@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> A few little comments inline.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../ABI/testing/sysfs-devices-pci-host-bridge |  38 ++
> >  MAINTAINERS                                   |   1 +
> >  drivers/pci/ide.c                             | 366 ++++++++++++++++++
> >  include/linux/pci-ide.h                       |  76 ++++
> >  include/linux/pci.h                           |   6 +
> >  include/uapi/linux/pci_regs.h                 |   2 +
> >  6 files changed, 489 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-devices-pci-host-bridge
> >  create mode 100644 include/linux/pci-ide.h
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-devices-pci-host-bridge b/Documentation/ABI/testing/sysfs-devices-pci-host-bridge
> > new file mode 100644
> > index 000000000000..d592b68c7333
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-devices-pci-host-bridge
> > @@ -0,0 +1,38 @@
> > +What:		/sys/devices/pciDDDD:BB
> > +		/sys/devices/.../pciDDDD: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 DDDD:BB format
> > +		conveys the PCI domain (ACPI segment) number and root bus number
> > +		(in hexadecimal) of the host bridge. Note that the domain number
> > +		may be larger than the 16-bits that the "DDDD" format implies
> > +		for emulated host-bridges.
> > +
> > +What:		pciDDDD:BB/firmware_node
> > +Date:		December, 2024
> > +Contact:	linux-pci@vger.kernel.org
> > +Description:
> > +		(RO) Symlink to the platform firmware device object "companion"
> > +		of the host bridge. For example, an ACPI device with an _HID of
> > +		PNP0A08 (/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00). See
> > +		/sys/devices/pciDDDD:BB entry for details about the DDDD:BB
> > +		format.
> 
> No problem with this documentation but not I think related to this patch and
> could go upstream before this?

Sure, given there are a few other things in this series that could go upstream
in advance of the TSM bits, I will pull this and those out.

> 
> > +
> > +What:		pciDDDD:BB/streamH.R.E:DDDD:BB:DD:F
> > +Date:		December, 2024
> > +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 at domain:DDDD
> > +		bus:BB device:DD function:F. Where 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.
> 
> > diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> > index 98a51596e329..a529926647f4 100644
> > --- a/drivers/pci/ide.c
> > +++ b/drivers/pci/ide.c
> 
> > +/**
> > + * pci_ide_stream_enable() - after setup, enable the stream
> > + * @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.
> > + */
> > +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)
> > +		return -ENXIO;
> 
> Trivial but blank line here would I think help readability a tiny bit.

Ack.

> 
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_ide_stream_enable);
> 
> > diff --git a/include/linux/pci-ide.h b/include/linux/pci-ide.h
> > new file mode 100644
> > index 000000000000..0753c3cd752a
> > --- /dev/null
> > +++ b/include/linux/pci-ide.h
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> > +
> > +/* PCIe 6.2 section 6.33 Integrity & Data Encryption (IDE) */
> > +
> > +#ifndef __PCI_IDE_H__
> > +#define __PCI_IDE_H__
> > +
> > +#include <linux/range.h>
> 
> Needed?  I'm guessing it was and isn't any more.

Yup, good catch.

> > +
> > +#define SEL_ADDR1_LOWER_MASK GENMASK(31, 20)
> > +#define SEL_ADDR_UPPER_MASK GENMASK_ULL(63, 32)
> > +#define PREP_PCI_IDE_SEL_ADDR1(base, limit)                    \
> 
> ADDR_1 would be more consistent.
> 
> However, unless we are going to see a lot of these I'd personally prefer
> to see this lot inline in the code.
> 
> > +	(FIELD_PREP(PCI_IDE_SEL_ADDR_1_VALID, 1) |             \
> > +	 FIELD_PREP(PCI_IDE_SEL_ADDR_1_BASE_LOW_MASK,          \
> > +		    FIELD_GET(SEL_ADDR1_LOWER_MASK, (base))) | \
> 
> This is a case I'd just not use FIELD_PREP / GET for. Just ends up
> confusing and needs definitions that make little sense on their own.
> 	lower_32_bits(base) & PCI_IDE_SEL_ADDR_1_BASE_LOW_MASK
> perhaps.
> 
> > +	 FIELD_PREP(PCI_IDE_SEL_ADDR_1_LIMIT_LOW_MASK,         \
> > +		    FIELD_GET(SEL_ADDR1_LOWER_MASK, (limit))))
> 
> Maybe use upper_32_bits() for this one.
> 
> However it is an odd macro and I can't immediately find where it is used
> so maybe just drop it?

It was in an earlier rev, but I dropped it for simplicity. For now,
there is no address limitations within the the stream.

> 
> > +
> > +#define PREP_PCI_IDE_SEL_RID_2(base, domain)               \
> > +	(FIELD_PREP(PCI_IDE_SEL_RID_2_VALID, 1) |          \
> > +	 FIELD_PREP(PCI_IDE_SEL_RID_2_BASE_MASK, (base)) | \
> > +	 FIELD_PREP(PCI_IDE_SEL_RID_2_SEG_MASK, (domain)))
> This one I'd prefer to see inline.
> 
> > +/**
> > + * struct pci_ide_partner - Per port 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
> > + */
> > +struct pci_ide_partner {
> > +	u16 rid_start;
> > +	u16 rid_end;
> > +	u8 stream_index;
> > +};
> > +
> > +/**
> > + * struct pci_ide - PCIe Selective IDE Stream descriptor
> > + * @pdev: PCIe Endpoint for the stream
> > + * @partner: settings for both partner ports in a stream
> > + * @host_bridge_stream: track platform Stream index
> 
> Why the capital S?  Seems a little inconsistent across different comments.

So this was deliberate, but does indeed look strange. Bjorn has asked,
and I agree with him, that specification terms be capitalized. So the
"Stream index" is the PCI defined number that gets programmed into
hardware registers for a Selective IDE Stream, and "stream" is the name
of the related Linux software collateral.

That is indeed too subtle. I think adding the full Selective IDE Stream
for those cases will help distinguish.