From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35926) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2kdn-0003iC-GU for qemu-devel@nongnu.org; Wed, 10 Jun 2015 14:22:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2kdi-0003zb-7e for qemu-devel@nongnu.org; Wed, 10 Jun 2015 14:22:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57546) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2kdh-0003zB-Uj for qemu-devel@nongnu.org; Wed, 10 Jun 2015 14:22:14 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 63F5436B4D6 for ; Wed, 10 Jun 2015 18:22:13 +0000 (UTC) Message-ID: <55788052.5020408@redhat.com> Date: Wed, 10 Jun 2015 21:22:10 +0300 From: Marcel Apfelbaum MIME-Version: 1.0 References: <1433956033-11584-1-git-send-email-lersek@redhat.com> <1433956033-11584-4-git-send-email-lersek@redhat.com> In-Reply-To: <1433956033-11584-4-git-send-email-lersek@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed 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: Laszlo Ersek , qemu-devel@nongnu.org Cc: "Michael S. Tsirkin" 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? In the meantime: Reviewed-by: Marcel Apfelbaum Thanks, Marcel