[Qemu-devel] [PATCH v7 00/10] PXB changes

[Qemu-devel] [PATCH v7 00/10] PXB changes

[Laszlo Ersek]

I tried hard to address the two sets of comments I got for v6:
- in the first half of the series, I reworked the new pci-bridge
  property
- in the second half, I implemented the agreed upon OFW format

I also adapted the OVMF series to the changes in the second half
(locally only, for now), and retested QEMU+OVMF with my Windows Server
2012 r2 guest.

In any case, this is my swan song for QEMU as far as this feature is
concerned.

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

Thanks
Laszlo

Laszlo Ersek (10):
  migration: introduce VMSTATE_BUFFER_UNSAFE_INFO_TEST()
  hw/pci-bridge: expose _test parameter in SHPC_VMSTATE()
  hw/pci-bridge: add macro for "chassis_nr" property
  hw/pci-bridge: add macro for "msi" property
  hw/pci: introduce shpc_present() helper function
  hw/pci-bridge: introduce "shpc" property
  hw/pci-bridge: disable SHPC in 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 special OFW unit address for PXB host

 include/hw/pci/pci_bridge.h         |  4 ++
 include/hw/pci/shpc.h               | 11 ++++-
 include/hw/sysbus.h                 | 17 +++++++
 include/migration/vmstate.h         |  7 ++-
 hw/core/sysbus.c                    | 27 +++++++----
 hw/pci-bridge/pci_bridge_dev.c      | 92 +++++++++++++++++++++++++++++--------
 hw/pci-bridge/pci_expander_bridge.c | 57 ++++++++++++++++++++++-
 7 files changed, 183 insertions(+), 32 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v7 01/10] migration: introduce VMSTATE_BUFFER_UNSAFE_INFO_TEST()

[Laszlo Ersek]

There is no _TEST() variant of VMSTATE_BUFFER_UNSAFE_INFO() yet, but we'll
soon need it. Introduce it and rebase the original
VMSTATE_BUFFER_UNSAFE_INFO() on top.

The parameter order of the new function-like macro follows that of
VMSTATE_SINGLE_TEST(): "_test" is introduced between "_state" and
"_version".

Cc: Juan Quintela <quintela@redhat.com>
Cc: Amit Shah <amit.shah@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v7:
    - no changes
    
    v6:
    - new in v6

 include/migration/vmstate.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 7153b1e..0695d7c 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -500,9 +500,10 @@ extern const VMStateInfo vmstate_info_bitmap;
     .start        = (_start),                                        \
 }
 
-#define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info, _size) { \
+#define VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _state, _test, _version, _info, _size) { \
     .name       = (stringify(_field)),                               \
     .version_id = (_version),                                        \
+    .field_exists = (_test),                                         \
     .size       = (_size),                                           \
     .info       = &(_info),                                          \
     .flags      = VMS_BUFFER,                                        \
@@ -562,6 +563,10 @@ extern const VMStateInfo vmstate_info_bitmap;
     VMSTATE_STRUCT_ARRAY_TEST(_field, _state, _num, NULL, _version,   \
             _vmsd, _type)
 
+#define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info, _size) \
+    VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _state, NULL, _version, _info, \
+            _size)
+
 #define VMSTATE_BOOL_V(_f, _s, _v)                                    \
     VMSTATE_SINGLE(_f, _s, _v, vmstate_info_bool, bool)
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 02/10] hw/pci-bridge: expose _test parameter in SHPC_VMSTATE()

[Laszlo Ersek]

Change the signature of the function-like macro SHPC_VMSTATE(), so that we
can produce and expect this field conditionally in the migration stream,
starting with an upcoming patch.

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

Notes:
    v7:
    - no changes
    
    v6:
    - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
      hotplug-less bridge for PXB" from v5) [Michael]

 include/hw/pci/shpc.h          | 5 +++--
 hw/pci-bridge/pci_bridge_dev.c | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 9bbea39..14015af 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -51,7 +51,8 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                        DeviceState *dev, Error **errp);
 
 extern VMStateInfo shpc_vmstate_info;
