[Qemu-devel] PXB changes for QEMU, and extra root buses for OVMF, round 2

[Qemu-devel] PXB changes for QEMU, and extra root buses for OVMF, round 2

[Laszlo Ersek]

Following up on this cross-posted message, I will send two patch sets,
one for QEMU (to qemu-devel) and another for OVMF (to edk2-devel). With
both in place, OVMF supports multiple PCI root buses, and SeaBIOS
recognizes boot options that reference devices behind PXBs.

* Background.

Since the last such "bundled" posting, which was

  http://thread.gmane.org/gmane.comp.emulators.qemu/342206
  PXB fixes for QEMU, and extra root buses for OVMF

with subthreads

  [edk2] [PATCH 00/21] OvmfPkg: support extra PCI root buses

  [qemu-devel] [PATCH 0/4] PXB tweaks and fixes

I have posted no updated OVMF series, and posted two updated QEMU series:

  http://thread.gmane.org/gmane.comp.emulators.qemu/343098
  [qemu-devel] [PATCH v2 0/4] PXB tweaks and fixes

  http://thread.gmane.org/gmane.comp.emulators.qemu/343150
  [qemu-devel] [PATCH v3 0/6] PXB tweaks and fixes

Version 3 of the QEMU series looked okay-ish for a while. Michael
applied the first two of those patches, and (I believe) was planning to
investigate the 3rd patch ("hw/pci-bridge: create interrupt-less,
hotplug-less bridge for PXB"). Plus, I requested Markus's feedback on
some "core" QOM stuff.

Other than that, everything seemed to work fine between QEMU and OVMF.
In the topmost reference I had named a boot option matching problem (as
point (7)) and I was writing code for that in the then-upcoming OVMF
patches (planned for OVMF v2). In parallel Marcel turned his attention
to SeaBIOS, in order to address the same boot order issue in it. Marcel
posted:

  http://thread.gmane.org/gmane.comp.emulators.qemu/343284
  [SeaBIOS] [PATCH] pci: fixes to allow booting from extra root pci
                    buses.

  http://thread.gmane.org/gmane.comp.emulators.qemu/343298
  [SeaBIOS] [PATCH V2] pci: fixes to allow booting from extra root pci
                       buses.

In the resultant discussion with Kevin, it became clear that minimally
some special casing for QEMU would be necessary in the SeaBIOS patch
(because SeaBIOS was already handling extra PCI root buses, and
differently from how we had expected). Sticking with the existing code
was preferred however.

The main issue was that SeaBIOS expected the *serial numbers* of the
extra PCI root buses in the boot order OFW devpaths, while QEMU was
offering the *bus numbers* themselves. Although the bus numbers in
question are permanent (and not guest-assigned) on QEMU, this wasn't a
good match for SeaBIOS: on physical hardware, Coreboot assigns the extra
root bus numbers (before launching SeaBIOS), and SeaBIOS considers only
the serial numbers stable (as long as the hardware config itself is stable).

 Here's an example:

  serial number      bus number
  of extra root bus  of extra root bus
  SeaBIOS :)         SeaBIOS :(
  -----------------  -----------------
                0x1                0x7
                0x2                0xB
                0x3                0xF

The serial numbers always start at 1, are consecutive, and they order
the extra root buses based on the bus numbers, increasingly.

* New stuff.

The two series I'm about to post (QEMU v4 and OVMF v2), supersede *all*
of the above. No changes are necessary for SeaBIOS. QEMU applies a
transformation from the right column to the left column when formatting
the OFW nodes. SeaBIOS consumes those nodes directly, while OVMF applies
an inverse transform for boot order matching.

(

Internally, OVMF sticks with the right column in the UID fields of the
ACPI_HID_DEVICE_PATH nodes (also known as "PciRoot(UID)" nodes) that it
creates alongside the extra PCI root bridge IO protocols, for the extra
root buses.

This equality is important because those ACPI_HID_DEVICE_PATH.UID fields
must in turn match, by requirement of the UEFI spec, the UIDs placed
into the SSDT by QEMU's ACPI generator.

Therefore, I had to choose between picking the PciRoot(UID) values from
the left column -- which would have necessitated parallel changes to
QEMU's ACPI generator -- vs. implementing a reverse transform in OVMF's
boot order matching. I chose the latter.

)

Summary:
- no ACPI changes in QEMU
- no SeaBIOS changes
- re-verified interrupt line assignments between SeaBIOS and OVMF: they
  continue to match
- retested Windows Server 2012 R2 boot & networking on OVMF (with NIC
  behind PXB)
- regression-tested the "extra root bus"-less case on OVMF
- booted a Fedora CD-ROM on a virtio-scsi HBA on a PXB with SeaBIOS
  (with -boot strict=on)

Thanks
Laszlo

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

[Laszlo Ersek]

Patches 1 and 2 are carried forward from the last (v3) posting without
any changes. Patch 1 is now formatted non-differentially (ie. without
copy detection).

Patches 3 and 4 are new.

Thanks
Laszlo

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>

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                  |   9 +++
 hw/core/sysbus.c                     |  27 ++++++---
 hw/pci-bridge/pci_basic_bridge_dev.c | 114 +++++++++++++++++++++++++++++++++++
 hw/pci-bridge/pci_expander_bridge.c  |  41 ++++++++++++-
 6 files changed, 182 insertions(+), 11 deletions(-)
 create mode 100644 hw/pci-bridge/pci_basic_bridge_dev.c

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 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, 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:
    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 | 114 +++++++++++++++++++++++++++++++++++
 hw/pci-bridge/pci_expander_bridge.c  |   2 +-
 4 files changed, 117 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..d8bfb6c
--- /dev/null
+++ b/hw/pci-bridge/pci_basic_bridge_dev.c
@@ -0,0 +1,114 @@
+/*
+ * 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>
+ *
+ * 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 v4 2/4] 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:
    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..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

[Qemu-devel] [PATCH v4 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.

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

Notes:
    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 |  9 +++++++++
 hw/core/sysbus.c    | 13 +++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index d1f3f00..63b036b 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -41,6 +41,15 @@ typedef struct SysBusDeviceClass {
     /*< public >*/
 
     int (*init)(SysBusDevice *dev);
+
+    /*
+     * 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
+     * same sysbus. For that end we expose this callback. It returns a
+     * dynamically allocated string.
+     */
+    char *(*explicit_ofw_unit_address)(SysBusDevice *dev);
 } SysBusDeviceClass;
 
 struct SysBusDevice {
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 0ebb4e2..a0ec814 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -281,6 +281,7 @@ 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);
 
     if (s->num_mmio) {
         return g_strdup_printf("%s@"TARGET_FMT_plx, qdev_fw_name(dev),
@@ -289,6 +290,18 @@ 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) {
+        char *addr;
+
+        addr = sbc->explicit_ofw_unit_address(s);
+        if (addr) {
+            char *fw_dev_path;
+
+            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 v4 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>
---

Notes:
    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..bed8ec9 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(SysBusDevice *dev)
+{
+    PCIHostState *host = PCI_HOST_BRIDGE(dev);
+    PCIBus *bus;
+    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

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

[Marcel Apfelbaum]

On 06/13/2015 04:52 PM, Laszlo Ersek wrote:
> 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>
> ---
>
> Notes:
>      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..bed8ec9 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(SysBusDevice *dev)
> +{
> +    PCIHostState *host = PCI_HOST_BRIDGE(dev);
> +    PCIBus *bus;
> +    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;
> +}
return pxb_a->bus_nr - pxb_b->bus_nr ? :)

This will not hold the series... I can send a patch on top.


Thanks,
Marcel

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

Re: [Qemu-devel] PXB changes for QEMU, and extra root buses for OVMF, round 2

[Marcel Apfelbaum]

On 06/13/2015 04:39 PM, Laszlo Ersek wrote:
> Following up on this cross-posted message, I will send two patch sets,
> one for QEMU (to qemu-devel) and another for OVMF (to edk2-devel). With
> both in place, OVMF supports multiple PCI root buses, and SeaBIOS
> recognizes boot options that reference devices behind PXBs.
>
> * Background.
>
> Since the last such "bundled" posting, which was
>
>    http://thread.gmane.org/gmane.comp.emulators.qemu/342206
>    PXB fixes for QEMU, and extra root buses for OVMF
>
> with subthreads
>
>    [edk2] [PATCH 00/21] OvmfPkg: support extra PCI root buses
>
>    [qemu-devel] [PATCH 0/4] PXB tweaks and fixes
>
> I have posted no updated OVMF series, and posted two updated QEMU series:
>
>    http://thread.gmane.org/gmane.comp.emulators.qemu/343098
>    [qemu-devel] [PATCH v2 0/4] PXB tweaks and fixes
>
>    http://thread.gmane.org/gmane.comp.emulators.qemu/343150
>    [qemu-devel] [PATCH v3 0/6] PXB tweaks and fixes
>
> Version 3 of the QEMU series looked okay-ish for a while. Michael
> applied the first two of those patches, and (I believe) was planning to
> investigate the 3rd patch ("hw/pci-bridge: create interrupt-less,
> hotplug-less bridge for PXB"). Plus, I requested Markus's feedback on
> some "core" QOM stuff.
>
> Other than that, everything seemed to work fine between QEMU and OVMF.
> In the topmost reference I had named a boot option matching problem (as
> point (7)) and I was writing code for that in the then-upcoming OVMF
> patches (planned for OVMF v2). In parallel Marcel turned his attention
> to SeaBIOS, in order to address the same boot order issue in it. Marcel
> posted:
>
>    http://thread.gmane.org/gmane.comp.emulators.qemu/343284
>    [SeaBIOS] [PATCH] pci: fixes to allow booting from extra root pci
>                      buses.
>
>    http://thread.gmane.org/gmane.comp.emulators.qemu/343298
>    [SeaBIOS] [PATCH V2] pci: fixes to allow booting from extra root pci
>                         buses.
>
> In the resultant discussion with Kevin, it became clear that minimally
> some special casing for QEMU would be necessary in the SeaBIOS patch
> (because SeaBIOS was already handling extra PCI root buses, and
> differently from how we had expected). Sticking with the existing code
> was preferred however.
>
> The main issue was that SeaBIOS expected the *serial numbers* of the
> extra PCI root buses in the boot order OFW devpaths, while QEMU was
> offering the *bus numbers* themselves. Although the bus numbers in
> question are permanent (and not guest-assigned) on QEMU, this wasn't a
> good match for SeaBIOS: on physical hardware, Coreboot assigns the extra
> root bus numbers (before launching SeaBIOS), and SeaBIOS considers only
> the serial numbers stable (as long as the hardware config itself is stable).
>
>   Here's an example:
>
>    serial number      bus number
>    of extra root bus  of extra root bus
>    SeaBIOS :)         SeaBIOS :(
>    -----------------  -----------------
>                  0x1                0x7
>                  0x2                0xB
>                  0x3                0xF
>
> The serial numbers always start at 1, are consecutive, and they order
> the extra root buses based on the bus numbers, increasingly.
>
> * New stuff.
>
> The two series I'm about to post (QEMU v4 and OVMF v2), supersede *all*
> of the above. No changes are necessary for SeaBIOS. QEMU applies a
> transformation from the right column to the left column when formatting
> the OFW nodes. SeaBIOS consumes those nodes directly, while OVMF applies
> an inverse transform for boot order matching.
>
> (
>
> Internally, OVMF sticks with the right column in the UID fields of the
> ACPI_HID_DEVICE_PATH nodes (also known as "PciRoot(UID)" nodes) that it
> creates alongside the extra PCI root bridge IO protocols, for the extra
> root buses.
>
> This equality is important because those ACPI_HID_DEVICE_PATH.UID fields
> must in turn match, by requirement of the UEFI spec, the UIDs placed
> into the SSDT by QEMU's ACPI generator.
>
> Therefore, I had to choose between picking the PciRoot(UID) values from
> the left column -- which would have necessitated parallel changes to
> QEMU's ACPI generator -- vs. implementing a reverse transform in OVMF's
> boot order matching. I chose the latter.
>
> )
>
> Summary:
> - no ACPI changes in QEMU
> - no SeaBIOS changes
> - re-verified interrupt line assignments between SeaBIOS and OVMF: they
>    continue to match
> - retested Windows Server 2012 R2 boot & networking on OVMF (with NIC
>    behind PXB)
> - regression-tested the "extra root bus"-less case on OVMF
> - booted a Fedora CD-ROM on a virtio-scsi HBA on a PXB with SeaBIOS
>    (with -boot strict=on)
>
> Thanks
> Laszlo
>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Tested booting with 1 device on main root bus, and 2 devices behind PXBs.
Changed the boot-order to test all the combinations and verified that
PXE is loaded from the right devices.

Thank you Laszlo for your effort to get this done so quickly.
Now let's wait for Michael/Markus opinion.
Marcel

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

[Laszlo Ersek]

On 06/14/15 12:08, Marcel Apfelbaum wrote:
> On 06/13/2015 04:52 PM, Laszlo Ersek wrote:
>> 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>
>> ---
>>
>> Notes:
>>      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..bed8ec9 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(SysBusDevice *dev)
>> +{
>> +    PCIHostState *host = PCI_HOST_BRIDGE(dev);
>> +    PCIBus *bus;
>> +    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;
>> +}
> return pxb_a->bus_nr - pxb_b->bus_nr ? :)

In this specific case, it would work, yes. The reason is that
PXBDev.bus_nr has type "uint8_t" (== unsigned char), which is promoted
(by the /integer promotions/) to "int", since on all the platforms that
we care about, "an int can represent all values of the original type"
(ie. those of unsigned char).

The upshot is that you'd have

  val1_int - val2_int /* performed in "int" */
  retval_int := difference

where val1_int is in the range [0, 255] and val2_int is in the range [0,
255], hence their difference is in the range [-255, 255], which can be
represented by an int on all platforms that we care about.

So, yes, *in this specific case* what you propose would be safe.

In general, however, I always use this pattern in comparator functions,
because it is safe
- against undefined behavior in integer expressions
  (example: (INT_MAX - (-1)) is undefined behavior),
- and against implementation defined behavior in integer assignments
  (example: return (int)(1u - 2u) is implementation defined)

> This will not hold the series... I can send a patch on top.

I'd prefer if you didn't :)

I didn't write the expression this way because I like the ternary
operator that much (aside: I do like it very much, but that's not the
cause here); it was a calculated decision instead. See above.

... When you proposed the subtraction, did you first consider the
/integer promotions/, the /usual arithmetic conversions/, and the range
of the difference (ie. [-255, 255])? :)

Thanks
Laszlo

> Thanks,
> Marcel
> 
>> +
>>   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;
>>
> 

Re: [Qemu-devel] PXB changes for QEMU, and extra root buses for OVMF, round 2

[Laszlo Ersek]

On 06/14/15 12:09, Marcel Apfelbaum wrote:
> On 06/13/2015 04:39 PM, Laszlo Ersek wrote:
>> Following up on this cross-posted message, I will send two patch sets,
>> one for QEMU (to qemu-devel) and another for OVMF (to edk2-devel). With
>> both in place, OVMF supports multiple PCI root buses, and SeaBIOS
>> recognizes boot options that reference devices behind PXBs.
>>
>> * Background.
>>
>> Since the last such "bundled" posting, which was
>>
>>    http://thread.gmane.org/gmane.comp.emulators.qemu/342206
>>    PXB fixes for QEMU, and extra root buses for OVMF
>>
>> with subthreads
>>
>>    [edk2] [PATCH 00/21] OvmfPkg: support extra PCI root buses
>>
>>    [qemu-devel] [PATCH 0/4] PXB tweaks and fixes
>>
>> I have posted no updated OVMF series, and posted two updated QEMU series:
>>
>>    http://thread.gmane.org/gmane.comp.emulators.qemu/343098
>>    [qemu-devel] [PATCH v2 0/4] PXB tweaks and fixes
>>
>>    http://thread.gmane.org/gmane.comp.emulators.qemu/343150
>>    [qemu-devel] [PATCH v3 0/6] PXB tweaks and fixes
>>
>> Version 3 of the QEMU series looked okay-ish for a while. Michael
>> applied the first two of those patches, and (I believe) was planning to
>> investigate the 3rd patch ("hw/pci-bridge: create interrupt-less,
>> hotplug-less bridge for PXB"). Plus, I requested Markus's feedback on
>> some "core" QOM stuff.
>>
>> Other than that, everything seemed to work fine between QEMU and OVMF.
>> In the topmost reference I had named a boot option matching problem (as
>> point (7)) and I was writing code for that in the then-upcoming OVMF
>> patches (planned for OVMF v2). In parallel Marcel turned his attention
>> to SeaBIOS, in order to address the same boot order issue in it. Marcel
>> posted:
>>
>>    http://thread.gmane.org/gmane.comp.emulators.qemu/343284
>>    [SeaBIOS] [PATCH] pci: fixes to allow booting from extra root pci
>>                      buses.
>>
>>    http://thread.gmane.org/gmane.comp.emulators.qemu/343298
>>    [SeaBIOS] [PATCH V2] pci: fixes to allow booting from extra root pci
>>                         buses.
>>
>> In the resultant discussion with Kevin, it became clear that minimally
>> some special casing for QEMU would be necessary in the SeaBIOS patch
>> (because SeaBIOS was already handling extra PCI root buses, and
>> differently from how we had expected). Sticking with the existing code
>> was preferred however.
>>
>> The main issue was that SeaBIOS expected the *serial numbers* of the
>> extra PCI root buses in the boot order OFW devpaths, while QEMU was
>> offering the *bus numbers* themselves. Although the bus numbers in
>> question are permanent (and not guest-assigned) on QEMU, this wasn't a
>> good match for SeaBIOS: on physical hardware, Coreboot assigns the extra
>> root bus numbers (before launching SeaBIOS), and SeaBIOS considers only
>> the serial numbers stable (as long as the hardware config itself is
>> stable).
>>
>>   Here's an example:
>>
>>    serial number      bus number
>>    of extra root bus  of extra root bus
>>    SeaBIOS :)         SeaBIOS :(
>>    -----------------  -----------------
>>                  0x1                0x7
>>                  0x2                0xB
>>                  0x3                0xF
>>
>> The serial numbers always start at 1, are consecutive, and they order
>> the extra root buses based on the bus numbers, increasingly.
>>
>> * New stuff.
>>
>> The two series I'm about to post (QEMU v4 and OVMF v2), supersede *all*
>> of the above. No changes are necessary for SeaBIOS. QEMU applies a
>> transformation from the right column to the left column when formatting
>> the OFW nodes. SeaBIOS consumes those nodes directly, while OVMF applies
>> an inverse transform for boot order matching.
>>
>> (
>>
>> Internally, OVMF sticks with the right column in the UID fields of the
>> ACPI_HID_DEVICE_PATH nodes (also known as "PciRoot(UID)" nodes) that it
>> creates alongside the extra PCI root bridge IO protocols, for the extra
>> root buses.
>>
>> This equality is important because those ACPI_HID_DEVICE_PATH.UID fields
>> must in turn match, by requirement of the UEFI spec, the UIDs placed
>> into the SSDT by QEMU's ACPI generator.
>>
>> Therefore, I had to choose between picking the PciRoot(UID) values from
>> the left column -- which would have necessitated parallel changes to
>> QEMU's ACPI generator -- vs. implementing a reverse transform in OVMF's
>> boot order matching. I chose the latter.
>>
>> )
>>
>> Summary:
>> - no ACPI changes in QEMU
>> - no SeaBIOS changes
>> - re-verified interrupt line assignments between SeaBIOS and OVMF: they
>>    continue to match
>> - retested Windows Server 2012 R2 boot & networking on OVMF (with NIC
>>    behind PXB)
>> - regression-tested the "extra root bus"-less case on OVMF
>> - booted a Fedora CD-ROM on a virtio-scsi HBA on a PXB with SeaBIOS
>>    (with -boot strict=on)
>>
>> Thanks
>> Laszlo
>>
> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> 
> Tested booting with 1 device on main root bus, and 2 devices behind PXBs.
> Changed the boot-order to test all the combinations and verified that
> PXE is loaded from the right devices.

Awesome, thanks! :)

> Thank you Laszlo for your effort to get this done so quickly.
> Now let's wait for Michael/Markus opinion.

Yes, let's. :)

Cheers
Laszlo

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

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

Suggest to have the commit message state explicitly how
"pci-basic-bridge" is related to "pci-bridge".  If I understand it
correctly, it's a stripped-down copy.  A comment in the code might be in
order, too.

We'd normally avoid copying, and instead have the same code implement
both variants of the device.  I figure your reason for copying is that
the "copy" implementing "pci-basic-bridge" is so small and simple that
avoiding it wouldn't make things simpler or more maintainable.  Suggest
to put that rationale into the commit message as well.

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> 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:
>     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..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),

