On Tue, Jun 16, 2015 at 08:30:42PM +0200, Laszlo Ersek wrote: > 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 (or flag), because said device > model has a fixed (non-conditional) SHPC_VMSTATE() field, which would > crash outgoing migration without a preceding shpc_init() call. (And the > new property (or flag) would eliminate the shpc_init() call.) Changing > SHPC_VMSTATE() into an optional subsection, in order to prevent the crash, > would in turn break incoming migration from older hosts. > > Therefore implement a separate, 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. The new device > model is a stripped-down copy of "pci-bridge"; it is very small and > simple, and shouldn't cause a significant maintenance burden.
This is making user-visible changes because of implementation detail. To address cross-version migration, just add field_exists to SHPC_VMSTATE, checking the new property. > Suggested-by: Marcel Apfelbaum <mar...@redhat.com> > Cc: Marcel Apfelbaum <mar...@redhat.com> > Cc: Michael S. Tsirkin <m...@redhat.com> > Cc: Markus Armbruster <arm...@redhat.com> > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > Reviewed-by: Marcel Apfelbaum <mar...@redhat.com> > Tested-by: Marcel Apfelbaum <mar...@redhat.com> > --- > > Notes: > v5: > - Describe in both the commit message and a code comment how > "pci-basic-bridge" relates to "pci-bridge" [Markus]. No functional > changes. > > v4: > - unchanged > > - unlike in v2 and v3, in v4 I'm formatting this as any other patch, > without "--find-copies-harder" etc. That formatting was visible (for > the exact same patch) in v3 and v4, so let's format the patch now in > its "genuine glory" :) It shows that the patch is quite small. > > v3: > - unchanged, added Marcel's R-b > > v2: > - Replaced the corresponding v1 patch with a new approach. Instead of > considering the PXB's disabled SHPC BAR in the PXB's _CRS (and > correspondingly, punching it out of the main _CRS), this patch creates > a separate device model that lacks the SHPC functionality completely. > > I already know from IRC that Michael doesn't like this, but that's > okay: I'm formatting this with "-C35% --find-copies-harder", so that > the differences are obvious against the clone origin, and Michael can > pinpoint the locations where and how I should use a new device > property instead, in the original device model. > > (That said, I did test this code, with both Windows and Linux, and > compared the dumped / decompiled SSDTs too.) > > hw/pci-bridge/Makefile.objs | 1 + > include/hw/pci/pci.h | 1 + > hw/pci-bridge/pci_basic_bridge_dev.c | 124 > +++++++++++++++++++++++++++++++++++ > hw/pci-bridge/pci_expander_bridge.c | 2 +- > 4 files changed, 127 insertions(+), 1 deletion(-) > create mode 100644 hw/pci-bridge/pci_basic_bridge_dev.c > > diff --git a/hw/pci-bridge/Makefile.objs b/hw/pci-bridge/Makefile.objs > index f2adfe3..bcfe753 100644 > --- a/hw/pci-bridge/Makefile.objs > +++ b/hw/pci-bridge/Makefile.objs > @@ -1,4 +1,5 @@ > common-obj-y += pci_bridge_dev.o > +common-obj-y += pci_basic_bridge_dev.o > common-obj-y += pci_expander_bridge.o > common-obj-$(CONFIG_XIO3130) += xio3130_upstream.o xio3130_downstream.o > common-obj-$(CONFIG_IOH3420) += ioh3420.o > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index d44bc84..619c31a 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -92,6 +92,7 @@ > #define PCI_DEVICE_ID_REDHAT_SDHCI 0x0007 > #define PCI_DEVICE_ID_REDHAT_PCIE_HOST 0x0008 > #define PCI_DEVICE_ID_REDHAT_PXB 0x0009 > +#define PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE 0x000A > #define PCI_DEVICE_ID_REDHAT_QXL 0x0100 > > #define FMT_PCIBUS PRIx64 > diff --git a/hw/pci-bridge/pci_basic_bridge_dev.c > b/hw/pci-bridge/pci_basic_bridge_dev.c > new file mode 100644 > index 0000000..0329d1b > --- /dev/null > +++ b/hw/pci-bridge/pci_basic_bridge_dev.c > @@ -0,0 +1,124 @@ > +/* > + * PCI Bridge Device without interrupt and SHPC / hotplug support. > + * > + * Copyright (c) 2011, 2015 Red Hat Inc. > + * Authors: Michael S. Tsirkin <m...@redhat.com> > + * Laszlo Ersek <ler...@redhat.com> > + * > + * This device is a simplified clone of pci-bridge: > + * - no SHPC bar & underlying MemoryRegion > + * - no interrupt (INTx or MSI) > + * - no flags, no "msi" property > + * - not a TYPE_HOTPLUG_HANDLER > + * - uses the generic PCI bridge write-config and reset callbacks > + * - separate PCI device ID > + * - most importantly, separate VMState description for migration (lack of > SHPC > + * prevents reuse of "pci-bridge", which statically depends on SHPC) > + * > + * > http://www.pcisig.com/specifications/conventional/pci_to_pci_bridge_architecture/ > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include "hw/pci/pci_bridge.h" > +#include "hw/pci/pci_ids.h" > +#include "hw/pci/slotid_cap.h" > +#include "exec/memory.h" > +#include "hw/pci/pci_bus.h" > + > +#define TYPE_PCI_BASIC_BRIDGE_DEV "pci-basic-bridge" > +#define PCI_BASIC_BRIDGE_DEV(obj) \ > + OBJECT_CHECK(PCIBasicBridgeDev, (obj), TYPE_PCI_BASIC_BRIDGE_DEV) > + > +struct PCIBasicBridgeDev { > + /*< private >*/ > + PCIBridge parent_obj; > + /*< public >*/ > + > + uint8_t chassis_nr; > +}; > +typedef struct PCIBasicBridgeDev PCIBasicBridgeDev; > + > +static int pci_basic_bridge_dev_initfn(PCIDevice *dev) > +{ > + PCIBasicBridgeDev *bridge_dev = PCI_BASIC_BRIDGE_DEV(dev); > + int err; > + > + err = pci_bridge_initfn(dev, TYPE_PCI_BUS); > + if (err) { > + goto bridge_error; > + } > + err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); > + if (err) { > + goto slotid_error; > + } > + return 0; > +slotid_error: > + pci_bridge_exitfn(dev); > +bridge_error: > + return err; > +} > + > +static void pci_basic_bridge_dev_exitfn(PCIDevice *dev) > +{ > + slotid_cap_cleanup(dev); > + pci_bridge_exitfn(dev); > +} > + > +static Property pci_basic_bridge_dev_properties[] = { > + /* Note: 0 is not a legal chassis number. */ > + DEFINE_PROP_UINT8("chassis_nr", PCIBasicBridgeDev, chassis_nr, 0), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static const VMStateDescription pci_basic_bridge_dev_vmstate = { > + .name = "pci_basic_bridge", > + .fields = (VMStateField[]) { > + VMSTATE_PCI_DEVICE(parent_obj, PCIBridge), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static void pci_basic_bridge_dev_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + > + k->init = pci_basic_bridge_dev_initfn; > + k->exit = pci_basic_bridge_dev_exitfn; > + k->config_write = pci_bridge_write_config; > + k->vendor_id = PCI_VENDOR_ID_REDHAT; > + k->device_id = PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE; > + k->class_id = PCI_CLASS_BRIDGE_PCI; > + k->is_bridge = 1, > + dc->desc = "Basic PCI Bridge (no interrupt, no hotplug)"; > + dc->reset = pci_bridge_reset; > + dc->props = pci_basic_bridge_dev_properties; > + dc->vmsd = &pci_basic_bridge_dev_vmstate; > + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > +} > + > +static const TypeInfo pci_basic_bridge_dev_info = { > + .name = TYPE_PCI_BASIC_BRIDGE_DEV, > + .parent = TYPE_PCI_BRIDGE, > + .instance_size = sizeof(PCIBasicBridgeDev), > + .class_init = pci_basic_bridge_dev_class_init, > +}; > + > +static void pci_basic_bridge_dev_register(void) > +{ > + type_register_static(&pci_basic_bridge_dev_info); > +} > + > +type_init(pci_basic_bridge_dev_register); > diff --git a/hw/pci-bridge/pci_expander_bridge.c > b/hw/pci-bridge/pci_expander_bridge.c > index ec2bb45..c7a085d 100644 > --- a/hw/pci-bridge/pci_expander_bridge.c > +++ b/hw/pci-bridge/pci_expander_bridge.c > @@ -173,7 +173,7 @@ static int pxb_dev_initfn(PCIDevice *dev) > bus->address_space_io = dev->bus->address_space_io; > bus->map_irq = pxb_map_irq_fn; > > - bds = qdev_create(BUS(bus), "pci-bridge"); > + bds = qdev_create(BUS(bus), "pci-basic-bridge"); > bds->id = dev_name; > qdev_prop_set_uint8(bds, "chassis_nr", pxb->bus_nr); > > -- > 1.8.3.1 >