| Message ID | 1433956033-11584-4-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/10/2015 08:07 PM, 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, 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 <marcel@redhat.com> > Cc: Marcel Apfelbaum <marcel@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > 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 Nice, I'll use that. , 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 + > .../{pci_bridge_dev.c => pci_basic_bridge_dev.c} | 128 ++++++--------------- > hw/pci-bridge/pci_expander_bridge.c | 2 +- > 4 files changed, 35 insertions(+), 97 deletions(-) > copy hw/pci-bridge/{pci_bridge_dev.c => pci_basic_bridge_dev.c} (35%) > > 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_bridge_dev.c b/hw/pci-bridge/pci_basic_bridge_dev.c > similarity index 35% > copy from hw/pci-bridge/pci_bridge_dev.c > copy to hw/pci-bridge/pci_basic_bridge_dev.c > index 36f73e1..d8bfb6c 100644 > --- a/hw/pci-bridge/pci_bridge_dev.c > +++ b/hw/pci-bridge/pci_basic_bridge_dev.c > @@ -1,7 +1,9 @@ > /* > - * Standard PCI Bridge Device > + * PCI Bridge Device without interrupt and SHPC / hotplug support. > * > - * Copyright (c) 2011 Red Hat Inc. Author: Michael S. Tsirkin <mst@redhat.com> > + * Copyright (c) 2011, 2015 Red Hat Inc. > + * Authors: Michael S. Tsirkin <mst@redhat.com> > + * Laszlo Ersek <lersek@redhat.com> > * > * http://www.pcisig.com/specifications/conventional/pci_to_pci_bridge_architecture/ > * > @@ -21,158 +23,92 @@ > > #include "hw/pci/pci_bridge.h" > #include "hw/pci/pci_ids.h" > -#include "hw/pci/msi.h" > -#include "hw/pci/shpc.h" > #include "hw/pci/slotid_cap.h" > #include "exec/memory.h" > #include "hw/pci/pci_bus.h" > -#include "hw/hotplug.h" > > -#define TYPE_PCI_BRIDGE_DEV "pci-bridge" > -#define PCI_BRIDGE_DEV(obj) \ > - OBJECT_CHECK(PCIBridgeDev, (obj), TYPE_PCI_BRIDGE_DEV) > +#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 PCIBridgeDev { > +struct PCIBasicBridgeDev { > /*< private >*/ > PCIBridge parent_obj; > /*< public >*/ > > - MemoryRegion bar; > uint8_t chassis_nr; > -#define PCI_BRIDGE_DEV_F_MSI_REQ 0 > - uint32_t flags; > }; > -typedef struct PCIBridgeDev PCIBridgeDev; > +typedef struct PCIBasicBridgeDev PCIBasicBridgeDev; > > -static int pci_bridge_dev_initfn(PCIDevice *dev) > +static int pci_basic_bridge_dev_initfn(PCIDevice *dev) > { > - PCIBridge *br = PCI_BRIDGE(dev); > - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); > + PCIBasicBridgeDev *bridge_dev = PCI_BASIC_BRIDGE_DEV(dev); > int err; > > err = pci_bridge_initfn(dev, TYPE_PCI_BUS); > if (err) { > goto bridge_error; > } > - dev->config[PCI_INTERRUPT_PIN] = 0x1; > - memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", shpc_bar_size(dev)); > - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0); > - if (err) { > - goto shpc_error; > - } > err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); > if (err) { > goto slotid_error; > } > - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && > - msi_supported) { > - err = msi_init(dev, 0, 1, true, true); > - if (err < 0) { > - goto msi_error; > - } > - } > - /* TODO: spec recommends using 64 bit prefetcheable BAR. > - * Check whether that works well. */ > - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY | > - PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar); > return 0; > -msi_error: > - slotid_cap_cleanup(dev); > slotid_error: > - shpc_cleanup(dev, &bridge_dev->bar); > -shpc_error: > pci_bridge_exitfn(dev); > bridge_error: > return err; > } > > -static void pci_bridge_dev_exitfn(PCIDevice *dev) > +static void pci_basic_bridge_dev_exitfn(PCIDevice *dev) > { > - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); > - if (msi_present(dev)) { > - msi_uninit(dev); > - } > slotid_cap_cleanup(dev); > - shpc_cleanup(dev, &bridge_dev->bar); > pci_bridge_exitfn(dev); > } > > -static void pci_bridge_dev_instance_finalize(Object *obj) > -{ > - shpc_free(PCI_DEVICE(obj)); > -} > - > -static void pci_bridge_dev_write_config(PCIDevice *d, > - uint32_t address, uint32_t val, int len) > -{ > - pci_bridge_write_config(d, address, val, len); > - if (msi_present(d)) { > - msi_write_config(d, address, val, len); > - } > - shpc_cap_write_config(d, address, val, len); > -} > - > -static void qdev_pci_bridge_dev_reset(DeviceState *qdev) > -{ > - PCIDevice *dev = PCI_DEVICE(qdev); > - > - pci_bridge_reset(qdev); > - shpc_reset(dev); > -} > - > -static Property pci_bridge_dev_properties[] = { > +static Property pci_basic_bridge_dev_properties[] = { > /* Note: 0 is not a legal chassis number. */ > - DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0), > - DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_MSI_REQ, true), > + DEFINE_PROP_UINT8("chassis_nr", PCIBasicBridgeDev, chassis_nr, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > -static const VMStateDescription pci_bridge_dev_vmstate = { > - .name = "pci_bridge", > +static const VMStateDescription pci_basic_bridge_dev_vmstate = { > + .name = "pci_basic_bridge", > .fields = (VMStateField[]) { > VMSTATE_PCI_DEVICE(parent_obj, PCIBridge), > - SHPC_VMSTATE(shpc, PCIDevice), > VMSTATE_END_OF_LIST() > } > }; > > -static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) > +static void pci_basic_bridge_dev_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > > - k->init = pci_bridge_dev_initfn; > - k->exit = pci_bridge_dev_exitfn; > - k->config_write = pci_bridge_dev_write_config; > + 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_BRIDGE; > + k->device_id = PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE; > k->class_id = PCI_CLASS_BRIDGE_PCI; > k->is_bridge = 1, > - dc->desc = "Standard PCI Bridge"; > - dc->reset = qdev_pci_bridge_dev_reset; > - dc->props = pci_bridge_dev_properties; > - dc->vmsd = &pci_bridge_dev_vmstate; > + 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); > - hc->plug = shpc_device_hotplug_cb; > - hc->unplug_request = shpc_device_hot_unplug_request_cb; > } > > -static const TypeInfo pci_bridge_dev_info = { > - .name = TYPE_PCI_BRIDGE_DEV, > +static const TypeInfo pci_basic_bridge_dev_info = { > + .name = TYPE_PCI_BASIC_BRIDGE_DEV, > .parent = TYPE_PCI_BRIDGE, > - .instance_size = sizeof(PCIBridgeDev), > - .class_init = pci_bridge_dev_class_init, > - .instance_finalize = pci_bridge_dev_instance_finalize, > - .interfaces = (InterfaceInfo[]) { > - { TYPE_HOTPLUG_HANDLER }, > - { } > - } > + .instance_size = sizeof(PCIBasicBridgeDev), > + .class_init = pci_basic_bridge_dev_class_init, > }; > > -static void pci_bridge_dev_register(void) > +static void pci_basic_bridge_dev_register(void) > { > - type_register_static(&pci_bridge_dev_info); > + type_register_static(&pci_basic_bridge_dev_info); > } > > -type_init(pci_bridge_dev_register); > +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); > > In my opinion this is the cleanest solution. If Michael doesn't like the idea of a new "public" device, we can incorporate it (hide it) into the PXB device, the same I did with the PXB bus type. In this way it will be a PXB 'internal'. What do you think? In the meantime: Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel
On 06/10/15 20:22, Marcel Apfelbaum wrote: > On 06/10/2015 08:07 PM, 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, 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 <marcel@redhat.com> >> Cc: Marcel Apfelbaum <marcel@redhat.com> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> >> Notes: >> 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 > Nice, I'll use that. > > , 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 + >> .../{pci_bridge_dev.c => pci_basic_bridge_dev.c} | 128 >> ++++++--------------- >> hw/pci-bridge/pci_expander_bridge.c | 2 +- >> 4 files changed, 35 insertions(+), 97 deletions(-) >> copy hw/pci-bridge/{pci_bridge_dev.c => pci_basic_bridge_dev.c} (35%) >> >> 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_bridge_dev.c >> b/hw/pci-bridge/pci_basic_bridge_dev.c >> similarity index 35% >> copy from hw/pci-bridge/pci_bridge_dev.c >> copy to hw/pci-bridge/pci_basic_bridge_dev.c >> index 36f73e1..d8bfb6c 100644 >> --- a/hw/pci-bridge/pci_bridge_dev.c >> +++ b/hw/pci-bridge/pci_basic_bridge_dev.c >> @@ -1,7 +1,9 @@ >> /* >> - * Standard PCI Bridge Device >> + * PCI Bridge Device without interrupt and SHPC / hotplug support. >> * >> - * Copyright (c) 2011 Red Hat Inc. Author: Michael S. Tsirkin >> <mst@redhat.com> >> + * Copyright (c) 2011, 2015 Red Hat Inc. >> + * Authors: Michael S. Tsirkin <mst@redhat.com> >> + * Laszlo Ersek <lersek@redhat.com> >> * >> * >> http://www.pcisig.com/specifications/conventional/pci_to_pci_bridge_architecture/ >> >> * >> @@ -21,158 +23,92 @@ >> >> #include "hw/pci/pci_bridge.h" >> #include "hw/pci/pci_ids.h" >> -#include "hw/pci/msi.h" >> -#include "hw/pci/shpc.h" >> #include "hw/pci/slotid_cap.h" >> #include "exec/memory.h" >> #include "hw/pci/pci_bus.h" >> -#include "hw/hotplug.h" >> >> -#define TYPE_PCI_BRIDGE_DEV "pci-bridge" >> -#define PCI_BRIDGE_DEV(obj) \ >> - OBJECT_CHECK(PCIBridgeDev, (obj), TYPE_PCI_BRIDGE_DEV) >> +#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 PCIBridgeDev { >> +struct PCIBasicBridgeDev { >> /*< private >*/ >> PCIBridge parent_obj; >> /*< public >*/ >> >> - MemoryRegion bar; >> uint8_t chassis_nr; >> -#define PCI_BRIDGE_DEV_F_MSI_REQ 0 >> - uint32_t flags; >> }; >> -typedef struct PCIBridgeDev PCIBridgeDev; >> +typedef struct PCIBasicBridgeDev PCIBasicBridgeDev; >> >> -static int pci_bridge_dev_initfn(PCIDevice *dev) >> +static int pci_basic_bridge_dev_initfn(PCIDevice *dev) >> { >> - PCIBridge *br = PCI_BRIDGE(dev); >> - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); >> + PCIBasicBridgeDev *bridge_dev = PCI_BASIC_BRIDGE_DEV(dev); >> int err; >> >> err = pci_bridge_initfn(dev, TYPE_PCI_BUS); >> if (err) { >> goto bridge_error; >> } >> - dev->config[PCI_INTERRUPT_PIN] = 0x1; >> - memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", >> shpc_bar_size(dev)); >> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0); >> - if (err) { >> - goto shpc_error; >> - } >> err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); >> if (err) { >> goto slotid_error; >> } >> - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && >> - msi_supported) { >> - err = msi_init(dev, 0, 1, true, true); >> - if (err < 0) { >> - goto msi_error; >> - } >> - } >> - /* TODO: spec recommends using 64 bit prefetcheable BAR. >> - * Check whether that works well. */ >> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY | >> - PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar); >> return 0; >> -msi_error: >> - slotid_cap_cleanup(dev); >> slotid_error: >> - shpc_cleanup(dev, &bridge_dev->bar); >> -shpc_error: >> pci_bridge_exitfn(dev); >> bridge_error: >> return err; >> } >> >> -static void pci_bridge_dev_exitfn(PCIDevice *dev) >> +static void pci_basic_bridge_dev_exitfn(PCIDevice *dev) >> { >> - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); >> - if (msi_present(dev)) { >> - msi_uninit(dev); >> - } >> slotid_cap_cleanup(dev); >> - shpc_cleanup(dev, &bridge_dev->bar); >> pci_bridge_exitfn(dev); >> } >> >> -static void pci_bridge_dev_instance_finalize(Object *obj) >> -{ >> - shpc_free(PCI_DEVICE(obj)); >> -} >> - >> -static void pci_bridge_dev_write_config(PCIDevice *d, >> - uint32_t address, uint32_t >> val, int len) >> -{ >> - pci_bridge_write_config(d, address, val, len); >> - if (msi_present(d)) { >> - msi_write_config(d, address, val, len); >> - } >> - shpc_cap_write_config(d, address, val, len); >> -} >> - >> -static void qdev_pci_bridge_dev_reset(DeviceState *qdev) >> -{ >> - PCIDevice *dev = PCI_DEVICE(qdev); >> - >> - pci_bridge_reset(qdev); >> - shpc_reset(dev); >> -} >> - >> -static Property pci_bridge_dev_properties[] = { >> +static Property pci_basic_bridge_dev_properties[] = { >> /* Note: 0 is not a legal chassis number. */ >> - DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0), >> - DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, >> PCI_BRIDGE_DEV_F_MSI_REQ, true), >> + DEFINE_PROP_UINT8("chassis_nr", PCIBasicBridgeDev, chassis_nr, 0), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> -static const VMStateDescription pci_bridge_dev_vmstate = { >> - .name = "pci_bridge", >> +static const VMStateDescription pci_basic_bridge_dev_vmstate = { >> + .name = "pci_basic_bridge", >> .fields = (VMStateField[]) { >> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge), >> - SHPC_VMSTATE(shpc, PCIDevice), >> VMSTATE_END_OF_LIST() >> } >> }; >> >> -static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) >> +static void pci_basic_bridge_dev_class_init(ObjectClass *klass, void >> *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); >> >> - k->init = pci_bridge_dev_initfn; >> - k->exit = pci_bridge_dev_exitfn; >> - k->config_write = pci_bridge_dev_write_config; >> + 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_BRIDGE; >> + k->device_id = PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE; >> k->class_id = PCI_CLASS_BRIDGE_PCI; >> k->is_bridge = 1, >> - dc->desc = "Standard PCI Bridge"; >> - dc->reset = qdev_pci_bridge_dev_reset; >> - dc->props = pci_bridge_dev_properties; >> - dc->vmsd = &pci_bridge_dev_vmstate; >> + 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); >> - hc->plug = shpc_device_hotplug_cb; >> - hc->unplug_request = shpc_device_hot_unplug_request_cb; >> } >> >> -static const TypeInfo pci_bridge_dev_info = { >> - .name = TYPE_PCI_BRIDGE_DEV, >> +static const TypeInfo pci_basic_bridge_dev_info = { >> + .name = TYPE_PCI_BASIC_BRIDGE_DEV, >> .parent = TYPE_PCI_BRIDGE, >> - .instance_size = sizeof(PCIBridgeDev), >> - .class_init = pci_bridge_dev_class_init, >> - .instance_finalize = pci_bridge_dev_instance_finalize, >> - .interfaces = (InterfaceInfo[]) { >> - { TYPE_HOTPLUG_HANDLER }, >> - { } >> - } >> + .instance_size = sizeof(PCIBasicBridgeDev), >> + .class_init = pci_basic_bridge_dev_class_init, >> }; >> >> -static void pci_bridge_dev_register(void) >> +static void pci_basic_bridge_dev_register(void) >> { >> - type_register_static(&pci_bridge_dev_info); >> + type_register_static(&pci_basic_bridge_dev_info); >> } >> >> -type_init(pci_bridge_dev_register); >> +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); >> >> > > In my opinion this is the cleanest solution. > If Michael doesn't like the idea of a new "public" device, > we can incorporate it (hide it) into the PXB device, the same > I did with the PXB bus type. In this way it will be a PXB 'internal'. > What do you think? It would work for me, of course. > In the meantime: > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks! NB, I'll mention that I *did* start implementing the "new flag"-based approach, before opting for this avenue. Basically I added a new bit to the "flags" bitmap, and tried to make everything conditional on that. I named the two spots earlier where that idea broke down for me: (1) migration (2) hotplug interface (the shpc_device_hotplug_cb() and shpc_device_hot_unplug_request_cb() callbacks, and the TYPE_HOTPLUG_HANDLER interface). Regarding the hotplug interface, that's problematic because these settings are class-level, not instance level (and the property would be instance level). But, if I understood Michael on IRC correctly, I could simply add different (locally defined) hotplug callbacks that always failed. Probably doable, but it's (a different kind of) overkill IMO -- the current patch is "overkill" because it adds a new device model, while this "advertize hotplug, but always fail it" is overkill IMO exactly because of that. :) Why advertize it if it always fails? Regarding migration, I must not have done a good job describing my concern, in <http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342905>. So let me try again: The flag approach would go like this: in pci_bridge_dev_initfn(), I'd check the new "shpc" bit in the flags bitmask (same as it is done now for "msi"), and it if is set, I'd omit all of the following: - the (dev->config[PCI_INTERRUPT_PIN] = 0x1) assignment - the memory_region_init() call - the shpc_init() call - the msi_init() call - the pci_register_bar() call Straightforward, right? Accordingly, the error paths in the initfn, and pci_bridge_dev_exitfn() would be updated in sync. Etc etc etc. Now, the problem is this: static const VMStateDescription pci_bridge_dev_vmstate = { .name = "pci_bridge", .fields = (VMStateField[]) { VMSTATE_PCI_DEVICE(parent_obj, PCIBridge), SHPC_VMSTATE(shpc, PCIDevice), VMSTATE_END_OF_LIST() } }; I *cannot* make SHPC_VMSTATE() conditional on the new flag. It is always there. I don't know what it does. What if it includes a callback of some kind that *depends* on shpc_init()? If that's the case, then *outgoing* migration with "shpc=off" might crash. Therefore, I'd have to turn this SHPC_VMSTATE() vmstate field into a subsection, and introduce an "is this needed" function for it, which would of course key off of the "shpc" flag. But that would break incoming migration *from* qemu-2.3! That's why in sync with introducing the subsection, I'd have bump *both* "minimum_version_id" and "version_id" in pci_bridge_dev_vmstate. Because this way the v2.3 -> v2.4 migration would be cleanly rejected. Of course, if SHPC_VMSTATE(), as-is, simply decays to a no-op, if shpc_init() is omitted, then migration is not a question at all, under Michael's proposal. But, I don't know if that's the case. ... I like this patch because it eliminates both questions (hotplug and migration) at once. Thanks Laszlo
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_bridge_dev.c b/hw/pci-bridge/pci_basic_bridge_dev.c similarity index 35% copy from hw/pci-bridge/pci_bridge_dev.c copy to hw/pci-bridge/pci_basic_bridge_dev.c index 36f73e1..d8bfb6c 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_basic_bridge_dev.c @@ -1,7 +1,9 @@ /* - * Standard PCI Bridge Device + * PCI Bridge Device without interrupt and SHPC / hotplug support. * - * Copyright (c) 2011 Red Hat Inc. Author: Michael S. Tsirkin <mst@redhat.com> + * Copyright (c) 2011, 2015 Red Hat Inc. + * Authors: Michael S. Tsirkin <mst@redhat.com> + * Laszlo Ersek <lersek@redhat.com> * * http://www.pcisig.com/specifications/conventional/pci_to_pci_bridge_architecture/ * @@ -21,158 +23,92 @@ #include "hw/pci/pci_bridge.h" #include "hw/pci/pci_ids.h" -#include "hw/pci/msi.h" -#include "hw/pci/shpc.h" #include "hw/pci/slotid_cap.h" #include "exec/memory.h" #include "hw/pci/pci_bus.h" -#include "hw/hotplug.h" -#define TYPE_PCI_BRIDGE_DEV "pci-bridge" -#define PCI_BRIDGE_DEV(obj) \ - OBJECT_CHECK(PCIBridgeDev, (obj), TYPE_PCI_BRIDGE_DEV) +#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 PCIBridgeDev { +struct PCIBasicBridgeDev { /*< private >*/ PCIBridge parent_obj; /*< public >*/ - MemoryRegion bar; uint8_t chassis_nr; -#define PCI_BRIDGE_DEV_F_MSI_REQ 0 - uint32_t flags; }; -typedef struct PCIBridgeDev PCIBridgeDev; +typedef struct PCIBasicBridgeDev PCIBasicBridgeDev; -static int pci_bridge_dev_initfn(PCIDevice *dev) +static int pci_basic_bridge_dev_initfn(PCIDevice *dev) { - PCIBridge *br = PCI_BRIDGE(dev); - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); + PCIBasicBridgeDev *bridge_dev = PCI_BASIC_BRIDGE_DEV(dev); int err; err = pci_bridge_initfn(dev, TYPE_PCI_BUS); if (err) { goto bridge_error; } - dev->config[PCI_INTERRUPT_PIN] = 0x1; - memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", shpc_bar_size(dev)); - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0); - if (err) { - goto shpc_error; - } err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); if (err) { goto slotid_error; } - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && - msi_supported) { - err = msi_init(dev, 0, 1, true, true); - if (err < 0) { - goto msi_error; - } - } - /* TODO: spec recommends using 64 bit prefetcheable BAR. - * Check whether that works well. */ - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY | - PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar); return 0; -msi_error: - slotid_cap_cleanup(dev); slotid_error: - shpc_cleanup(dev, &bridge_dev->bar); -shpc_error: pci_bridge_exitfn(dev); bridge_error: return err; } -static void pci_bridge_dev_exitfn(PCIDevice *dev) +static void pci_basic_bridge_dev_exitfn(PCIDevice *dev) { - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); - if (msi_present(dev)) { - msi_uninit(dev); - } slotid_cap_cleanup(dev); - shpc_cleanup(dev, &bridge_dev->bar); pci_bridge_exitfn(dev); } -static void pci_bridge_dev_instance_finalize(Object *obj) -{ - shpc_free(PCI_DEVICE(obj)); -} - -static void pci_bridge_dev_write_config(PCIDevice *d, - uint32_t address, uint32_t val, int len) -{ - pci_bridge_write_config(d, address, val, len); - if (msi_present(d)) { - msi_write_config(d, address, val, len); - } - shpc_cap_write_config(d, address, val, len); -} - -static void qdev_pci_bridge_dev_reset(DeviceState *qdev) -{ - PCIDevice *dev = PCI_DEVICE(qdev); - - pci_bridge_reset(qdev); - shpc_reset(dev); -} - -static Property pci_bridge_dev_properties[] = { +static Property pci_basic_bridge_dev_properties[] = { /* Note: 0 is not a legal chassis number. */ - DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0), - DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_MSI_REQ, true), + DEFINE_PROP_UINT8("chassis_nr", PCIBasicBridgeDev, chassis_nr, 0), DEFINE_PROP_END_OF_LIST(), }; -static const VMStateDescription pci_bridge_dev_vmstate = { - .name = "pci_bridge", +static const VMStateDescription pci_basic_bridge_dev_vmstate = { + .name = "pci_basic_bridge", .fields = (VMStateField[]) { VMSTATE_PCI_DEVICE(parent_obj, PCIBridge), - SHPC_VMSTATE(shpc, PCIDevice), VMSTATE_END_OF_LIST() } }; -static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) +static void pci_basic_bridge_dev_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); - k->init = pci_bridge_dev_initfn; - k->exit = pci_bridge_dev_exitfn; - k->config_write = pci_bridge_dev_write_config; + 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_BRIDGE; + k->device_id = PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE; k->class_id = PCI_CLASS_BRIDGE_PCI; k->is_bridge = 1, - dc->desc = "Standard PCI Bridge"; - dc->reset = qdev_pci_bridge_dev_reset; - dc->props = pci_bridge_dev_properties; - dc->vmsd = &pci_bridge_dev_vmstate; + 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); - hc->plug = shpc_device_hotplug_cb; - hc->unplug_request = shpc_device_hot_unplug_request_cb; } -static const TypeInfo pci_bridge_dev_info = { - .name = TYPE_PCI_BRIDGE_DEV, +static const TypeInfo pci_basic_bridge_dev_info = { + .name = TYPE_PCI_BASIC_BRIDGE_DEV, .parent = TYPE_PCI_BRIDGE, - .instance_size = sizeof(PCIBridgeDev), - .class_init = pci_bridge_dev_class_init, - .instance_finalize = pci_bridge_dev_instance_finalize, - .interfaces = (InterfaceInfo[]) { - { TYPE_HOTPLUG_HANDLER }, - { } - } + .instance_size = sizeof(PCIBasicBridgeDev), + .class_init = pci_basic_bridge_dev_class_init, }; -static void pci_bridge_dev_register(void) +static void pci_basic_bridge_dev_register(void) { - type_register_static(&pci_bridge_dev_info); + type_register_static(&pci_basic_bridge_dev_info); } -type_init(pci_bridge_dev_register); +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);
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 <marcel@redhat.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: 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 + .../{pci_bridge_dev.c => pci_basic_bridge_dev.c} | 128 ++++++--------------- hw/pci-bridge/pci_expander_bridge.c | 2 +- 4 files changed, 35 insertions(+), 97 deletions(-) copy hw/pci-bridge/{pci_bridge_dev.c => pci_basic_bridge_dev.c} (35%)