[Qemu-devel] [PATCH v3 0/6] PXB tweaks and fixes

[Qemu-devel] [PATCH v3 0/6] PXB tweaks and fixes

[Laszlo Ersek]

V2 was at: <http://thread.gmane.org/gmane.comp.emulators.qemu/343098>.

Patches #1 through #3 have not been modified, relative to v2.

Patch #4 has been dropped and reimlpemented with an arguably better /
simpler approach. PXE-boot tested with OVMF, using a virtio-net-pci NIC
behind a PXB.

Thanks
Laszlo

Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>

Laszlo Ersek (6):
  i386/acpi-build: more traditional _UID and _HID for PXB root buses
  i386/acpi-build: fix PXB workarounds for unsupported BIOSes
  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 property for SysBusDevice
  hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

 hw/pci-bridge/Makefile.objs                        |   1 +
 include/hw/pci/pci.h                               |   1 +
 include/hw/sysbus.h                                |   8 ++
 hw/core/sysbus.c                                   |  27 +++--
 hw/i386/acpi-build.c                               |  13 +--
 .../{pci_bridge_dev.c => pci_basic_bridge_dev.c}   | 128 ++++++---------------
 hw/pci-bridge/pci_expander_bridge.c                |   9 +-
 7 files changed, 72 insertions(+), 115 deletions(-)
 copy hw/pci-bridge/{pci_bridge_dev.c => pci_basic_bridge_dev.c} (35%)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/6] i386/acpi-build: more traditional _UID and _HID for PXB root buses

[Laszlo Ersek]

The ACPI specification permits the _HID and _UID objects to evaluate to
strings. (See "6.1.5 _HID (Hardware ID)" and "6.1.12 _UID (Unique ID)" in
the ACPI v6.0 spec.)

With regard to related standards, the UEFI specification can also express
a device address composed from string _HID and _UID identifiers, inside
the Expanded ACPI Device Path Node. (See "9.3.3 ACPI Device Path", Table
49, in the UEFI v2.5 spec.)

However, numeric (integer) contents for both _HID and _UID are more
traditional. They are recommended by the UEFI spec for size reasons:

  [...] the ACPI Device Path node is smaller and should be used if
  possible to reduce the size of device paths that may potentially be
  stored in nonvolatile storage [...]

External tools support them better (for example the --acpi_hid and
--acpi_uid options of "efibootmgr" only take numeric identifiers).
Finally, numeric _HID and _UID contents are existing practice in the QEMU
source.

This patch was tested with a Fedora 20 LiveCD and a preexistent Windows
Server 2012 R2 guest. Using "acpidump" and "iasl" in the Fedora guest, we
get, in the SSDT:

> Scope (\_SB)
> {
>   Device (PC04)
>   {
>     Name (_UID, 0x04)  // _UID: Unique ID
>     Name (_HID, EisaId ("PNP0A03") /* PCI Bus */)  // _HID: Hardware ID

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>
---

Notes:
    v2:
    - no changes, added Marcel's R-b

 hw/i386/acpi-build.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 5593e41..52c2591 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -945,9 +945,8 @@ build_ssdt(GArray *table_data, GArray *linker,
 
             scope = aml_scope("\\_SB");
             dev = aml_device("PC%.02X", bus_num);
-            aml_append(dev,
-                       aml_name_decl("_UID", aml_string("PC%.02X", bus_num)));
-            aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03")));
+            aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)));
+            aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
             aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
 
             if (numa_node != NUMA_NODE_UNASSIGNED) {
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/6] i386/acpi-build: fix PXB workarounds for unsupported BIOSes

[Laszlo Ersek]

The patch

  apci: fix PXB behaviour if used with unsupported BIOS

uses the following condition to see if a "PXB mem/IO chunk" has *not* been
configured by the BIOS:

  (!range_base || range_base > range_limit)

When this condition evaluates to true, said patch *omits* the
corresponding entry from the _CRS.

Later on the patch checks for the opposite condition (with the intent of
*adding* entries to the _CRS if the "PXB mem/IO chunks" *have* been
configured). Unfortunately, the condition was negated incorrectly: only
the first ! operator was removed, which led to the nonsensical expression

  (range_base || range_base > range_limit)

leading to bogus entries in the _CRS, and causing BSOD in Windows Server
2012 R2 when it runs on OVMF.