I'd put a space between "%s@" and TARGET_FMT_plx.

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

Would be nice if we didn't have to triplicate qdev_fw_name(), but I
don't have better ideas.

Bonus: no arbitrary length limit.  Before your 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.
Textbook example of how not to use snprintf().  Worth mentioning in the
commit message?

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

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

Got an example for such a device?  Mind adding it to the commit message?

> 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>
> ---
>
> Notes:
>     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 |  9 +++++++++
>  hw/core/sysbus.c    | 13 +++++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index d1f3f00..63b036b 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -41,6 +41,15 @@ typedef struct SysBusDeviceClass {
>      /*< public >*/
>  
>      int (*init)(SysBusDevice *dev);
> +
> +    /*
> +     * 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
> +     * same sysbus. For that end we expose this callback. It returns a
> +     * dynamically allocated string.
> +     */
> +    char *(*explicit_ofw_unit_address)(SysBusDevice *dev);

I prefer function comments to follow a strict pattern:

    /*
     * Headline explaining the function's purpose[*]
     * Zero or more paragraphs explaining preconditions, side effects,
     * return values, error conditions.
     */

[*] If you can't come up with a headline fitting into a single line,
chances are the function does too many things.

>  } SysBusDeviceClass;
>  
>  struct SysBusDevice {
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 0ebb4e2..a0ec814 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -281,6 +281,7 @@ 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);
>  
>      if (s->num_mmio) {
>          return g_strdup_printf("%s@"TARGET_FMT_plx, qdev_fw_name(dev),
> @@ -289,6 +290,18 @@ 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) {
> +        char *addr;
> +
> +        addr = sbc->explicit_ofw_unit_address(s);
> +        if (addr) {
> +            char *fw_dev_path;
> +
> +            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));
>  }

