Re: [Qemu-devel] [PATCH v4 1/4] hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> OVMF downloads the ACPI linker/loader script from QEMU when the edk2 PCI
> Bus driver globally signals the firmware that PCI enumeration and resource
> allocation have completed. At this point QEMU regenerates the ACPI payload
> in an fw_cfg read callback, and this is when the PXB's _CRS gets
> populated.
>
> Unfortunately, when this happens, the PCI_COMMAND_MEMORY bit is clear in
> the root bus's command register, *unlike* under SeaBIOS. The consequences
> unfold as follows:
>
> - When build_crs() fetches dev->io_regions[i].addr, it is all-bits-one,
>   because pci_update_mappings() --> pci_bar_address() calculated it as
>   PCI_BAR_UNMAPPED, due to the PCI_COMMAND_MEMORY bit being clear.
>
> - Consequently, the SHPC MMIO BAR (bar 0) of the bridge is not added to
>   the _CRS, *despite* having been programmed in PCI config space.
>
> - Similarly, the SHPC MMIO BAR of the PXB is not removed from the main
>   root bus's DWordMemory descriptor.
>
> - Guest OSes (Linux and Windows alike) notice the pre-programmed SHPC BAR
>   within the PXB's config space, and notice that it conflicts with the
>   main root bus's memory resource descriptors. Linux reports
>
>   pci 0000:04:00.0: BAR 0: can't assign mem (size 0x100)
>   pci 0000:04:00.0: BAR 0: trying firmware assignment [mem
>                            0x88200000-0x882000ff 64bit]
>   pci 0000:04:00.0: BAR 0: [mem 0x88200000-0x882000ff 64bit] conflicts
>                            with PCI Bus 0000:00 [mem
>                            0x88200000-0xfebfffff]
>
>   While Windows Server 2012 R2 reports
>
>     https://technet.microsoft.com/en-us/library/cc732199%28v=ws.10%29.aspx
>
>     This device cannot find enough free resources that it can use. If you
>     want to use this device, you will need to disable one of the other
>     devices on this system. (Code 12)
>
> This issue was apparently encountered earlier, see the "hack" in:
>
>   https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg02983.html
>
> and the current hole-punching logic in build_crs() and build_ssdt() is
> probably supposed to remedy exactly that problem -- however, for OVMF they
> don't work, because at the end of the PCI enumeration and resource
> allocation, which cues the ACPI linker/loader client, the command register
> is clear.
>
> It has been suggested to implement the above "hack" more cleanly and
> permanently. Unfortunately, we can't just disable the SHPC bar of
> TYPE_PCI_BRIDGE_DEV based on a new property, because this device model has
> class-level ties to hotplug, and the SHPC bar (and the interrupt)
> originate from there.
>
> Therefore implement a more basic bridge device type, to be used by PXB,
> that has no SHPC / hotplug support at all, and consequently (see commit
> c008ac0c), no need for an interrupt / MSI either.
>
> Suggested-by: Marcel Apfelbaum <address@hidden>
> Cc: Marcel Apfelbaum <address@hidden>
> Cc: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Laszlo Ersek <address@hidden>
> Reviewed-by: Marcel Apfelbaum <address@hidden>

Suggest to have the commit message state explicitly how
"pci-basic-bridge" is related to "pci-bridge".  If I understand it
correctly, it's a stripped-down copy.  A comment in the code might be in
order, too.

We'd normally avoid copying, and instead have the same code implement
both variants of the device.  I figure your reason for copying is that
the "copy" implementing "pci-basic-bridge" is so small and simple that
avoiding it wouldn't make things simpler or more maintainable.  Suggest
to put that rationale into the commit message as well.