The correct negative of the condition seen at the top is

  (range_base && range_base <= range_limit)

Fix the expressions.

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>
---

Notes:
    v2:
    - no changes, added Marcel's R-b

 hw/i386/acpi-build.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 52c2591..b71e942 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -833,7 +833,7 @@ static Aml *build_crs(PCIHostState *host,
              * Work-around for old bioses
              * that do not support multiple root buses
              */
-            if (range_base || range_base > range_limit) {
+            if (range_base && range_base <= range_limit) {
                 aml_append(crs,
                            aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
                                        AML_POS_DECODE, AML_ENTIRE_RANGE,
@@ -854,7 +854,7 @@ static Aml *build_crs(PCIHostState *host,
              * Work-around for old bioses
              * that do not support multiple root buses
              */
-            if (range_base || range_base > range_limit) {
+            if (range_base && range_base <= range_limit) {
                 aml_append(crs,
                            aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
                                             AML_MAX_FIXED, AML_NON_CACHEABLE,
@@ -865,7 +865,7 @@ static Aml *build_crs(PCIHostState *host,
                                             0,
                                             range_limit - range_base + 1));
                 crs_range_insert(mem_ranges, range_base, range_limit);
-          }
+            }
 
             range_base =
                 pci_bridge_get_base(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
@@ -876,7 +876,7 @@ static Aml *build_crs(PCIHostState *host,
              * Work-around for old bioses
              * that do not support multiple root buses
              */
-            if (range_base || range_base > range_limit) {
+            if (range_base && range_base <= range_limit) {
                 aml_append(crs,
                            aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
                                             AML_MAX_FIXED, AML_NON_CACHEABLE,
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 3/6] 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, 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>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---

Notes:
    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 +
 .../{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);
 
-- 
1.8.3.1

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

[Laszlo Ersek]

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>
---

Notes:
    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..0ebb4e2 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

Re: [Qemu-devel] [PATCH v3 4/6] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf()

[Marcel Apfelbaum]

On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
> 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>
> ---
>
> Notes:
>      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..0ebb4e2 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,
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

[Qemu-devel] [PATCH v3 5/6] hw/core: explicit OFW unit address property for SysBusDevice

[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.

For such devices, delegate a fallback property,
"explicit_ofw_unit_address", to whoever creates the device. The creator
can set the property to any appropriate value.

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>
---

Notes:
    v3:
    - new in v3
    - new approach

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

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index d1f3f00..de41b06 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -55,6 +55,14 @@ struct SysBusDevice {
     } mmio[QDEV_MAX_MMIO];
     int num_pio;
     pio_addr_t pio[QDEV_MAX_PIO];
+
+    /*
+     * Sometimes a SysBusDevice has neither MMIO nor PIO resources, yet it
+     * would like to distinguish itself, in OpenFirmware device paths, from
+     * other instances of the same class on the same sysbus. For that end we
+     * expose this property.
+     */
+    char *explicit_ofw_unit_address;
 };
 
 typedef int FindSysbusDeviceFunc(SysBusDevice *sbdev, void *opaque);
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 0ebb4e2..6e9af1c 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -289,6 +289,10 @@ 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 (s->explicit_ofw_unit_address) {
+        return g_strdup_printf("%s@%s", qdev_fw_name(dev),
+                               s->explicit_ofw_unit_address);
+    }
     return g_strdup(qdev_fw_name(dev));
 }
 
@@ -303,11 +307,18 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
     return get_system_memory();
 }
 
+static Property sysbus_device_properties[] = {
+    DEFINE_PROP_STRING("explicit_ofw_unit_address", SysBusDevice,
+                       explicit_ofw_unit_address),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void sysbus_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
     k->init = sysbus_device_init;
     k->bus_type = TYPE_SYSTEM_BUS;
+    k->props = sysbus_device_properties;
 }
 
 static const TypeInfo sysbus_device_type_info = {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 5/6] hw/core: explicit OFW unit address property for SysBusDevice

[Marcel Apfelbaum]

On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
> 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.
>
> For such devices, delegate a fallback property,
> "explicit_ofw_unit_address", to whoever creates the device. The creator
> can set the property to any appropriate value.
>
> 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>
> ---
>
> Notes:
>      v3:
>      - new in v3
>      - new approach
>
>   include/hw/sysbus.h |  8 ++++++++
>   hw/core/sysbus.c    | 11 +++++++++++
>   2 files changed, 19 insertions(+)
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index d1f3f00..de41b06 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -55,6 +55,14 @@ struct SysBusDevice {
>       } mmio[QDEV_MAX_MMIO];
>       int num_pio;
>       pio_addr_t pio[QDEV_MAX_PIO];
> +
> +    /*
> +     * Sometimes a SysBusDevice has neither MMIO nor PIO resources, yet it
> +     * would like to distinguish itself, in OpenFirmware device paths, from
> +     * other instances of the same class on the same sysbus. For that end we
> +     * expose this property.
> +     */
> +    char *explicit_ofw_unit_address;
>   };
>
>   typedef int FindSysbusDeviceFunc(SysBusDevice *sbdev, void *opaque);
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 0ebb4e2..6e9af1c 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -289,6 +289,10 @@ 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 (s->explicit_ofw_unit_address) {
> +        return g_strdup_printf("%s@%s", qdev_fw_name(dev),
> +                               s->explicit_ofw_unit_address);
> +    }
>       return g_strdup(qdev_fw_name(dev));
>   }
>
> @@ -303,11 +307,18 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
>       return get_system_memory();
>   }
>
> +static Property sysbus_device_properties[] = {
> +    DEFINE_PROP_STRING("explicit_ofw_unit_address", SysBusDevice,
> +                       explicit_ofw_unit_address),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>   static void sysbus_device_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *k = DEVICE_CLASS(klass);
>       k->init = sysbus_device_init;
>       k->bus_type = TYPE_SYSTEM_BUS;
> +    k->props = sysbus_device_properties;
>   }
>
>   static const TypeInfo sysbus_device_type_info = {
>
Better than previous approach.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

[Qemu-devel] [PATCH v3 6/6] hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

[Laszlo Ersek]

The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot
off devices behind the PXB. This happens because the
sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have
enough information to format a unique identifier for the PXB in question,
and consequently the OpenFirmware device path passed down to the guest
firmware in the "bootorder" fw_cfg file is unusable for identifying the
boot device.

For example, the command line fragment

  -device pxb,id=bridge1,bus_nr=4 \
  \
  -netdev user,id=netdev0 \
  -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0

results in the following "bootorder" entry:

  /pci/pci-bridge@0/ethernet@2/ethernet-phy@0

The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and the
resultant OpenFirmware device path is independent of bus_nr=4 -- and
therefore it is useless for identifying the device.

In this patch we change the fw_name device class member of TYPE_PXB_HOST
from "pci" to "pci-root", and set each instance's explicit OFW unit
address to the PXB bus number. The same command line fragment results in
the following OpenFirmware device path in the "bootorder" fw_cfg file:

  /pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0

The original, initial "/pci" fragment has been replaced with
"/pci-root@4", which (a) looks better, (b) provides all the necessary
information.

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>
---

Notes:
    v3:
    - using new sysbus device property, not inserting additional bus

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

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index c7a085d..d468670 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -93,7 +93,7 @@ static void pxb_host_class_init(ObjectClass *class, void *data)
     DeviceClass *dc = DEVICE_CLASS(class);
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
 
-    dc->fw_name = "pci";
+    dc->fw_name = "pci-root";
     hc->root_bus_path = pxb_host_root_bus_path;
 }
 
@@ -152,6 +152,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
 {
     PXBDev *pxb = PXB_DEV(dev);
     DeviceState *ds, *bds;
+    char *bus_nr_str;
     PCIBus *bus;
     const char *dev_name = NULL;
 
@@ -166,6 +167,10 @@ static int pxb_dev_initfn(PCIDevice *dev)
     }
 
     ds = qdev_create(NULL, TYPE_PXB_HOST);
+    bus_nr_str = g_strdup_printf("%x", pxb->bus_nr);
+    qdev_prop_set_string(ds, "explicit_ofw_unit_address", bus_nr_str);
+    g_free(bus_nr_str);
+
     bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
 
     bus->parent_dev = dev;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 6/6] hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

[Marcel Apfelbaum]

On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
> The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot
> off devices behind the PXB. This happens because the
> sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have
> enough information to format a unique identifier for the PXB in question,
> and consequently the OpenFirmware device path passed down to the guest
> firmware in the "bootorder" fw_cfg file is unusable for identifying the
> boot device.
>
> For example, the command line fragment
>
>    -device pxb,id=bridge1,bus_nr=4 \
>    \
>    -netdev user,id=netdev0 \
>    -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0
>
> results in the following "bootorder" entry:
>
>    /pci/pci-bridge@0/ethernet@2/ethernet-phy@0
>
> The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and the
> resultant OpenFirmware device path is independent of bus_nr=4 -- and
> therefore it is useless for identifying the device.
>
> In this patch we change the fw_name device class member of TYPE_PXB_HOST
> from "pci" to "pci-root", and set each instance's explicit OFW unit
> address to the PXB bus number. The same command line fragment results in
> the following OpenFirmware device path in the "bootorder" fw_cfg file:
>
>    /pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
Clean

>
> The original, initial "/pci" fragment has been replaced with
> "/pci-root@4", which (a) looks better, (b) provides all the necessary
> information.
>
> 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>
> ---
>
> Notes:
>      v3:
>      - using new sysbus device property, not inserting additional bus
>
>   hw/pci-bridge/pci_expander_bridge.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index c7a085d..d468670 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -93,7 +93,7 @@ static void pxb_host_class_init(ObjectClass *class, void *data)
>       DeviceClass *dc = DEVICE_CLASS(class);
>       PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>
> -    dc->fw_name = "pci";
> +    dc->fw_name = "pci-root";
>       hc->root_bus_path = pxb_host_root_bus_path;
>   }
>
> @@ -152,6 +152,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
>   {
>       PXBDev *pxb = PXB_DEV(dev);
>       DeviceState *ds, *bds;
> +    char *bus_nr_str;
>       PCIBus *bus;
>       const char *dev_name = NULL;
>
> @@ -166,6 +167,10 @@ static int pxb_dev_initfn(PCIDevice *dev)
>       }
>
>       ds = qdev_create(NULL, TYPE_PXB_HOST);
> +    bus_nr_str = g_strdup_printf("%x", pxb->bus_nr);
> +    qdev_prop_set_string(ds, "explicit_ofw_unit_address", bus_nr_str);
> +    g_free(bus_nr_str);

This is the best approach yet.
I think it will not affect migration, because on the target site
it will have the same bus number, so the property doesn't need to be
passed.

Is my time to see how it will work on Seabios.
Thanks a lot for your help!!

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


> +
>       bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
>
>       bus->parent_dev = dev;
>

Re: [Qemu-devel] [PATCH v3 6/6] hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

[Laszlo Ersek]

On 06/11/15 11:15, Marcel Apfelbaum wrote:
> On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
>> The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot
>> off devices behind the PXB. This happens because the
>> sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have
>> enough information to format a unique identifier for the PXB in question,
>> and consequently the OpenFirmware device path passed down to the guest
>> firmware in the "bootorder" fw_cfg file is unusable for identifying the
>> boot device.
>>
>> For example, the command line fragment
>>
>>    -device pxb,id=bridge1,bus_nr=4 \
>>    \
>>    -netdev user,id=netdev0 \
>>    -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0
>>
>> results in the following "bootorder" entry:
>>
>>    /pci/pci-bridge@0/ethernet@2/ethernet-phy@0
>>
>> The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and the
>> resultant OpenFirmware device path is independent of bus_nr=4 -- and
>> therefore it is useless for identifying the device.
>>
>> In this patch we change the fw_name device class member of TYPE_PXB_HOST
>> from "pci" to "pci-root", and set each instance's explicit OFW unit
>> address to the PXB bus number. The same command line fragment results in
>> the following OpenFirmware device path in the "bootorder" fw_cfg file:
>>
>>    /pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
> Clean
> 
>>
>> The original, initial "/pci" fragment has been replaced with
>> "/pci-root@4", which (a) looks better, (b) provides all the necessary
>> information.
>>
>> 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>
>> ---
>>
>> Notes:
>>      v3:
>>      - using new sysbus device property, not inserting additional bus
>>
>>   hw/pci-bridge/pci_expander_bridge.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c
>> b/hw/pci-bridge/pci_expander_bridge.c
>> index c7a085d..d468670 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -93,7 +93,7 @@ static void pxb_host_class_init(ObjectClass *class,
>> void *data)
>>       DeviceClass *dc = DEVICE_CLASS(class);
>>       PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>>
>> -    dc->fw_name = "pci";
>> +    dc->fw_name = "pci-root";
>>       hc->root_bus_path = pxb_host_root_bus_path;
>>   }
>>
>> @@ -152,6 +152,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>   {
>>       PXBDev *pxb = PXB_DEV(dev);
>>       DeviceState *ds, *bds;
>> +    char *bus_nr_str;
>>       PCIBus *bus;
>>       const char *dev_name = NULL;
>>
>> @@ -166,6 +167,10 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>       }
>>
>>       ds = qdev_create(NULL, TYPE_PXB_HOST);
>> +    bus_nr_str = g_strdup_printf("%x", pxb->bus_nr);
>> +    qdev_prop_set_string(ds, "explicit_ofw_unit_address", bus_nr_str);
>> +    g_free(bus_nr_str);
> 
> This is the best approach yet.
> I think it will not affect migration, because on the target site
> it will have the same bus number, so the property doesn't need to be
> passed.

Yes, I thought the same.

Migration should carry guest-alterable state. If, in some device,
"explicit_ofw_unit_address" will ever be changed due to guest actions
(as opposed to deriving it from a constant on the command line, which
management would sync anyway), then that specific device will have to
migrate the underlying state as it sees fit, and update the property in
a post load callback.

> 
> Is my time to see how it will work on Seabios.
> Thanks a lot for your help!!
> 
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks! I hope to hear back from Michael and Markus as well. Until then,
this should suffice to work on the firmware(s).

Laszlo

>> +
>>       bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
>>
>>       bus->parent_dev = dev;
>>
> 

Re: [Qemu-devel] [PATCH v3 6/6] hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

[Marcel Apfelbaum]

On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
> The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot
> off devices behind the PXB. This happens because the
> sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have
> enough information to format a unique identifier for the PXB in question,
> and consequently the OpenFirmware device path passed down to the guest
> firmware in the "bootorder" fw_cfg file is unusable for identifying the
> boot device.
>
> For example, the command line fragment
>
>    -device pxb,id=bridge1,bus_nr=4 \
>    \
>    -netdev user,id=netdev0 \
>    -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0
>
> results in the following "bootorder" entry:
>
>    /pci/pci-bridge@0/ethernet@2/ethernet-phy@0
>
> The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and the
> resultant OpenFirmware device path is independent of bus_nr=4 -- and
> therefore it is useless for identifying the device.
>
> In this patch we change the fw_name device class member of TYPE_PXB_HOST
> from "pci" to "pci-root", and set each instance's explicit OFW unit
> address to the PXB bus number. The same command line fragment results in
> the following OpenFirmware device path in the "bootorder" fw_cfg file:
>
>    /pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
Hi Laszlo,

I applied your patches but I still get  /pci@i0cf8/ethernet@5/ethernet-phy@0
in the boot list

I checked and  the code enters only once in sysbus_get_fw_dev_path
for pci@i0cf8 and goes for pio branch.

Do you know maybe what I missed?

Thanks,
Marcel


>
> The original, initial "/pci" fragment has been replaced with
> "/pci-root@4", which (a) looks better, (b) provides all the necessary
> information.
>
> 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>
> ---
>
> Notes:
>      v3:
>      - using new sysbus device property, not inserting additional bus
>
>   hw/pci-bridge/pci_expander_bridge.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index c7a085d..d468670 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -93,7 +93,7 @@ static void pxb_host_class_init(ObjectClass *class, void *data)
>       DeviceClass *dc = DEVICE_CLASS(class);
>       PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>
> -    dc->fw_name = "pci";
> +    dc->fw_name = "pci-root";
>       hc->root_bus_path = pxb_host_root_bus_path;
>   }
>
> @@ -152,6 +152,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
>   {
>       PXBDev *pxb = PXB_DEV(dev);
>       DeviceState *ds, *bds;
> +    char *bus_nr_str;
>       PCIBus *bus;
>       const char *dev_name = NULL;
>
> @@ -166,6 +167,10 @@ static int pxb_dev_initfn(PCIDevice *dev)
>       }
>
>       ds = qdev_create(NULL, TYPE_PXB_HOST);
> +    bus_nr_str = g_strdup_printf("%x", pxb->bus_nr);
> +    qdev_prop_set_string(ds, "explicit_ofw_unit_address", bus_nr_str);
> +    g_free(bus_nr_str);
> +
>       bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
>
>       bus->parent_dev = dev;
>