In short functions like this one, I prefer to have declarations out of
the way in one place rather than cluttering inner blocks.  Matter of
taste, so

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Laszlo Ersek]

On 06/15/15 16:10, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> 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>
> 
> Suggest to have the commit message state explicitly how
> "pci-basic-bridge" is related to "pci-bridge".  If I understand it
> correctly, it's a stripped-down copy.  A comment in the code might be in
> order, too.
> 
> We'd normally avoid copying, and instead have the same code implement
> both variants of the device.  I figure your reason for copying is that
> the "copy" implementing "pci-basic-bridge" is so small and simple that
> avoiding it wouldn't make things simpler or more maintainable.  Suggest
> to put that rationale into the commit message as well.

That reason is a valid one, yes, but it's not the main reason.

The main reason is that I cannot implement this specific flavor of the
device, in the original device model, without (potentially) crashing
outgoing migration.

The reason is that this stripped down version (regardless of *how* it is
implemented) will never initialize the "shpc" pointer in the grandparent
PCIDevice object (-> "grandparent" used in the class hierarchy sense) --
as it will never call shpc_init().

Leaving "PCIDevice.shpc" at NULL is not a problem at all per se. Many
(practically: all) other devices derived from PCIDevice do *not* call
shpc_init() either, and they are all fine.

