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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

Hi Dan

On 04/03/2025 07:15, 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()
>    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: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   .../ABI/testing/sysfs-devices-pci-host-bridge      |   32 ++
>   drivers/pci/ide.c                                  |  352 ++++++++++++++++++++

...

> +
> +static struct pci_ide_partner *to_settings(struct pci_dev *pdev, struct pci_ide *ide)
> +{
> +	if (!pci_is_pcie(pdev)) {
> +		pci_warn_once(pdev, "not a PCIe device\n");
> +		return NULL;
> +	}
> +
> +	switch (pci_pcie_type(pdev)) {
> +	case PCI_EXP_TYPE_ENDPOINT:
> +		if (pdev != ide->pdev) {
> +			pci_warn_once(pdev, "setup expected Endpoint: %s\n", pci_name(ide->pdev));
> +			return NULL;
> +		}
> +		return &ide->partner[PCI_IDE_EP];
> +	case PCI_EXP_TYPE_ROOT_PORT:
> +		struct pci_dev *rp = pcie_find_root_port(ide->pdev);

My (relatively old) compiler complains about this:

drivers/pci/ide.c: In function ‘to_settings’:
drivers/pci/ide.c:322:3: error: a label can only be part of a statement 
and a declaration is not a statement
   322 |   struct pci_dev *rp = pcie_find_root_port(ide->pdev);
       |   ^~~~~~

$ gcc -v
...
Target: aarch64-none-linux-gnu
...
gcc version 10.3.1 20210621 (GNU Toolchain for the A-profile 
Architecture 10.3-2021.07 (arm-10.29))

Works fine on a later version of the GCC (version 12.2)

The following hunk fixes the build for me.

diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
index 0c72985e6a65..f6f4cb71307d 100644
--- a/drivers/pci/ide.c
+++ b/drivers/pci/ide.c
@@ -318,15 +318,16 @@ static struct pci_ide_partner *to_settings(struct 
pci_dev *pdev, struct pci_ide
                         return NULL;
                 }
                 return &ide->partner[PCI_IDE_EP];
-       case PCI_EXP_TYPE_ROOT_PORT:
+       case PCI_EXP_TYPE_ROOT_PORT: {
                 struct pci_dev *rp = pcie_find_root_port(ide->pdev);

-               if (pdev != pcie_find_root_port(ide->pdev)) {
+               if (pdev != rp) {
                         pci_warn_once(pdev, "setup expected Root Port: 
%s\n",
                                       pci_name(rp));
                         return NULL;
                 }
                 return &ide->partner[PCI_IDE_RP];
+       }


Suzuki