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

[Laszlo Ersek]

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 <address@hidden>
>> Cc: Marcel Apfelbaum <address@hidden>
>> Cc: Michael S. Tsirkin <address@hidden>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>
>> 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
>> <address@hidden>
>> + * Copyright (c) 2011, 2015 Red Hat Inc.
>> + * Authors: Michael S. Tsirkin <address@hidden>
>> + *          Laszlo Ersek <address@hidden>
>>    *
>>    *
>> 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 <address@hidden>

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