From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44977) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2lJD-0002TX-GF for qemu-devel@nongnu.org; Wed, 10 Jun 2015 15:05:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2lJ7-00019K-Jo for qemu-devel@nongnu.org; Wed, 10 Jun 2015 15:05:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60209) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2lJ7-00017o-9b for qemu-devel@nongnu.org; Wed, 10 Jun 2015 15:05:01 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 5EA5B8E7A9 for ; Wed, 10 Jun 2015 19:05:00 +0000 (UTC) Message-ID: <55788A57.4080209@redhat.com> Date: Wed, 10 Jun 2015 21:04:55 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1433956033-11584-1-git-send-email-lersek@redhat.com> <1433956033-11584-4-git-send-email-lersek@redhat.com> <55788052.5020408@redhat.com> In-Reply-To: <55788052.5020408@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/4] hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum , qemu-devel@nongnu.org Cc: "Michael S. Tsirkin" 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 >> Cc: Marcel Apfelbaum >> Cc: Michael S. Tsirkin >> Signed-off-by: Laszlo Ersek >> --- >> >> 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 >> >> + * Copyright (c) 2011, 2015 Red Hat Inc. >> + * Authors: Michael S. Tsirkin >> + * Laszlo Ersek >> * >> * >> 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 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 . 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