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

[Michael S. Tsirkin]

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 <address@hidden>
> Cc: Marcel Apfelbaum <address@hidden>
> Cc: Michael S. Tsirkin <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Signed-off-by: Laszlo Ersek <address@hidden>
> Reviewed-by: Marcel Apfelbaum <address@hidden>
> Tested-by: Marcel Apfelbaum <address@hidden>
> ---
> 
> 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 <address@hidden>
> + *          Laszlo Ersek <address@hidden>
> + *
> + * 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
>