[qemu-xen master] spapr_pci: Robustify support of PCI bridges

[patchbot@xxxxxxx]

commit a4beb5f5d4672400a76cc0551c4f0878e42d921c
Author:     Greg Kurz <groug@xxxxxxxx>
AuthorDate: Thu Jul 9 19:12:47 2020 +0200
Commit:     David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
CommitDate: Mon Jul 20 09:21:39 2020 +1000

    spapr_pci: Robustify support of PCI bridges
    
    Some recent error handling cleanups unveiled issues with our support of
    PCI bridges:
    
    1) QEMU aborts when using non-standard PCI bridge types,
       unveiled by commit 7ef1553dac "spapr_pci: Drop some dead error handling"
    
    $ qemu-system-ppc64 -M pseries -device pcie-pci-bridge
    Unexpected error in object_property_find() at qom/object.c:1240:
    qemu-system-ppc64: -device pcie-pci-bridge: Property '.chassis_nr' not found
    Aborted (core dumped)
    
    This happens because we assume all PCI bridge types to have a "chassis_nr"
    property. This property only exists with the standard PCI bridge type
    "pci-bridge" actually. We could possibly revert 7ef1553dac but it seems
    much simpler to check the presence of "chassis_nr" earlier.
    
    2) QEMU abort if same "chassis_nr" value is used several times,
       unveiled by commit d2623129a7de "qom: Drop parameter @errp of
       object_property_add() & friends"
    
    $ qemu-system-ppc64 -M pseries -device pci-bridge,chassis_nr=1 \
                            -device pci-bridge,chassis_nr=1
    Unexpected error in object_property_try_add() at qom/object.c:1167:
    qemu-system-ppc64: -device pci-bridge,chassis_nr=1: attempt to add 
duplicate property '40000100' to object (type 'container')
    Aborted (core dumped)
    
    This happens because we assume that "chassis_nr" values are unique, but
    nobody enforces that and we end up generating duplicate DRC ids. The PCI
    code doesn't really care for duplicate "chassis_nr" properties since it
    is only used to initialize the "Chassis Number Register" of the bridge,
    with no functional impact on QEMU. So, even if passing the same value
    several times might look weird, it never broke anything before, so
    I guess we don't necessarily want to enforce strict checking in the PCI
    code now.
    
    Workaround both issues in the PAPR code: check that the bridge has a
    unique and non null "chassis_nr" when plugging it into its parent bus.
    
    Fixes: 05929a6c5dfe ("spapr: Don't use bus number for building DRC ids")
    Fixes: 7ef1553dac ("spapr_pci: Drop some dead error handling")
    Fixes: d2623129a7de ("qom: Drop parameter @errp of object_property_add() & 
friends")
    Reported-by: Thomas Huth <thuth@xxxxxxxxxx>
    Signed-off-by: Greg Kurz <groug@xxxxxxxx>
    Message-Id: <159431476748.407044.16711294833569014964.stgit@xxxxxxxxx>
    [dwg: Move check slightly to a better place]
    Signed-off-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
---
 hw/ppc/spapr_pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2a6a48744a..21681215d4 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1480,6 +1480,57 @@ static void spapr_pci_bridge_plug(SpaprPhbState *phb,
     add_drcs(phb, bus);
 }
 
+/* Returns non-zero if the value of "chassis_nr" is already in use */
+static int check_chassis_nr(Object *obj, void *opaque)
+{
+    int new_chassis_nr =
+        object_property_get_uint(opaque, "chassis_nr", &error_abort);
+    int chassis_nr =
+        object_property_get_uint(obj, "chassis_nr", NULL);
+
+    if (!object_dynamic_cast(obj, TYPE_PCI_BRIDGE)) {
+        return 0;
+    }
+
+    /* Skip unsupported bridge types */
+    if (!chassis_nr) {
+        return 0;
+    }
+
+    /* Skip self */
+    if (obj == opaque) {
+        return 0;
+    }
+
+    return chassis_nr == new_chassis_nr;
+}
+
+static bool bridge_has_valid_chassis_nr(Object *bridge, Error **errp)
+{
+    int chassis_nr =
+        object_property_get_uint(bridge, "chassis_nr", NULL);
+
+    /*
+     * slotid_cap_init() already ensures that "chassis_nr" isn't null for
+     * standard PCI bridges, so this really tells if "chassis_nr" is present
+     * or not.
+     */
+    if (!chassis_nr) {
+        error_setg(errp, "PCI Bridge lacks a \"chassis_nr\" property");
+        error_append_hint(errp, "Try -device pci-bridge instead.\n");
+        return false;
+    }
+
+    /* We want unique values for "chassis_nr" */
+    if (object_child_foreach_recursive(object_get_root(), check_chassis_nr,
+                                       bridge)) {
+        error_setg(errp, "Bridge chassis %d already in use", chassis_nr);
+        return false;
+    }
+
+    return true;
+}
+
 static void spapr_pci_plug(HotplugHandler *plug_handler,
                            DeviceState *plugged_dev, Error **errp)
 {
@@ -1508,6 +1559,9 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
     g_assert(drc);
 
     if (pc->is_bridge) {
+        if (!bridge_has_valid_chassis_nr(OBJECT(plugged_dev), errp)) {
+            return;
+        }
         spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
     }
 
--
generated by git-patchbot for /home/xen/git/qemu-xen.git#master