Re: [Qemu-devel] [PATCH v3 6/6] hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

[Laszlo Ersek]

On 06/11/15 12:21, Marcel Apfelbaum wrote:
> On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
>> The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot
>> off devices behind the PXB. This happens because the
>> sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have
>> enough information to format a unique identifier for the PXB in question,
>> and consequently the OpenFirmware device path passed down to the guest
>> firmware in the "bootorder" fw_cfg file is unusable for identifying the
>> boot device.
>>
>> For example, the command line fragment
>>
>>    -device pxb,id=bridge1,bus_nr=4 \
>>    \
>>    -netdev user,id=netdev0 \
>>    -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0
>>
>> results in the following "bootorder" entry:
>>
>>    /pci/pci-bridge@0/ethernet@2/ethernet-phy@0
>>
>> The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and the
>> resultant OpenFirmware device path is independent of bus_nr=4 -- and
>> therefore it is useless for identifying the device.
>>
>> In this patch we change the fw_name device class member of TYPE_PXB_HOST
>> from "pci" to "pci-root", and set each instance's explicit OFW unit
>> address to the PXB bus number. The same command line fragment results in
>> the following OpenFirmware device path in the "bootorder" fw_cfg file:
>>
>>    /pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
> Hi Laszlo,
> 
> I applied your patches but I still get 
> /pci@i0cf8/ethernet@5/ethernet-phy@0
> in the boot list
> 
> I checked and  the code enters only once in sysbus_get_fw_dev_path
> for pci@i0cf8 and goes for pio branch.
> 
> Do you know maybe what I missed?

