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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

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