[PATCH] MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top

[PATCH] MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top

[Ruiyu Ni]

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
-- 
2.12.2.windows.2

Re: [PATCH] MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Ni, Ruiyu 
Sent: Thursday, July 27, 2017 11:06 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: [PATCH] MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top

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
--
2.12.2.windows.2

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

[Laszlo Ersek]

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
>

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

[Ni, Ruiyu]

Laszlo,
Thanks for raising this questions.
Please see my embedded reply in below

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, July 27, 2017 7:23 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Marcel Apfelbaum <marcel@redhat.com>; Aleksandr Bezzubikov
> <zuban32s@gmail.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid
> hang when BUS pad resource is not in top]
> 
> 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.
[Ray] It's in bus allocation phase. So the PciBus driver only cares about the
bus padding.
PciBus driver firstly allocates the bus resource and programs the PCI-PCI bridges
to set the bus numbers.
After that, PciBus allocates the IO/MMIO resources and program the BARs in
PCI devices & PCI-PCI bridges.
So it's good here.
> 
> (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:
[Ray] Bus number padding should be considered for "non-root" HPCs.
So I agree it's a bug/gap I need to fix. I noticed that already before your question.
And I do have a plan to fix it though the priority is not very high.

> - 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?
[Ray] In summary, site(1) is good. Site(2) contains a bug/gap.
Would you please submit a Bugzilla to record this issue?

But can you just return the non-root HPCs as root for a workaround?
What prevent you from reporting the HPC as non-root?
My opinion is the differences between non-root and root HPCs are:
1. Explicitly initialization is needed for root HPCs
2. Root HPCs may not be detected as hot-pluggable by PciBus using standard ways
If your platform doesn't report any root HPCs, why not just report non-root as root ones?

> 
> 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
> >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

On 07/28/17 12:19, Ni, Ruiyu wrote:
> Laszlo,
> Thanks for raising this questions.
> Please see my embedded reply in below
> 
> Thanks/Ray
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Thursday, July 27, 2017 7:23 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>> Cc: Marcel Apfelbaum <marcel@redhat.com>; Aleksandr Bezzubikov
>> <zuban32s@gmail.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid
>> hang when BUS pad resource is not in top]
>>
>> 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.
> [Ray] It's in bus allocation phase. So the PciBus driver only cares about the
> bus padding.
> PciBus driver firstly allocates the bus resource and programs the PCI-PCI bridges
> to set the bus numbers.
> After that, PciBus allocates the IO/MMIO resources and program the BARs in
> PCI devices & PCI-PCI bridges.
> So it's good here.
>>
>> (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:
> [Ray] Bus number padding should be considered for "non-root" HPCs.
> So I agree it's a bug/gap I need to fix. I noticed that already before your question.
> And I do have a plan to fix it though the priority is not very high.

Awesome, thank you!

It is not very urgent for us. Do you agree that I file a TianoCore BZ
about this?

> 
>> - 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?
> [Ray] In summary, site(1) is good. Site(2) contains a bug/gap.
> Would you please submit a Bugzilla to record this issue?

Thank you for answering my question :) I've now filed the following
TianoCore BZ:

https://bugzilla.tianocore.org/show_bug.cgi?id=656

> But can you just return the non-root HPCs as root for a workaround?
> What prevent you from reporting the HPC as non-root?

Yes, I've thought of this as well.

The problem is that the "a-priori" knowledge of the root HPCs, that the
PI spec mentions for GetRootHpcList(), is actually *dynamic* knowledge
in QEMU's case.

In "OvmfPkg/PciHotPlugInitDxe", I could only collect the affected
bridges by doing a full (recursive) PCI hierarchy enumeration, and look
into the config space of each "candidate" controller. However, PciBusDxe
already implements the full enumeration, so I don't think
PciHotPlugInitDxe should it do internally. (In particular in a platform
hook that could be called from a "sensitive" location in PciBusDxe.)

> My opinion is the differences between non-root and root HPCs are:
> 1. Explicitly initialization is needed for root HPCs

I agree that this is one difference.

Such initialization is not needed in QEMU's case however.

> 2. Root HPCs may not be detected as hot-pluggable by PciBus using standard ways

I agree that this is the other difference.

All of QEMU's hotplug controllers can be enumerated in standard ways
however.

> If your platform doesn't report any root HPCs, why not just report non-root as root ones?

(See above:) because the list of non-root HPCs is not known to OVMF in
any static way, it is dynamic (boot time) information. And, QEMU does
not expose this information to OVMF in any dedicated, easy-to-consume way.

The only way to find out about the non-root HPCs is to actually traverse
the PCI hierarchy.

Given that PciBusDxe is in the middle of doing exactly that, I think
that OvmfPkg/PciHotPlugInitDxe should not perform an "internal"
enumeration in GetRootHpcList(). I don't think that OvmfPkg should have
a private implementation of this PCI traversal, when the "main"
traversal is already done in PciBusDxe. (And, likely much better than
what I could write.)

When you fix TianoCore BZ#656, that will be perfect for us. :)

Thank you very much!
Laszlo