I think so, yes: you added the ...,bootorder=N property to a device that
is *not* behind a PXB. :) You forgot the ...,bus=bridgeX property.

Thanks
Laszlo

> 
> Thanks,
> Marcel
> 
> 
>>
>> The original, initial "/pci" fragment has been replaced with
>> "/pci-root@4", which (a) looks better, (b) provides all the necessary
>> information.
>>
>> 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>
>> ---
>>
>> Notes:
>>      v3:
>>      - using new sysbus device property, not inserting additional bus
>>
>>   hw/pci-bridge/pci_expander_bridge.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c
>> b/hw/pci-bridge/pci_expander_bridge.c
>> index c7a085d..d468670 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -93,7 +93,7 @@ static void pxb_host_class_init(ObjectClass *class,
>> void *data)
>>       DeviceClass *dc = DEVICE_CLASS(class);
>>       PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>>
>> -    dc->fw_name = "pci";
>> +    dc->fw_name = "pci-root";
>>       hc->root_bus_path = pxb_host_root_bus_path;
>>   }
>>
>> @@ -152,6 +152,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>   {
>>       PXBDev *pxb = PXB_DEV(dev);
>>       DeviceState *ds, *bds;
>> +    char *bus_nr_str;
>>       PCIBus *bus;
>>       const char *dev_name = NULL;
>>
>> @@ -166,6 +167,10 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>       }
>>
>>       ds = qdev_create(NULL, TYPE_PXB_HOST);
>> +    bus_nr_str = g_strdup_printf("%x", pxb->bus_nr);
>> +    qdev_prop_set_string(ds, "explicit_ofw_unit_address", bus_nr_str);
>> +    g_free(bus_nr_str);
>> +
>>       bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
>>
>>       bus->parent_dev = dev;
>>
> 

