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/[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. (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 <[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