However, the vmstate definition for the original PciBridgeDev -- that
is, "pci_bridge_dev_vmstate" -- tries to migrate "PCIDevice.shpc"
*unconditionally*. (Ie. it is not a subsection.)

Meaning, if I add a property-controlled variant to PciBridgeDev that
leaves "PCIDevice.shpc" at NULL, then outgoing migration will
dereference that null pointer, in:

  SHPC_VMSTATE()
    VMSTATE_BUFFER_UNSAFE_INFO(... shpc_vmstate_info ...)
      shpc_save()
        qemu_put_buffer(f, d->shpc->config, SHPC_SIZEOF(d));

Therefore I'm forced to detach this new device model completely -- I
have to eliminate that unconditional SHPC_VMSTATE() too.

I could rework the original device model to use a property, *and*
out-migrate the shpc field as an optional *subsection*. That would not
crash. However, if I changed the original device model like this, then
it could not *accept* a migration stream from earlier QEMU versions,
because they would not send the SHPC as a subsection; they'd send it as
a fixed field.

I agree that this should be documented in the commit message; I'll write
it up in the next version.

Thanks
Laszlo

Re: [Qemu-devel] PXB changes for QEMU, and extra root buses for OVMF, round 2

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 06/14/15 12:09, Marcel Apfelbaum wrote:
[...]
>> Thank you Laszlo for your effort to get this done so quickly.
>> Now let's wait for Michael/Markus opinion.
>
> Yes, let's. :)