Re: [Qemu-devel] [PATCH v3 6/6] hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

[Marcel Apfelbaum]

On 06/11/2015 01:26 PM, Laszlo Ersek wrote:
> On 06/11/15 12:21, Marcel Apfelbaum wrote:
>> On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
>>> The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot
>>> off devices behind the PXB. This happens because the
>>> sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have
>>> enough information to format a unique identifier for the PXB in question,
>>> and consequently the OpenFirmware device path passed down to the guest
>>> firmware in the "bootorder" fw_cfg file is unusable for identifying the
>>> boot device.
>>>
>>> For example, the command line fragment
>>>
>>>     -device pxb,id=bridge1,bus_nr=4 \
>>>     \
>>>     -netdev user,id=netdev0 \
>>>     -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0
>>>
>>> results in the following "bootorder" entry:
>>>
>>>     /pci/pci-bridge@0/ethernet@2/ethernet-phy@0
>>>
>>> The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and the
>>> resultant OpenFirmware device path is independent of bus_nr=4 -- and
>>> therefore it is useless for identifying the device.
>>>
>>> In this patch we change the fw_name device class member of TYPE_PXB_HOST
>>> from "pci" to "pci-root", and set each instance's explicit OFW unit
>>> address to the PXB bus number. The same command line fragment results in
>>> the following OpenFirmware device path in the "bootorder" fw_cfg file:
>>>
>>>     /pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
>> Hi Laszlo,
>>
>> I applied your patches but I still get
>> /pci@i0cf8/ethernet@5/ethernet-phy@0
>> in the boot list
>>
>> I checked and  the code enters only once in sysbus_get_fw_dev_path
>> for pci@i0cf8 and goes for pio branch.
>>
>> Do you know maybe what I missed?
>
> I think so, yes: you added the ...,bootorder=N property to a device that
> is *not* behind a PXB. :) You forgot the ...,bus=bridgeX property.