-#define SHPC_VMSTATE(_field, _type) \
-    VMSTATE_BUFFER_UNSAFE_INFO(_field, _type, 0, shpc_vmstate_info, 0)
+#define SHPC_VMSTATE(_field, _type,  _test) \
+    VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _type, _test, 0, \
+                                    shpc_vmstate_info, 0)
 
 #endif
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 36f73e1..d697c7b 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -131,7 +131,7 @@ static const VMStateDescription pci_bridge_dev_vmstate = {
     .name = "pci_bridge",
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
-        SHPC_VMSTATE(shpc, PCIDevice),
+        SHPC_VMSTATE(shpc, PCIDevice, NULL),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 03/10] hw/pci-bridge: add macro for "chassis_nr" property

[Laszlo Ersek]

This should help catch property name typos at compile time.

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

Notes:
    v7:
    - new in v7

 include/hw/pci/pci_bridge.h         | 2 ++
 hw/pci-bridge/pci_bridge_dev.c      | 3 ++-
 hw/pci-bridge/pci_expander_bridge.c | 3 ++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 1d8f997..f3ac49f 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -28,6 +28,8 @@
 
 #include "hw/pci/pci.h"
 
+#define PCI_BRIDGE_DEV_PROP_CHASSIS_NR "chassis_nr"
+
 int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
                           uint16_t svid, uint16_t ssid);
 
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index d697c7b..f669ecd 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -122,7 +122,8 @@ static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
 
 static Property pci_bridge_dev_properties[] = {
                     /* Note: 0 is not a legal chassis number. */
-    DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
+    DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr,
+                      0),
     DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_MSI_REQ, true),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index ec2bb45..3d840ef 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -14,6 +14,7 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_host.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/pci/pci_bridge.h"
 #include "hw/i386/pc.h"
 #include "qemu/range.h"
 #include "qemu/error-report.h"
@@ -175,7 +176,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
 
     bds = qdev_create(BUS(bus), "pci-bridge");
     bds->id = dev_name;
-    qdev_prop_set_uint8(bds, "chassis_nr", pxb->bus_nr);
+    qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
 
     PCI_HOST_BRIDGE(ds)->bus = bus;
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 04/10] hw/pci-bridge: add macro for "msi" property

[Laszlo Ersek]

This should help catch property name typos at compile time.

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

Notes:
    v7:
    - new in v7

 include/hw/pci/pci_bridge.h    | 1 +
 hw/pci-bridge/pci_bridge_dev.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index f3ac49f..a438eda 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -29,6 +29,7 @@
 #include "hw/pci/pci.h"
 
 #define PCI_BRIDGE_DEV_PROP_CHASSIS_NR "chassis_nr"
+#define PCI_BRIDGE_DEV_PROP_MSI        "msi"
 
 int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
                           uint16_t svid, uint16_t ssid);
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index f669ecd..6b99f08 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -124,7 +124,8 @@ static Property pci_bridge_dev_properties[] = {
                     /* Note: 0 is not a legal chassis number. */
     DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr,
                       0),
-    DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_MSI_REQ, true),
+    DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags,
+                    PCI_BRIDGE_DEV_F_MSI_REQ, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 05/10] hw/pci: introduce shpc_present() helper function

[Laszlo Ersek]

It follows msi_present() in "include/hw/pci/msi.h".

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

Notes:
    v7:
    - new in v7 [Michael]

 include/hw/pci/shpc.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 14015af..2c871b9 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -6,6 +6,7 @@
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "hw/hotplug.h"
+#include "hw/pci/pci.h"
 
 struct SHPCDevice {
     /* Capability offset in device's config space */
@@ -55,4 +56,9 @@ extern VMStateInfo shpc_vmstate_info;
     VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _type, _test, 0, \
                                     shpc_vmstate_info, 0)
 
+static inline bool shpc_present(const PCIDevice *dev)
+{
+    return dev->cap_present & QEMU_PCI_CAP_SHPC;
+}
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 06/10] hw/pci-bridge: introduce "shpc" property

[Laszlo Ersek]

In the PCI expander bridge, we will want to disable those features of
pci-bridge that relate to SHPC (standard hotplug controller):

- SHPC bar and underlying MemoryRegion
- interrupt (INTx or MSI)
- effective hotplug callbacks
- other SHPC hooks (initialization, cleanup, migration etc)

