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

[Marcel Apfelbaum <marcel@redhat.com>]

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