Actually:
-device pxb,id=bridge1,bus_nr=4 -netdev user,id=u \
-device e1000,id=net2,bus=bridge1,netdev=u,addr=0x5,bootindex=0,romfile=../pc-bios/efi-e1000.rom,bus=pci.0

Hmm :(



>
> Thanks
> Laszlo
>
>>
>> Thanks,
>> Marcel
>>
>>
>>>
>>> The original, initial "/pci" fragment has been replaced with
>>> "/pci-root@4", which (a) looks better, (b) provides all the necessary
>>> information.
>>>
>>> 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>
>>> ---
>>>
>>> Notes:
>>>       v3:
>>>       - using new sysbus device property, not inserting additional bus
>>>
>>>    hw/pci-bridge/pci_expander_bridge.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/pci-bridge/pci_expander_bridge.c
>>> b/hw/pci-bridge/pci_expander_bridge.c
>>> index c7a085d..d468670 100644
>>> --- a/hw/pci-bridge/pci_expander_bridge.c
>>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>>> @@ -93,7 +93,7 @@ static void pxb_host_class_init(ObjectClass *class,
>>> void *data)
>>>        DeviceClass *dc = DEVICE_CLASS(class);
>>>        PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>>>
>>> -    dc->fw_name = "pci";
>>> +    dc->fw_name = "pci-root";
>>>        hc->root_bus_path = pxb_host_root_bus_path;
>>>    }
>>>
>>> @@ -152,6 +152,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>>    {
>>>        PXBDev *pxb = PXB_DEV(dev);
>>>        DeviceState *ds, *bds;
>>> +    char *bus_nr_str;
>>>        PCIBus *bus;
>>>        const char *dev_name = NULL;
>>>
>>> @@ -166,6 +167,10 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>>        }
>>>
>>>        ds = qdev_create(NULL, TYPE_PXB_HOST);
>>> +    bus_nr_str = g_strdup_printf("%x", pxb->bus_nr);
>>> +    qdev_prop_set_string(ds, "explicit_ofw_unit_address", bus_nr_str);
>>> +    g_free(bus_nr_str);
>>> +
>>>        bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
>>>
>>>        bus->parent_dev = dev;
>>>
>>
>

