[Qemu-devel] [PATCH v5 0/4] PXB changes

[Qemu-devel] [PATCH v5 0/4] PXB changes

[Laszlo Ersek]

No functional changes whatsoever; addressing commit message, code
comment, and coding style remarks from Markus. Those updates are listed
individually per patch. I added / preserved all Tested-by and
Reviewed-by tags I got for v4.

Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Kevin O'Connor <kevin@koconnor.net>

Thanks
Laszlo

Laszlo Ersek (4):
  hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB
  hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf()
  hw/core: explicit OFW unit address callback for SysBusDeviceClass
  hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB

 hw/pci-bridge/Makefile.objs          |   1 +
 include/hw/pci/pci.h                 |   1 +
 include/hw/sysbus.h                  |  17 +++++
 hw/core/sysbus.c                     |  27 +++++---
 hw/pci-bridge/pci_basic_bridge_dev.c | 124 +++++++++++++++++++++++++++++++++++
 hw/pci-bridge/pci_expander_bridge.c  |  41 +++++++++++-
 6 files changed, 199 insertions(+), 12 deletions(-)
 create mode 100644 hw/pci-bridge/pci_basic_bridge_dev.c

-- 
1.8.3.1

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

[Laszlo Ersek]

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.

Suggested-by: Marcel Apfelbaum <marcel@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
---

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 <mst@redhat.com>
+ *          Laszlo Ersek <lersek@redhat.com>
+ *
+ * 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

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 <marcel@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> 
> 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 <mst@redhat.com>
> + *          Laszlo Ersek <lersek@redhat.com>
> + *
> + * 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
> 

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

[Laszlo Ersek]

On 06/17/15 08:24, Michael S. Tsirkin wrote:
> 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.

The field_exists() callback is not documented in "docs/migration.txt",
and I can see no actual use in the tree -- there are some device models
that spell out the name of this callback field, but they all set it to
NULL. Anyway it's great that this callback exists, because (apparently?)
it allows all fields to become optional. I just wish it had been
mentioned earlier :)

Hm, in fact, looking at the *_TEST() macros in
"include/migration/vmstate.h", I think I should:
- introduce VMSTATE_BUFFER_POINTER_UNSAFE_TEST()
- rebase VMSTATE_BUFFER_POINTER_UNSAFE() on top
- rebase SHPC_VMSTATE() to VMSTATE_BUFFER_POINTER_UNSAFE_TEST()

(I note that *_TEST() is also not documented in docs/migration.txt.)

I'll seek to submit a v6 with this change today.

Thanks
Laszlo

> 
> 
>> Suggested-by: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>
>> 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 <mst@redhat.com>
>> + *          Laszlo Ersek <lersek@redhat.com>
>> + *
>> + * 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
>>

[Qemu-devel] [PATCH v5 2/4] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf()

[Laszlo Ersek]

This is done mainly for improving readability, and in preparation for the
next patch, but Markus pointed out another bonus for the string being
returned:

"No arbitrary length limit. Before the patch, it's 39 characters, and the
code breaks catastrophically when qdev_fw_name() is longer: the second
snprintf() is called with its first argument pointing beyond path[], and
its second argument underflowing to a huge size."

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---

Notes:
    v5:
    - separate "%s@" from TARGET_FMT_plx with a space [Markus]
    - copied Markus's note about "no arbitrary length limit" on the retval
      into the commit message
    
    v4:
    - unchanged
    
    v3:
    - new in v3

 hw/core/sysbus.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index b53c351..92eced9 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -281,19 +281,15 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
 static char *sysbus_get_fw_dev_path(DeviceState *dev)
 {
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
-    char path[40];
-    int off;
-
-    off = snprintf(path, sizeof(path), "%s", qdev_fw_name(dev));
 
     if (s->num_mmio) {
-        snprintf(path + off, sizeof(path) - off, "@"TARGET_FMT_plx,
-                 s->mmio[0].addr);
-    } else if (s->num_pio) {
-        snprintf(path + off, sizeof(path) - off, "@i%04x", s->pio[0]);
+        return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
+                               s->mmio[0].addr);
     }
-
-    return g_strdup(path);
+    if (s->num_pio) {
+        return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
+    }
+    return g_strdup(qdev_fw_name(dev));
 }
 
 void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 3/4] hw/core: explicit OFW unit address callback for SysBusDeviceClass

[Laszlo Ersek]

The sysbus_get_fw_dev_path() function formats OpenFirmware device path
nodes ("driver-name@unit-address") for sysbus devices. The first choice
for "unit-address" is the base address of the device's first MMIO region.
The second choice is its first IO port.

However, if two sysbus devices with the same "driver-name" lack both MMIO
and PIO resources, then there is no good way to distinguish them based on
their OFW nodes, because in this case unit-address is omitted completely
for both devices. An example is TYPE_PXB_HOST ("pxb-host").

For the sake of such devices, introduce the explicit_ofw_unit_address()
"virtual member function". With this function, each sysbus device in the
same SysBusDeviceClass can state its own address.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---

