bus number padding [was: MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top]

[Laszlo Ersek <lersek@redhat.com>]

Hello Ray,

please excuse me for hijacking this thread a little bit, but I'm very
happy that you happen to be looking at this exact part of the code,
which is also what I've been discussing with Marcel and Aleksandr (CC'd)
due to a different reason.

(The relevant sub-thread is at

  Allow RedHat PCI bridges reserve more buses than necessary during init

  http://mid.mail-archive.com/d0bd04f4-1ac4-0e3a-885f-2ceb6180f69a@redhat.com

but it's very long, so I'm not asking you to read that -- instead,
please let me ask you our question independently and in a self-contained
form.)

In "OvmfPkg/PciHotPlugInitDxe/", we plan to return some BUS resource
paddings, from the EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()
function.

QEMU has no "root" hotplug controllers, only "non-root" hotplug
controllers (using the terminology from "12.6 PCI Hot Plug PCI
Initialization Protocol" in the PI-1.6 spec). Therefore OVMF's
EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetRootHpcList() returns an empty array.
The BUS resource padding that we'd like to return from
GetResourcePadding() would be for "non-root" hotplug controllers.

According to the PI spec, this is a valid thing to do:

* From "12.5 Sample Implementation for a Platform Containing PCI
  Hot Plug* Slots":

> [...] For all the root HPCs and the nonroot HPCs, call
> EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() to obtain the
> amount of overallocation and add that amount to the requests from the
> physical devices. Reprogram the bus numbers by taking into account the
> bus resource padding information. [...]

* From "12.6 PCI Hot Plug PCI Initialization Protocol,
  EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()":

> [...] This member function is called for all the root HPCs and nonroot
> HPCs that are detected by the PCI bus enumerator. [...] The PCI bus
> driver is responsible for adding this resource request to the resource
> requests by the physical PCI devices. [...]

I searched PciBusDxe for GetResourcePadding() call sites, and there are
two:

(1) In "MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c", function
    GetResourcePaddingForHpb(). This call site seems to work as follows,
    for our purposes:

    GetResourcePaddingPpb()                   [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c]
      GetResourcePaddingForHpb()              [MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c]
        gPciHotPlugInit->GetResourcePadding()
        //
        // and then save the resource descriptors, including the bus
        // padding, in PciIoDevice->ResourcePaddingDescriptors
        //

    ResourcePaddingPolicy()  [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c]
      ApplyResourcePadding() [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c]
        //
        // translate IO and MMIO resources from
        // PciDev->ResourcePaddingDescriptors to internal padding
        // representation
        //

    This call site and processing seem to be active for "non-root"
    hotplug controllers, but they do not consider bus numbers, only IO
    and MMIO.

(2) In the PciScanBus() function, file
    "MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c", we have another
    GetResourcePadding() call -- your present patch is updating this
    code part.

    From the resource padding list returned by this GetResourcePadding()
    invocation, PciBusDxe cares only about the bus numbers. I think
    that's probably fine. However, in this location,
    GetResourcePadding() is called *only* for "root" hotplug
    controllers, of which QEMU has none.

The end result is that bus number paddings are not considered at all
for "non-root" hotplug controllers:
- site (1) is active for "non-root" HPCs, but it ignores bus number
  paddings,
- site (2) processes bus number paddings, but it is inactive for
  "non-root" HPCs.

This doesn't seem to match what the PI spec says. Is this behavior
intentional in PciBusDxe? If so, what is the reason for it?

Thank you!
Laszlo

On 07/27/17 05:05, Ruiyu Ni wrote:
> PciScanBus() assumes the GetResourcePadding() puts BUS descriptor
> in the very beginning, if it's not, the Descriptors will be updated
> to point to middle of the pool buffer, which can cause
> FreePool(Descriptors) hang in DEBUG image.
> No functionality impact to RELEASE image.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index e1d62e8c21..8b076e8791 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Internal library implementation for PCI Bus module.
>
> -Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
>  (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
> @@ -986,6 +986,7 @@ PciScanBus (
>    UINT64                            PciAddress;
>    EFI_HPC_PADDING_ATTRIBUTES        Attributes;
>    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptors;
> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *NextDescriptors;
>    UINT16                            BusRange;
>    EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL   *PciRootBridgeIo;
>    BOOLEAN                           BusPadding;
> @@ -1143,8 +1144,9 @@ PciScanBus (
>                }
>
>                BusRange = 0;
> +              NextDescriptors = Descriptors;
>                Status = PciGetBusRange (
> -                        &Descriptors,
> +                        &NextDescriptors,
>                          NULL,
>                          NULL,
>                          &BusRange
>