Re: [Qemu-devel] [PATCH v3 6/6] hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

[Laszlo Ersek]

On 06/11/15 12:29, Marcel Apfelbaum wrote:
> On 06/11/2015 01:26 PM, Laszlo Ersek wrote:
>> On 06/11/15 12:21, Marcel Apfelbaum wrote:
>>> On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
>>>> The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot
>>>> off devices behind the PXB. This happens because the
>>>> sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have
>>>> enough information to format a unique identifier for the PXB in
>>>> question,
>>>> and consequently the OpenFirmware device path passed down to the guest
>>>> firmware in the "bootorder" fw_cfg file is unusable for identifying the
>>>> boot device.
>>>>
>>>> For example, the command line fragment
>>>>
>>>>     -device pxb,id=bridge1,bus_nr=4 \
>>>>     \
>>>>     -netdev user,id=netdev0 \
>>>>     -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0
>>>>
>>>> results in the following "bootorder" entry:
>>>>
>>>>     /pci/pci-bridge@0/ethernet@2/ethernet-phy@0
>>>>
>>>> The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and
>>>> the
>>>> resultant OpenFirmware device path is independent of bus_nr=4 -- and
>>>> therefore it is useless for identifying the device.
>>>>
>>>> In this patch we change the fw_name device class member of
>>>> TYPE_PXB_HOST
>>>> from "pci" to "pci-root", and set each instance's explicit OFW unit
>>>> address to the PXB bus number. The same command line fragment
>>>> results in
>>>> the following OpenFirmware device path in the "bootorder" fw_cfg file:
>>>>
>>>>     /pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
>>> Hi Laszlo,
>>>
>>> I applied your patches but I still get
>>> /pci@i0cf8/ethernet@5/ethernet-phy@0
>>> in the boot list
>>>
>>> I checked and  the code enters only once in sysbus_get_fw_dev_path
>>> for pci@i0cf8 and goes for pio branch.
>>>
>>> Do you know maybe what I missed?
>>
>> I think so, yes: you added the ...,bootorder=N property to a device that
>> is *not* behind a PXB. :) You forgot the ...,bus=bridgeX property.
> 
> Actually:
> -device pxb,id=bridge1,bus_nr=4 -netdev user,id=u \
> -device
> e1000,id=net2,bus=bridge1,netdev=u,addr=0x5,bootindex=0,romfile=../pc-bios/efi-e1000.rom,bus=pci.0
> 
> 
> Hmm :(

Count the "bus=" substrings on your command line :)

