Jonathan Cameron wrote:
> On Thu, 15 May 2025 22:47:27 -0700
> Dan Williams <dan.j.williams@xxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Lukas Wunner <lukas@xxxxxxxxx>
> > Cc: Samuel Ortiz <sameo@xxxxxxxxxxxx>
> > Co-developed-by: Alexey Kardashevskiy <aik@xxxxxxx>
> > Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
> > Co-developed-by: Yilun Xu <yilun.xu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Yilun Xu <yilun.xu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxx
> > +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@xxxxxxxxxxxxxxx
> > +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@xxxxxxxxxxxxxxx
> > +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.
