Laszlo, Thanks for raising this questions. Please see my embedded reply in below
Thanks/Ray > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of > Laszlo Ersek > Sent: Thursday, July 27, 2017 7:23 PM > To: Ni, Ruiyu <[email protected]>; [email protected] > Cc: Marcel Apfelbaum <[email protected]>; Aleksandr Bezzubikov > <[email protected]>; Zeng, Star <[email protected]> > 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- > [email protected] > > 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 <[email protected]> > > Cc: Star Zeng <[email protected]> > > --- > > 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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
