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