Introduce a new feature request bit in the PCIBridgeDev.flags field, and
turn off the above if the bit is explicitly cleared.

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

Notes:
    v7:
    - call the new property "shpc" rather than "hotplug" [Michael]
    - use macro for property name [Michael]
    - rename PCI_BRIDGE_DEV_F_HOTPLUG to PCI_BRIDGE_DEV_F_SHPC_REQ [Michael]
    - clear "msi" when "shpc" is clear [Michael]
    - check SHPC presence with shpc_present(), paralleling msi_present()
      [Michael]
    
    v6:
    - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
      hotplug-less bridge for PXB" from v5) [Michael]
    - I tested migration-to-file and migration-from-file, with a single PXB
      behind which a virtio-scsi HBA lived
    - No idea how to test the hotplug callbacks. I tried the device_add and
      device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
      monitor reports no error at all; the callbacks don't seem to be
      reached.
    - Retested with windows 2012; it works

 include/hw/pci/pci_bridge.h    |  1 +
 hw/pci-bridge/pci_bridge_dev.c | 86 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 71 insertions(+), 16 deletions(-)

diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index a438eda..93b621c 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -30,6 +30,7 @@
 
 #define PCI_BRIDGE_DEV_PROP_CHASSIS_NR "chassis_nr"
 #define PCI_BRIDGE_DEV_PROP_MSI        "msi"
+#define PCI_BRIDGE_DEV_PROP_SHPC       "shpc"
 
 int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
                           uint16_t svid, uint16_t ssid);
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 6b99f08..c4c8f77 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -40,6 +40,7 @@ struct PCIBridgeDev {
     MemoryRegion bar;
     uint8_t chassis_nr;
 #define PCI_BRIDGE_DEV_F_MSI_REQ 0
+#define PCI_BRIDGE_DEV_F_SHPC_REQ 1
     uint32_t flags;
 };
 typedef struct PCIBridgeDev PCIBridgeDev;
@@ -54,11 +55,17 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
     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;
+    if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) {
+        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;
+        }
+    } else {
+        /* MSI is not applicable without SHPC */
+        bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
     }
     err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
     if (err) {
@@ -71,15 +78,19 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
             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);