I'm fine with PATCH 2+3.  Haven't looked closely at the other two.
Anything else you need from me?

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

[Laszlo Ersek]

On 06/15/15 16:23, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> 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:
>>     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..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),
> 
> I'd put a space between "%s@" and TARGET_FMT_plx.

I tried to copy the original format string very carefully, but you do
have a point. I'll do it in the next version.

> 
>> +                               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,
> 
> Would be nice if we didn't have to triplicate qdev_fw_name(), but I
> don't have better ideas.

Right, I could "cache it" in a local variable, but then I'd have to
triplicate the variable reference. And, runtime-wise, qdev_fw_name() is
called only once, it's just spelled out thrice in the source.

> 
> Bonus: no arbitrary length limit.  Before your 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.
> Textbook example of how not to use snprintf().  Worth mentioning in the
> commit message?

I was afraid that bashing on old code would only earn me some ire (not
from you, mind you -- in general), so I didn't mention this fact in the
commit message on purpose. :)

I will, in the next version.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks!
Laszlo

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

[Laszlo Ersek]

On 06/15/15 16:33, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> 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.
> 
> Got an example for such a device?  Mind adding it to the commit message?

That's the right next patch in the series (on which I didn't Cc you,
apologies). If you'd like I can hint at the next patch / the device in
question (PXB) in the commit message.

