On 2015-06-08 00:51:27, Laszlo Ersek wrote: > On 06/07/15 06:51, Jordan Justen wrote: > > On 2015-06-05 16:47:40, Laszlo Ersek wrote: > >> The first 9 patches are uninteresting (although they certainly decimated > >> my grey matter): > >> > >> - Patch #1 copies PciHostBridgeDxe from PcAtChipsetPkg to OvmfPkg. > > > > Arg. After I have bugged the DuetPkg and CorebootPayloadPkg package > > owners to try to fixup PcAtChipsetPkg/PciHostBridgeDxe so all 3 > > platforms could potentially use it... > > > > As far as I know, no one else is using PcAtChipsetPkg/PciHostBridgeDxe > > right now, so if OVMF stops, then, I think, the attempt to make a > > common driver in PcAtChipsetPkg has become a failure, and it should be > > deleted. > > I can add such a patch, but its acceptance would depend on Ray.
So, do I need to read between the lines and assume a part of your motivation for moving the driver is to avoid being dependent on acceptance from Ray for your patches? :) Regarding this driver, it was adding in 2009 / 21b404d for OVMF. I'm still not aware of another platform which uses the driver. But I've been trying to migrate OvmfPkg and DuetPkg to use common drivers where possible. (Example: b8781a7) In fact, many of the original PcAtChipsetPkg drivers came from DuetPkg so they could be used by both DuetPkg and OvmfPkg. > > That will leave CorebootPayloadPkg still using the DuetPkg > > one, and no path to resolve that odd situation. > > I'm not sure this is odd. Host bridge / root bus drivers are supposed to > be platform specific. First, I would say they generally are chipset 'north bridge' specific, and not platform specific. (Based on this, OVMF might have a good argument for making this platform specific, since it supports multiple north bridges. But, 440fx vs. q35 is not a factor at this point.) Second, the fact that CorebootPayloadPkg depends on another platform's driver is what seems odd to me. I don't think DuetPkg should be an 'upstream' package for CorebootPayloadPkg. :) > I do agree that a good part of the code could be shared between the > implementations. We usually have two ways for that: > - extracting a library class & providing several instances > - using multiple INF files (with different source file sets) in the > same directory, and/or arch dependent INF file sections. Or, adding a PCD. For example, to through out an idea, what about adding a PcdScanAdditionalPciRootBusesCount integer PCD with a default of 0? Then the changes would generally be a no-op, but QEMU could set the PCD to a higher value based on the fw-cfg value. This might also help keep the changes as a no-op for Xen OVMF. I should say that I'm just throwing out an idea here, for the purpose of discussion. If it seems reasonable, and potentially still leaves the door open for DuetPkg and CorebootPayloadPkg to all use the same driver in the future, then maybe it is worthwhile. I should also say that I'm getting pretty tired of being the only one trying get these platforms to use common code. So, while I would be dissappointed to see this driver killed in the common location, at least I could stop stressing over it. :) -Jordan > You don't like the library class approach, as far as my experience goes > (plus, the library interface would be quite arbitrary / awkward). > > The arch-specific INF file sections are not appropriate, because we have > differences between the x86-targeted drivers. > > Finally, using multiple INF files in the same subdirectory would imply > the same top-level Pkg directory as well, and that would require either > the addition of fw_cfg code to PcAtChipsetPkg, or (in the reverse > direction) non-virt packages to reach under (ie. depend on) OvmfPkg. > > > So, what of this is OVMF specific? > > As I wrote, patches 2 to 9 are just reformatting and could be done in > place. I didn't do that up-front because I thought Ray would not be > enthusiastic about reviewing 8 sizeable patches that add no features nor > bugfixes to PcAtChipsetPkg. > > Actually, all of the PciHostBridgeDxe patches except the last two could > go under PcAtChipsetPkg. The penultimate patch (the brute-force probing) > should work on physical hardware as well, but without the fw_cfg > "enlightenment" in the last patch, the performance hit is probably not > ideal. Also, it's been said on the list that host bridge / root bus > identification is platform specific and frequently hard-coded, so the > scanning might not make sense for a physical platform at all. > > And, of course, all of the prep patches lead up to the last two, so you > could argue the series (without the last two patches) would be useless > for PcAtChipsetPkg. > > > Aside from fw-cfg patch, I think the method scanning for other > > root-bridges in "OvmfPkg: PciHostBridgeDxe: look for all root buses" > > might be OVMF specific. Looking for devices on other buses seems like > > a platform specific way of doing that. > > The VendorId check per se appears to be an industry practice, > independent of virtualization: > > http://www.tldp.org/LDP/tlk/dd/pci.html > http://wiki.osdev.org/PCI > > The idea to scan all devices in order to determine root bus presence is > applied in SeaBIOS too -- on physical hardware as well --, I've been > told by Marcel Apfelbaum (who developed the QEMU side, ie. the PXB ("PCI > expander bridge") feature). Although we discussed earlier the > possibility of QEMU exposing the exact list of extra root bus numbers to > OVMF via fw_cfg, ultimately Michael Tsirkin (x86 and PCI maintainer for > QEMU) suggested to use such a scan in OVMF as well. From fw_cfg, only > the preexistent etc/etc/extra-pci-roots file is used (which gives the > extra root bus count). > > > (However, possibly something > > configurable by PCD.) > > > > What are the additional root bridges used for? > > NUMA affinity for passthrough devices: > > https://bugzilla.redhat.com/show_bug.cgi?id=1193080 > > > Description of problem: > > We need to support multiple NUMA nodes for pass-through devices. > > Operating systems can attach only a PCI root bus to a NUMA node. > > > > SeaBIOS/QEMU support for multiple PCI root buses is already on the > > mailing lists, and we may need to support this also in OVMF. > > http://thread.gmane.org/gmane.comp.emulators.qemu/340746 > > > We need multiple primary busess for a few reasons, the most important > > one is to be able to associate a pass-trough device with a guest NUMA > > node. The OS-es are able to associate a NUMA node only to a primary > > bus, not to a specific PCI device or a pci-2-pci bridge. PC machines > > support multiple NUMA nodes for CPUs and memory, however the IO was > > not yet supported. > > See also "docs/pci_expander_bridge.txt" in upstream QEMU: > > > As opposed to PCI-2-PCI bridge's secondary bus, PXB's bus > > is a primary bus and can be associated with a NUMA node > > (different from the main host bridge) allowing the guest OS > > to recognize the proximity of a pass-through device to > > other resources as RAM and CPUs. > > That document goes into QEMU command line details. The different guest > firmwares & OSes can be tested without a NUMA host and passed-through > devices too. > > ... I duplicated (and customized heavily) > PcAtChipsetPkg/PciHostBridgeDxe under ArmVirtPkg earlier. That driver > - uses PCIe with a larger config space, > - relies on a different (customized) PciLib instance, > - translates IO space access to MMIO, > - takes the (one) root bridge's resource apertures from dynamic PCDs > that were parsed from the DTB exported by QEMU, > - handles range lengths in RootBridgeIoCheckParameter() that are not > integral powers of two, > - allocates BARs top-down, > - and so on. > > The differences are far too arbitrary and incidental to design an > interface for the parts that are shared. > > If a problem is found in either driver, in code that is shared between > the copies, I'm willing to patch both drivers in sync. The code > duplication is not ideal, but the ways to eliminate it are neither. > > Thanks > Laszlo > > > > > -Jordan > > > >> I > >> paid attention and passed the --find-copies-harder option to > >> git-format-patch, hence reviewers should be able to spot the minimal > >> differences easily. > >> > >> - Patches #2 to #9 reformat the copied driver's source code so that it > >> is actually possible to work with it. Trailing whitespace is stripped, > >> overlong lines are rewrapped to 79 characters. This was suprisingly > >> difficult because the original code consistently uses 130-148 columns. > >> > >> Rather than dump the reformatting into one huge patch, I broke it up > >> in order to help reviewers, generally keeping comment reformatting > >> separate from code reformatting. These patches are responsible for the > >> bulk of the diffstat. > >> > >> This half of the series would actually apply to > >> PcAtChipsetPkg/PciHostBridgeDxe itself, and it would be possible to > >> clone the driver for OvmfPkg only at the end of the first half. > >> > >> The other half of the series is more interesting: > >> > >> - Patches #10 to #12 tweak PlatformBdsLib in preparation for multiple > >> root buses. For these: > >> > >> Cc: Gabriel Somlo <so...@cmu.edu> > >> > >> - Patches #13 to #19 clean up the cloned PciHostBridgeDxe, eliminating > >> the nominal (never used) multi-host-bridge bits, and fixing bugs in > >> the (until now unused, but soon to be exercised) multi-root-bridge > >> bits. At the end of this subset, "single host bridge" will be a > >> hard-coded trait, and "several root bridges" will be an actual > >> possibility. > >> > >> - Patch #20 detects the extra root buses with a brute-force scan, > >> creating root bridge protocol instances in turn. The feature is > >> functionally complete after this patch. > >> > >> - Patch #21 constrains the search based on fw_cfg, omitting it if QEMU > >> doesn't expose the number of extra root buses in fw_cfg, and > >> terminating the search ASAP otherwise. > >> > >> Public branch: > >> https://github.com/lersek/edk2/commits/multiple_root_bridges_bz1193080 > >> > >> Laszlo Ersek (21): > >> OvmfPkg: clone PciHostBridgeDxe from PcAtChipsetPkg > >> OvmfPkg: PciHostBridgeDxe: rewrap IoFifo source files to 79 columns > >> OvmfPkg: PciHostBridgeDxe: rewrap INF file to 79 columns > >> OvmfPkg/PciHostBridgeDxe/PciHostBridge.h: rewrap comments to 79 > >> columns > >> OvmfPkg/PciHostBridgeDxe/PciHostBridge.h: strip trailing ws from code > >> OvmfPkg/PciHostBridgeDxe/PciHostBridge.c: rewrap leading comments > >> OvmfPkg/PciHostBridgeDxe/PciHostBridge.c: rewrap code, strip trailing > >> ws > >> OvmfPkg/PciHostBridgeDxe/PciRootBridgeIo.c: rewrap leading comments > >> OvmfPkg/PciHostBridgeDxe/PciRootBridgeIo.c: rewrap code, strip > >> trailing ws > >> OvmfPkg: PlatformBdsLib: debug log interrupt line assignments > >> OvmfPkg: PlatformBdsLib: refine PCI host bridge assertion > >> OvmfPkg: PlatformBdsLib: connect all PCI root buses > >> OvmfPkg: PciHostBridgeDxe: eliminate nominal support for multiple host > >> bridges > >> OvmfPkg: PciHostBridgeDxe: kill RootBridgeNumber and > >> RootBridgeAttribute > >> OvmfPkg: PciHostBridgeDxe: embed device path in private root bridge > >> struct > >> OvmfPkg: PciHostBridgeDxe: factor out InitRootBridge() function > >> OvmfPkg: PciHostBridgeDxe: release resources on driver entry failure > >> OvmfPkg: PciHostBridgeDxe: use private buffer in > >> RootBridgeIoConfiguration() > >> OvmfPkg: PciHostBridgeDxe: eliminate > >> PCI_HOST_BRIDGE_INSTANCE.RootBridgeNumber > >> OvmfPkg: PciHostBridgeDxe: look for all root buses > >> OvmfPkg: PciHostBridgeDxe: shorten search for extra root buses > >> > >> OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 2 > >> +- > >> {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/PciHostBridgeDxe.inf | 21 > >> +- > >> OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h | 25 > >> +- > >> {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/IoFifo.h | 11 > >> +- > >> OvmfPkg/PciHostBridgeDxe/PciHostBridge.h | 651 > >> +++++ > >> OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c | 82 > >> +- > >> OvmfPkg/Library/PlatformBdsLib/PlatformData.c | 13 - > >> OvmfPkg/PciHostBridgeDxe/PciHostBridge.c | 1551 > >> ++++++++++++ > >> OvmfPkg/PciHostBridgeDxe/PciRootBridgeIo.c | 2628 > >> ++++++++++++++++++++ > >> OvmfPkg/OvmfPkgIa32.dsc | 2 > >> +- > >> OvmfPkg/OvmfPkgIa32.fdf | 2 > >> +- > >> OvmfPkg/OvmfPkgIa32X64.dsc | 2 > >> +- > >> OvmfPkg/OvmfPkgIa32X64.fdf | 2 > >> +- > >> OvmfPkg/OvmfPkgX64.dsc | 2 > >> +- > >> OvmfPkg/OvmfPkgX64.fdf | 2 > >> +- > >> {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/Ia32/IoFifo.S | 7 > >> +- > >> {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/Ia32/IoFifo.asm | 7 > >> +- > >> {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/X64/IoFifo.S | 7 > >> +- > >> {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/X64/IoFifo.asm | 7 > >> +- > >> 19 files changed, 4915 insertions(+), 109 deletions(-) > >> copy {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/PciHostBridgeDxe.inf > >> (72%) > >> copy {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/IoFifo.h (91%) > >> create mode 100644 OvmfPkg/PciHostBridgeDxe/PciHostBridge.h > >> create mode 100644 OvmfPkg/PciHostBridgeDxe/PciHostBridge.c > >> create mode 100644 OvmfPkg/PciHostBridgeDxe/PciRootBridgeIo.c > >> copy {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/Ia32/IoFifo.S (90%) > >> copy {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/Ia32/IoFifo.asm (90%) > >> copy {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/X64/IoFifo.S (92%) > >> copy {PcAtChipsetPkg => OvmfPkg}/PciHostBridgeDxe/X64/IoFifo.asm (91%) > >> > >> -- > >> 1.8.3.1 > >> > >> > >> ------------------------------------------------------------------------------ > >> _______________________________________________ > >> edk2-devel mailing list > >> edk2-devel@lists.sourceforge.net > >> https://lists.sourceforge.net/lists/listinfo/edk2-devel > ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