Laszlo

Re: [Qemu-devel] [PATCH v3 6/6] hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

[Marcel Apfelbaum]

On 06/11/2015 01:45 PM, Laszlo Ersek wrote:
> On 06/11/15 12:29, Marcel Apfelbaum wrote:
>> On 06/11/2015 01:26 PM, Laszlo Ersek wrote:
>>> On 06/11/15 12:21, Marcel Apfelbaum wrote:
>>>> On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
>>>>> The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot
>>>>> off devices behind the PXB. This happens because the
>>>>> sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have
>>>>> enough information to format a unique identifier for the PXB in
>>>>> question,
>>>>> and consequently the OpenFirmware device path passed down to the guest
>>>>> firmware in the "bootorder" fw_cfg file is unusable for identifying the
>>>>> boot device.
>>>>>
>>>>> For example, the command line fragment
>>>>>
>>>>>      -device pxb,id=bridge1,bus_nr=4 \
>>>>>      \
>>>>>      -netdev user,id=netdev0 \
>>>>>      -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0
>>>>>
>>>>> results in the following "bootorder" entry:
>>>>>
>>>>>      /pci/pci-bridge@0/ethernet@2/ethernet-phy@0
>>>>>
>>>>> The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and
>>>>> the
>>>>> resultant OpenFirmware device path is independent of bus_nr=4 -- and
>>>>> therefore it is useless for identifying the device.
>>>>>
>>>>> In this patch we change the fw_name device class member of
>>>>> TYPE_PXB_HOST
>>>>> from "pci" to "pci-root", and set each instance's explicit OFW unit
>>>>> address to the PXB bus number. The same command line fragment
>>>>> results in
>>>>> the following OpenFirmware device path in the "bootorder" fw_cfg file:
>>>>>
>>>>>      /pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
>>>> Hi Laszlo,
>>>>
>>>> I applied your patches but I still get
>>>> /pci@i0cf8/ethernet@5/ethernet-phy@0
>>>> in the boot list
>>>>
>>>> I checked and  the code enters only once in sysbus_get_fw_dev_path
>>>> for pci@i0cf8 and goes for pio branch.
>>>>
>>>> Do you know maybe what I missed?
>>>
>>> I think so, yes: you added the ...,bootorder=N property to a device that
>>> is *not* behind a PXB. :) You forgot the ...,bus=bridgeX property.
>>
>> Actually:
>> -device pxb,id=bridge1,bus_nr=4 -netdev user,id=u \
>> -device
>> e1000,id=net2,bus=bridge1,netdev=u,addr=0x5,bootindex=0,romfile=../pc-bios/efi-e1000.rom,bus=pci.0
>>
>>
>> Hmm :(
>
> Count the "bus=" substrings on your command line :)
Wow! Why would I do that to myself? I need a coffee fast!

>
> Laszlo
>