+    if (shpc_present(dev)) {
+        /* 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);
+    if (shpc_present(dev)) {
+        shpc_cleanup(dev, &bridge_dev->bar);
+    }
 shpc_error:
     pci_bridge_exitfn(dev);
 bridge_error:
@@ -93,12 +104,15 @@ static void pci_bridge_dev_exitfn(PCIDevice *dev)
         msi_uninit(dev);
     }
     slotid_cap_cleanup(dev);
-    shpc_cleanup(dev, &bridge_dev->bar);
+    if (shpc_present(dev)) {
+        shpc_cleanup(dev, &bridge_dev->bar);
+    }
     pci_bridge_exitfn(dev);
 }
 
 static void pci_bridge_dev_instance_finalize(Object *obj)
 {
+    /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
     shpc_free(PCI_DEVICE(obj));
 }
 
@@ -109,7 +123,9 @@ static void pci_bridge_dev_write_config(PCIDevice *d,
     if (msi_present(d)) {
         msi_write_config(d, address, val, len);
     }
-    shpc_cap_write_config(d, address, val, len);
+    if (shpc_present(d)) {
+        shpc_cap_write_config(d, address, val, len);
+    }
 }
 
 static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
@@ -117,7 +133,9 @@ static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
     PCIDevice *dev = PCI_DEVICE(qdev);
 
     pci_bridge_reset(qdev);
-    shpc_reset(dev);
+    if (shpc_present(dev)) {
+        shpc_reset(dev);
+    }
 }
 
 static Property pci_bridge_dev_properties[] = {
@@ -126,18 +144,54 @@ static Property pci_bridge_dev_properties[] = {
                       0),
     DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags,
                     PCI_BRIDGE_DEV_F_MSI_REQ, true),
+    DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
+                    PCI_BRIDGE_DEV_F_SHPC_REQ, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static bool pci_device_shpc_present(void *opaque, int version_id)
+{
+    PCIDevice *dev = opaque;
+
+    return shpc_present(dev);
+}
+
 static const VMStateDescription pci_bridge_dev_vmstate = {
     .name = "pci_bridge",
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
-        SHPC_VMSTATE(shpc, PCIDevice, NULL),
+        SHPC_VMSTATE(shpc, PCIDevice, pci_device_shpc_present),
         VMSTATE_END_OF_LIST()
     }
 };
 
+static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
+                                      DeviceState *dev, Error **errp)
+{
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+
+    if (!shpc_present(pci_hotplug_dev)) {
+        error_setg(errp, "standard hotplug controller has been disabled for "
+                   "this %s", TYPE_PCI_BRIDGE_DEV);
+        return;
+    }
+    shpc_device_hotplug_cb(hotplug_dev, dev, errp);
+}
+
+static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                                 DeviceState *dev,
+                                                 Error **errp)
+{
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+
+    if (!shpc_present(pci_hotplug_dev)) {
+        error_setg(errp, "standard hotplug controller has been disabled for "
+                   "this %s", TYPE_PCI_BRIDGE_DEV);
+        return;
+    }
+    shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
+}
+
 static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -156,8 +210,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
     dc->props = pci_bridge_dev_properties;
     dc->vmsd = &pci_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;
+    hc->plug = pci_bridge_dev_hotplug_cb;
+    hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
 }
 
 static const TypeInfo pci_bridge_dev_info = {
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 07/10] hw/pci-bridge: disable SHPC in 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.

The "shpc" property of "pci-bridge", introduced in the previous patches,
allows us to disable the standard hotplug controller cleanly, eliminating
the SHPC bar and the conflict.

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

Notes:
    v7:
    - reference "shpc" property by macro PCI_BRIDGE_DEV_PROP_SHPC
    
    v6:
    - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
      hotplug-less bridge for PXB" from v5) [Michael]

 hw/pci-bridge/pci_expander_bridge.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 3d840ef..70708ef 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -177,6 +177,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
     bds = qdev_create(BUS(bus), "pci-bridge");
     bds->id = dev_name;
     qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
+    qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
 
     PCI_HOST_BRIDGE(ds)->bus = bus;
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 08/10] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf()

[Laszlo Ersek]

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

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

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

Notes:
    v7:
    - no changes
    - already part of a pending pull request:
      http://thread.gmane.org/gmane.comp.emulators.qemu/344941/focus=344948
    
    v6:
    - no changes
    
    v5:
    - separate "%s@" from TARGET_FMT_plx with a space [Markus]
    - copied Markus's note about "no arbitrary length limit" on the retval
      into the commit message
    
    v4:
    - unchanged
    
    v3:
    - new in v3

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

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

[Qemu-devel] [PATCH v7 09/10] hw/core: explicit OFW unit address callback for SysBusDeviceClass

[Laszlo Ersek]

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

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

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

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

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

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

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

[Qemu-devel] [PATCH v7 10/10] hw/pci-bridge: format special OFW unit address for PXB host

[Laszlo Ersek]

We have agreed that OpenFirmware device paths in the "bootorder" fw_cfg
file should follow the pattern

  /pci@i0cf8,%x/...

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

The portion of the unit address that comes before the comma is dynamically
taken from the main host bridge, similarly to sysbus_get_fw_dev_path().

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

Notes:
    v7:
    - implement the format that both Kevin and Michael agreed with. Example:
      /pci@i0cf8,1/pci-bridge@0/scsi@0/channel@0/disk@0,0
    - I updated the OVMF patchset accordingly, but I won't post it until
      this QEMU patch is applied
    - Someone please write the SeaBIOS patch
    
    v6:
    - no changes
    
    v5:
    - constify parameter and local variables of pxb_host_ofw_unit_address(),
      in accord with the previous patch
    
    v4:
    - new in v4

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

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 70708ef..57f8a37 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -43,6 +43,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)
@@ -89,12 +91,45 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
     return bus->bus_path;
 }
 
+static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
+{
+    const PCIHostState *pxb_host;
+    const PCIBus *pxb_bus;
+    const PXBDev *pxb_dev;
+    int position;
+    const DeviceState *pxb_dev_base;
+    const PCIHostState *main_host;
+    const SysBusDevice *main_host_sbd;
+
+    pxb_host = PCI_HOST_BRIDGE(dev);
+    pxb_bus = pxb_host->bus;
+    pxb_dev = PXB_DEV(pxb_bus->parent_dev);
+    position = g_list_index(pxb_dev_list, pxb_dev);
+    assert(position >= 0);
+
+    pxb_dev_base = DEVICE(pxb_dev);
+    main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);
+    main_host_sbd = SYS_BUS_DEVICE(main_host);
+
+    if (main_host_sbd->num_mmio > 0) {
+        return g_strdup_printf(TARGET_FMT_plx ",%x",
+                               main_host_sbd->mmio[0].addr, position + 1);
+    }
+    if (main_host_sbd->num_pio > 0) {
+        return g_strdup_printf("i%04x,%x",
+                               main_host_sbd->pio[0], position + 1);
+    }
+    return NULL;
+}
+
 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";
+    sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
     hc->root_bus_path = pxb_host_root_bus_path;
 }
 
@@ -149,6 +184,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);
@@ -192,9 +236,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),
@@ -208,6 +260,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 v7 10/10] hw/pci-bridge: format special OFW unit address for PXB host

[Michael S. Tsirkin]

On Fri, Jun 19, 2015 at 04:40:17AM +0200, Laszlo Ersek wrote:
> We have agreed that OpenFirmware device paths in the "bootorder" fw_cfg
> file should follow the pattern
> 
>   /pci@i0cf8,%x/...
> 
> for devices that live behind an extra root bus. The extra root bus in
> question is the %x'th among the extra root buses. (In other words, %x
> gives the position of the affected extra root bus relative to the other
> extra root buses, in bus_nr order.) %x starts at 1, and is formatted in
> hex.
> 
> The portion of the unit address that comes before the comma is dynamically
> taken from the main host bridge, similarly to sysbus_get_fw_dev_path().
> 
> Cc: Kevin O'Connor <kevin@koconnor.net>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

So as I said, a global pxb_dev_list is ugly, and we do not want
duplicate data structures - please scan child devices of parent instead.
Functionally correct, so can be a patch on top.


> ---
> 
> Notes:
>     v7:
>     - implement the format that both Kevin and Michael agreed with. Example:
>       /pci@i0cf8,1/pci-bridge@0/scsi@0/channel@0/disk@0,0
>     - I updated the OVMF patchset accordingly, but I won't post it until
>       this QEMU patch is applied
>     - Someone please write the SeaBIOS patch
>     
>     v6:
>     - no changes
>     
>     v5:
>     - constify parameter and local variables of pxb_host_ofw_unit_address(),
>       in accord with the previous patch
>     
>     v4:
>     - new in v4
> 
>  hw/pci-bridge/pci_expander_bridge.c | 53 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index 70708ef..57f8a37 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -43,6 +43,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)
> @@ -89,12 +91,45 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
>      return bus->bus_path;
>  }
>  
> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
> +{
> +    const PCIHostState *pxb_host;
> +    const PCIBus *pxb_bus;
> +    const PXBDev *pxb_dev;
> +    int position;
> +    const DeviceState *pxb_dev_base;
> +    const PCIHostState *main_host;
> +    const SysBusDevice *main_host_sbd;
> +
> +    pxb_host = PCI_HOST_BRIDGE(dev);
> +    pxb_bus = pxb_host->bus;
> +    pxb_dev = PXB_DEV(pxb_bus->parent_dev);
> +    position = g_list_index(pxb_dev_list, pxb_dev);
> +    assert(position >= 0);
> +
> +    pxb_dev_base = DEVICE(pxb_dev);
> +    main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);
> +    main_host_sbd = SYS_BUS_DEVICE(main_host);
> +
> +    if (main_host_sbd->num_mmio > 0) {
> +        return g_strdup_printf(TARGET_FMT_plx ",%x",
> +                               main_host_sbd->mmio[0].addr, position + 1);
> +    }
> +    if (main_host_sbd->num_pio > 0) {
> +        return g_strdup_printf("i%04x,%x",
> +                               main_host_sbd->pio[0], position + 1);
> +    }
> +    return NULL;
> +}
> +
>  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";
> +    sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
>      hc->root_bus_path = pxb_host_root_bus_path;
>  }
>  
> @@ -149,6 +184,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);
> @@ -192,9 +236,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),
> @@ -208,6 +260,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 v7 10/10] hw/pci-bridge: format special OFW unit address for PXB host

[Laszlo Ersek]

On 06/24/15 07:40, Michael S. Tsirkin wrote:
> On Fri, Jun 19, 2015 at 04:40:17AM +0200, Laszlo Ersek wrote:
>> We have agreed that OpenFirmware device paths in the "bootorder" fw_cfg
>> file should follow the pattern
>>
>>   /pci@i0cf8,%x/...
>>
>> for devices that live behind an extra root bus. The extra root bus in
>> question is the %x'th among the extra root buses. (In other words, %x
>> gives the position of the affected extra root bus relative to the other
>> extra root buses, in bus_nr order.) %x starts at 1, and is formatted in
>> hex.
>>
>> The portion of the unit address that comes before the comma is dynamically
>> taken from the main host bridge, similarly to sysbus_get_fw_dev_path().
>>
>> Cc: Kevin O'Connor <kevin@koconnor.net>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> So as I said, a global pxb_dev_list is ugly, and we do not want
> duplicate data structures - please scan child devices of parent instead.
> Functionally correct, so can be a patch on top.

Marcel, can you pick this up please?

Thanks
Laszlo

> 
> 
>> ---
>>
>> Notes:
>>     v7:
>>     - implement the format that both Kevin and Michael agreed with. Example:
>>       /pci@i0cf8,1/pci-bridge@0/scsi@0/channel@0/disk@0,0
>>     - I updated the OVMF patchset accordingly, but I won't post it until
>>       this QEMU patch is applied
>>     - Someone please write the SeaBIOS patch
>>     
>>     v6:
>>     - no changes
>>     
>>     v5:
>>     - constify parameter and local variables of pxb_host_ofw_unit_address(),
>>       in accord with the previous patch
>>     
>>     v4:
>>     - new in v4
>>
>>  hw/pci-bridge/pci_expander_bridge.c | 53 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>> index 70708ef..57f8a37 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -43,6 +43,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)
>> @@ -89,12 +91,45 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
>>      return bus->bus_path;
>>  }
>>  
>> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
>> +{
>> +    const PCIHostState *pxb_host;
>> +    const PCIBus *pxb_bus;
>> +    const PXBDev *pxb_dev;
>> +    int position;
>> +    const DeviceState *pxb_dev_base;
>> +    const PCIHostState *main_host;
>> +    const SysBusDevice *main_host_sbd;
>> +
>> +    pxb_host = PCI_HOST_BRIDGE(dev);
>> +    pxb_bus = pxb_host->bus;
>> +    pxb_dev = PXB_DEV(pxb_bus->parent_dev);
>> +    position = g_list_index(pxb_dev_list, pxb_dev);
>> +    assert(position >= 0);
>> +
>> +    pxb_dev_base = DEVICE(pxb_dev);
>> +    main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);
>> +    main_host_sbd = SYS_BUS_DEVICE(main_host);
>> +
>> +    if (main_host_sbd->num_mmio > 0) {
>> +        return g_strdup_printf(TARGET_FMT_plx ",%x",
>> +                               main_host_sbd->mmio[0].addr, position + 1);
>> +    }
>> +    if (main_host_sbd->num_pio > 0) {
>> +        return g_strdup_printf("i%04x,%x",
>> +                               main_host_sbd->pio[0], position + 1);
>> +    }
>> +    return NULL;
>> +}
>> +
>>  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";
>> +    sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
>>      hc->root_bus_path = pxb_host_root_bus_path;
>>  }
>>  
>> @@ -149,6 +184,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);
>> @@ -192,9 +236,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),
>> @@ -208,6 +260,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 v7 10/10] hw/pci-bridge: format special OFW unit address for PXB host

[Marcel Apfelbaum]

On 06/24/2015 10:01 AM, Laszlo Ersek wrote:
> On 06/24/15 07:40, Michael S. Tsirkin wrote:
>> On Fri, Jun 19, 2015 at 04:40:17AM +0200, Laszlo Ersek wrote:
>>> We have agreed that OpenFirmware device paths in the "bootorder" fw_cfg
>>> file should follow the pattern
>>>
>>>    /pci@i0cf8,%x/...
>>>
>>> for devices that live behind an extra root bus. The extra root bus in
>>> question is the %x'th among the extra root buses. (In other words, %x
>>> gives the position of the affected extra root bus relative to the other
>>> extra root buses, in bus_nr order.) %x starts at 1, and is formatted in
>>> hex.
>>>
>>> The portion of the unit address that comes before the comma is dynamically
>>> taken from the main host bridge, similarly to sysbus_get_fw_dev_path().
>>>
>>> Cc: Kevin O'Connor <kevin@koconnor.net>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>> So as I said, a global pxb_dev_list is ugly, and we do not want
>> duplicate data structures - please scan child devices of parent instead.
>> Functionally correct, so can be a patch on top.
>
> Marcel, can you pick this up please?
Sure thing, thank you for your patches!
I'll take care of this,
Marcel

>
> Thanks
> Laszlo
>
>>
>>
>>> ---
>>>
>>> Notes:
>>>      v7:
>>>      - implement the format that both Kevin and Michael agreed with. Example:
>>>        /pci@i0cf8,1/pci-bridge@0/scsi@0/channel@0/disk@0,0
>>>      - I updated the OVMF patchset accordingly, but I won't post it until
>>>        this QEMU patch is applied
>>>      - Someone please write the SeaBIOS patch
>>>
>>>      v6:
>>>      - no changes
>>>
>>>      v5:
>>>      - constify parameter and local variables of pxb_host_ofw_unit_address(),
>>>        in accord with the previous patch
>>>
>>>      v4:
>>>      - new in v4
>>>
>>>   hw/pci-bridge/pci_expander_bridge.c | 53 +++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 53 insertions(+)
>>>
>>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>>> index 70708ef..57f8a37 100644
>>> --- a/hw/pci-bridge/pci_expander_bridge.c
>>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>>> @@ -43,6 +43,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)
>>> @@ -89,12 +91,45 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
>>>       return bus->bus_path;
>>>   }
>>>
>>> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
>>> +{
>>> +    const PCIHostState *pxb_host;
>>> +    const PCIBus *pxb_bus;
>>> +    const PXBDev *pxb_dev;
>>> +    int position;
>>> +    const DeviceState *pxb_dev_base;
>>> +    const PCIHostState *main_host;
>>> +    const SysBusDevice *main_host_sbd;
>>> +
>>> +    pxb_host = PCI_HOST_BRIDGE(dev);
>>> +    pxb_bus = pxb_host->bus;
>>> +    pxb_dev = PXB_DEV(pxb_bus->parent_dev);
>>> +    position = g_list_index(pxb_dev_list, pxb_dev);
>>> +    assert(position >= 0);
>>> +
>>> +    pxb_dev_base = DEVICE(pxb_dev);
>>> +    main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);
>>> +    main_host_sbd = SYS_BUS_DEVICE(main_host);
>>> +
>>> +    if (main_host_sbd->num_mmio > 0) {
>>> +        return g_strdup_printf(TARGET_FMT_plx ",%x",
>>> +                               main_host_sbd->mmio[0].addr, position + 1);
>>> +    }
>>> +    if (main_host_sbd->num_pio > 0) {
>>> +        return g_strdup_printf("i%04x,%x",
>>> +                               main_host_sbd->pio[0], position + 1);
>>> +    }
>>> +    return NULL;
>>> +}
>>> +
>>>   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";
>>> +    sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
>>>       hc->root_bus_path = pxb_host_root_bus_path;
>>>   }
>>>
>>> @@ -149,6 +184,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);
>>> @@ -192,9 +236,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),
>>> @@ -208,6 +260,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 v7 10/10] hw/pci-bridge: format special OFW unit address for PXB host

[Kevin O'Connor]

On Fri, Jun 19, 2015 at 04:40:17AM +0200, Laszlo Ersek wrote:
> We have agreed that OpenFirmware device paths in the "bootorder" fw_cfg
> file should follow the pattern
> 
>   /pci@i0cf8,%x/...
> 
> for devices that live behind an extra root bus. The extra root bus in
> question is the %x'th among the extra root buses. (In other words, %x
> gives the position of the affected extra root bus relative to the other
> extra root buses, in bus_nr order.) %x starts at 1, and is formatted in
> hex.
> 
> The portion of the unit address that comes before the comma is dynamically
> taken from the main host bridge, similarly to sysbus_get_fw_dev_path().
> 
> Cc: Kevin O'Connor <kevin@koconnor.net>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v7:
>     - implement the format that both Kevin and Michael agreed with. Example:
>       /pci@i0cf8,1/pci-bridge@0/scsi@0/channel@0/disk@0,0
>     - I updated the OVMF patchset accordingly, but I won't post it until
>       this QEMU patch is applied
>     - Someone please write the SeaBIOS patch

The associated SeaBIOS patch is below.

Does anyone have a qemu command line handy to test with the PXB bus?

-Kevin


--- a/src/boot.c
+++ b/src/boot.c
@@ -112,9 +112,9 @@ build_pci_path(char *buf, int max, const char *devname, struct pci_device *pci)
     if (pci->parent) {
         p = build_pci_path(p, max, "pci-bridge", pci->parent);
     } else {
-        if (pci->rootbus)
-            p += snprintf(p, max, "/pci-root@%x", pci->rootbus);
         p += snprintf(p, buf+max-p, "%s", FW_PCI_DOMAIN);
+        if (pci->rootbus)
+            p += snprintf(p, buf+max-p, ",%x", pci->rootbus);
     }
 
     int dev = pci_bdf_to_dev(pci->bdf), fn = pci_bdf_to_fn(pci->bdf);

Re: [Qemu-devel] [PATCH v7 10/10] hw/pci-bridge: format special OFW unit address for PXB host

[Marcel Apfelbaum]

On 06/24/2015 08:11 PM, Kevin O'Connor wrote:
> On Fri, Jun 19, 2015 at 04:40:17AM +0200, Laszlo Ersek wrote:
>> We have agreed that OpenFirmware device paths in the "bootorder" fw_cfg
>> file should follow the pattern
>>
>>    /pci@i0cf8,%x/...
>>
>> for devices that live behind an extra root bus. The extra root bus in
>> question is the %x'th among the extra root buses. (In other words, %x
>> gives the position of the affected extra root bus relative to the other
>> extra root buses, in bus_nr order.) %x starts at 1, and is formatted in
>> hex.
>>
>> The portion of the unit address that comes before the comma is dynamically
>> taken from the main host bridge, similarly to sysbus_get_fw_dev_path().
>>
>> Cc: Kevin O'Connor <kevin@koconnor.net>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>      v7:
>>      - implement the format that both Kevin and Michael agreed with. Example:
>>        /pci@i0cf8,1/pci-bridge@0/scsi@0/channel@0/disk@0,0
>>      - I updated the OVMF patchset accordingly, but I won't post it until
>>        this QEMU patch is applied
>>      - Someone please write the SeaBIOS patch
>
> The associated SeaBIOS patch is below.
>
> Does anyone have a qemu command line handy to test with the PXB bus?
-device pxb,id=bridge1,bus_nr=10 -netdev user,id=u -device e1000,id=net2,bus=bridge1,netdev=u

Let me know if you have any issues with it.

Thanks,
Marcel

>
> -Kevin
>
>
> --- a/src/boot.c
> +++ b/src/boot.c
> @@ -112,9 +112,9 @@ build_pci_path(char *buf, int max, const char *devname, struct pci_device *pci)
>       if (pci->parent) {
>           p = build_pci_path(p, max, "pci-bridge", pci->parent);
>       } else {
> -        if (pci->rootbus)
> -            p += snprintf(p, max, "/pci-root@%x", pci->rootbus);
>           p += snprintf(p, buf+max-p, "%s", FW_PCI_DOMAIN);
> +        if (pci->rootbus)
> +            p += snprintf(p, buf+max-p, ",%x", pci->rootbus);
>       }
>
>       int dev = pci_bdf_to_dev(pci->bdf), fn = pci_bdf_to_fn(pci->bdf);
>