Notes:
    v5:
    - mention "pxb-host" as an example device in the commit message [Markus]
    - reword documentation on explicit_ofw_unit_address(), specifying
      headline, preconditions / goal, side effects, retval, errors
      separately. [Markus]
    - constify parameter of explicit_ofw_unit_address(), after focusing on
      side effects [Markus]
    - move declaration of "addr" and "fw_dev_path" to the top level block in
      sysbus_get_fw_dev_path() [Markus]
    - no functional changes, add / keep Markus's R-b
    
    v4:
    - Yet another approach. Instead of allowing the creator of the device to
      set a string property statically, introduce a class level callback.
    
    v3:
    - new in v3
    - new approach

 include/hw/sysbus.h | 17 +++++++++++++++++
 hw/core/sysbus.c    | 11 +++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index d1f3f00..34f93c3 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -41,6 +41,23 @@ typedef struct SysBusDeviceClass {
     /*< public >*/
 
     int (*init)(SysBusDevice *dev);
+
+    /*
+     * Let the sysbus device format its own non-PIO, non-MMIO unit address.
+     *
+     * Sometimes a class of SysBusDevices has neither MMIO nor PIO resources,
+     * yet instances of it would like to distinguish themselves, in
+     * OpenFirmware device paths, from other instances of the same class on the
+     * sysbus. For that end we expose this callback.
+     *
+     * The implementation is not supposed to change *@dev, or incur other
+     * observable change.
+     *
+     * The function returns a dynamically allocated string. On error, NULL
+     * should be returned; the unit address portion of the OFW node will be
+     * omitted then. (This is not considered a fatal error.)
+     */
+    char *(*explicit_ofw_unit_address)(const SysBusDevice *dev);
 } SysBusDeviceClass;
 
 struct SysBusDevice {
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 92eced9..278a2d1 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -281,6 +281,9 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
 static char *sysbus_get_fw_dev_path(DeviceState *dev)
 {
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
+    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(s);
+    /* for the explicit unit address fallback case: */
+    char *addr, *fw_dev_path;
 
     if (s->num_mmio) {
         return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
@@ -289,6 +292,14 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
     if (s->num_pio) {
         return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
     }
+    if (sbc->explicit_ofw_unit_address) {
+        addr = sbc->explicit_ofw_unit_address(s);
+        if (addr) {
+            fw_dev_path = g_strdup_printf("%s@%s", qdev_fw_name(dev), addr);
+            g_free(addr);
+            return fw_dev_path;
+        }
+    }
     return g_strdup(qdev_fw_name(dev));
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 4/4] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB

[Laszlo Ersek]

SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
to follow the pattern

  /pci-root@N/pci@i0cf8/...

for devices that live behind an extra root bus. The extra root bus in
question is the N'th among the extra root bridges. (In other words, N
gives the position of the affected extra root bus relative to the other
extra root buses, in bus_nr order.) N starts at 1, and is formatted in
hex.

The "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro
FW_PCI_DOMAIN).

Cc: Kevin O'Connor <kevin@koconnor.net>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---

Notes:
    v5:
    - constify parameter and local variables of pxb_host_ofw_unit_address(),
      in accord with the previous patch
    
    v4:
    - new in v4

 hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index c7a085d..ee249b7 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -42,6 +42,8 @@ typedef struct PXBDev {
     uint16_t numa_node;
 } PXBDev;
 
+static GList *pxb_dev_list;
+
 #define TYPE_PXB_HOST "pxb-host"
 
 static int pxb_bus_num(PCIBus *bus)
@@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
     return bus->bus_path;
 }
 
+static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
+{
+    const PCIHostState *host = PCI_HOST_BRIDGE(dev);
+    const PCIBus *bus;
+    const PXBDev *pxb;
+    int position;
+
+    bus = host->bus;
+    pxb = PXB_DEV(bus->parent_dev);
+    position = g_list_index(pxb_dev_list, pxb);
+    assert(position >= 0);
+
+    return g_strdup_printf("%x/pci@i0cf8", position + 1);
+}
+
 static void pxb_host_class_init(ObjectClass *class, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(class);
+    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
 
-    dc->fw_name = "pci";
+    dc->fw_name = "pci-root";
+    sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
     hc->root_bus_path = pxb_host_root_bus_path;
 }
 
@@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
     return pin - PCI_SLOT(pxb->devfn);
 }
 
+static gint pxb_compare(gconstpointer a, gconstpointer b)
+{
+    const PXBDev *pxb_a = a, *pxb_b = b;
+
+    return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
+           pxb_a->bus_nr > pxb_b->bus_nr ?  1 :
+           0;
+}
+
 static int pxb_dev_initfn(PCIDevice *dev)
 {
     PXBDev *pxb = PXB_DEV(dev);
@@ -190,9 +218,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
                                PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
     pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
 
+    pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
     return 0;
 }
 
+static void pxb_dev_exitfn(PCIDevice *pci_dev)
+{
+    PXBDev *pxb = PXB_DEV(pci_dev);
+
+    pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
+}
+
 static Property pxb_dev_properties[] = {
     /* Note: 0 is not a legal a PXB bus number. */
     DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
@@ -206,6 +242,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = pxb_dev_initfn;
+    k->exit = pxb_dev_exitfn;
     k->vendor_id = PCI_VENDOR_ID_REDHAT;
     k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
     k->class_id = PCI_CLASS_BRIDGE_HOST;
-- 
1.8.3.1