> 
>> 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>
>> ---
>>
>> Notes:
>>     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 |  9 +++++++++
>>  hw/core/sysbus.c    | 13 +++++++++++++
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>> index d1f3f00..63b036b 100644
>> --- a/include/hw/sysbus.h
>> +++ b/include/hw/sysbus.h
>> @@ -41,6 +41,15 @@ typedef struct SysBusDeviceClass {
>>      /*< public >*/
>>  
>>      int (*init)(SysBusDevice *dev);
>> +
>> +    /*
>> +     * 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
>> +     * same sysbus. For that end we expose this callback. It returns a
>> +     * dynamically allocated string.
>> +     */
>> +    char *(*explicit_ofw_unit_address)(SysBusDevice *dev);
> 
> I prefer function comments to follow a strict pattern:
> 
>     /*
>      * Headline explaining the function's purpose[*]
>      * Zero or more paragraphs explaining preconditions, side effects,
>      * return values, error conditions.
>      */

I follow a very similar requirement in all my edk2 code closely -- but
in edk2 that's actually a *requirement*. :) I wasn't aware of any such
requirement in QEMU, and I thought "any function comment will be seen as
a bonus". :)

I'll rewrite the comment like this, thanks.

> [*] If you can't come up with a headline fitting into a single line,
> chances are the function does too many things.

"Delegate formatting of non-IO, non-MMIO address of sysbus device, due
to bus not knowing."

> 
>>  } SysBusDeviceClass;
>>  
>>  struct SysBusDevice {
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index 0ebb4e2..a0ec814 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -281,6 +281,7 @@ 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);
>>  
>>      if (s->num_mmio) {
>>          return g_strdup_printf("%s@"TARGET_FMT_plx, qdev_fw_name(dev),
>> @@ -289,6 +290,18 @@ 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) {
>> +        char *addr;
>> +
>> +        addr = sbc->explicit_ofw_unit_address(s);
>> +        if (addr) {
>> +            char *fw_dev_path;
>> +
>> +            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));
>>  }
> 
> In short functions like this one, I prefer to have declarations out of
> the way in one place rather than cluttering inner blocks.

Will do.

> Matter of
> taste, so
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Awesome! :) Thank you!
Laszlo

Re: [Qemu-devel] PXB changes for QEMU, and extra root buses for OVMF, round 2

[Laszlo Ersek]

On 06/15/15 16:35, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> On 06/14/15 12:09, Marcel Apfelbaum wrote:
> [...]
>>> Thank you Laszlo for your effort to get this done so quickly.
>>> Now let's wait for Michael/Markus opinion.
>>
>> Yes, let's. :)
> 
> I'm fine with PATCH 2+3.  Haven't looked closely at the other two.
> Anything else you need from me?

I'll implement the (non-functional) changes you pointed out (keeping
your R-b's); please just skim the next version of these two patches to
see if I get those changes right.

Much appreciated!
Laszlo