Re: [Qemu-devel] [PATCH 03/32] lsi: use qdev_reset_all

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

Il 23/04/2013 18:13, Paolo Bonzini ha scritto:
> Il 23/04/2013 10:47, Jan Kiszka ha scritto:
>>>>          if (val & LSI_ISTAT0_SRST) {
>>>> -            lsi_soft_reset(s);
>>>> +            qdev_reset_all(&s->dev.qdev);
>> What should ensure that all device resets are performed before the host
>> reset? I'm asking as we just ran into the assert(!s->current) over
>> 1.3.1. Or is there some patch from a later version missing (not seeing
>> anything suspicious for lsi at least)?
> 
> qdev should, but it looks like it uses pre-order instead of post-order.

Something like this series (based on 1.3.0), totally untested.

Paolo

[-- Attachment #2: reset-for-jan.patch --]
[-- Type: text/x-patch, Size: 13873 bytes --]

>From ec9930ec271a29041275284127d58104d58d4264 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 23 Apr 2013 18:40:25 +0200
Subject: [PATCH 0/4] *** SUBJECT HERE ***

*** BLURB HERE ***

Paolo Bonzini (4):
  qdev: add qbus_reset_all
  pci: do not export pci_bus_reset
  qdev: allow both pre- and post-order vists in qdev walking functions
  qdev: switch reset to post-order

 hw/pci.c        | 39 +++++++++++++++++++--------------------
 hw/pci.h        |  1 -
 hw/pci_bridge.c |  2 +-
 hw/qdev-core.h  | 27 ++++++++++++++++++++++-----
 hw/qdev.c       | 52 +++++++++++++++++++++++++++++++++++++++-------------
 5 files changed, 81 insertions(+), 40 deletions(-)

-- 
1.8.2

>From 483bc3eddd7804c1b101982fa6bb8150efcdf448 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Thu, 10 Jan 2013 15:49:07 +0100
Subject: [PATCH 1/4] qdev: add qbus_reset_all

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev-core.h | 12 ++++++++++++
 hw/qdev.c      |  7 ++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index fff7f0f..7dcf836 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -191,6 +191,18 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
 int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
                        qbus_walkerfn *busfn, void *opaque);
 void qdev_reset_all(DeviceState *dev);
+
+/**
+ * @qbus_reset_all:
+ * @bus: Bus to be reset.
+ *
+ * Reset @bus and perform a bus-level ("hard") reset of all devices connected
+ * to it, including recursive processing of all buses below @bus itself.  A
+ * hard reset means that qbus_reset_all will reset all state of the device.
+ * For PCI devices, for example, this will include the base address registers
+ * or configuration space.
+ */
+void qbus_reset_all(BusState *bus);
 void qbus_reset_all_fn(void *opaque);
 
 void qbus_free(BusState *bus);
diff --git a/hw/qdev.c b/hw/qdev.c
index 788b4da..d017782 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -227,10 +227,15 @@ void qdev_reset_all(DeviceState *dev)
     qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
 }
 
+void qbus_reset_all(BusState *bus)
+{
+    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
+}
+
 void qbus_reset_all_fn(void *opaque)
 {
     BusState *bus = opaque;
-    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
+    qbus_reset_all(bus);
 }
 
 /* can be used as ->unplug() callback for the simple cases */
-- 
1.8.2


>From 03a8c5e5fe2ff16b6048b49112b69802779de47b Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 17 Dec 2012 11:02:25 +0100
Subject: [PATCH 2/4] pci: do not export pci_bus_reset

qbus_reset_all can be used instead.  There is no semantic change
because pcibus_reset returns 1 and takes care of the device
tree traversal.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci.c        | 8 ++------
 hw/pci.h        | 1 -
 hw/pci_bridge.c | 2 +-
 3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 97a0cd7..916f83c 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -209,8 +209,9 @@ void pci_device_reset(PCIDevice *dev)
  * Trigger pci bus reset under a given bus.
  * To be called on RST# assert.
  */
-void pci_bus_reset(PCIBus *bus)
+static int pcibus_reset(BusState *qbus)
 {
+    PCIBus *bus = DO_UPCAST(PCIBus, qbus, qbus);
     int i;
 
     for (i = 0; i < bus->nirq; i++) {
@@ -221,11 +222,6 @@ void pci_bus_reset(PCIBus *bus)
             pci_device_reset(bus->devices[i]);
         }
     }
-}
-
-static int pcibus_reset(BusState *qbus)
-{
-    pci_bus_reset(DO_UPCAST(PCIBus, qbus, qbus));
 
     /* topology traverse is done by pci_bus_reset().
        Tell qbus/qdev walker not to traverse the tree */
diff --git a/hw/pci.h b/hw/pci.h
index 4da0c2a..88fd4a3 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -334,7 +334,6 @@ void pci_bus_fire_intx_routing_notifier(PCIBus *bus);
 void pci_device_set_intx_routing_notifier(PCIDevice *dev,
                                           PCIINTxRoutingNotifier notifier);
 void pci_device_reset(PCIDevice *dev);
-void pci_bus_reset(PCIBus *bus);
 
 PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
                         const char *default_devaddr);
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index 4680501..30ed34e 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -234,7 +234,7 @@ void pci_bridge_write_config(PCIDevice *d,
     newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
     if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
         /* Trigger hot reset on 0->1 transition. */
-        pci_bus_reset(&s->sec_bus);
+        qbus_reset_all(&s->sec_bus.qbus);
     }
 }
 
-- 
1.8.2


>From 7d76cfc73da60bd2ea225a7df8b5a1c6e4cfa799 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 17 Dec 2012 10:37:02 +0100
Subject: [PATCH 3/4] qdev: allow both pre- and post-order vists in qdev
 walking functions

Resetting should be done in post-order, not pre-order.  However,
qdev_walk_children and qbus_walk_children do not allow this.  Fix
it by adding two extra arguments to the functions.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-core.h | 13 +++++++++----
 hw/qdev.c      | 45 +++++++++++++++++++++++++++++++++------------
 2 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index 7dcf836..3d81969 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -186,10 +186,15 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
 /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
  *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
  *           0 otherwise. */
-int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque);
-int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque);
+int qbus_walk_children(BusState *bus,
+                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+                       void *opaque);
+int qdev_walk_children(DeviceState *dev,
+                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+                       void *opaque);
+
 void qdev_reset_all(DeviceState *dev);
 
 /**
diff --git a/hw/qdev.c b/hw/qdev.c
index d017782..3090135 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -224,12 +224,12 @@ static int qbus_reset_one(BusState *bus, void *opaque)
 
 void qdev_reset_all(DeviceState *dev)
 {
-    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
+    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
 }
 
 void qbus_reset_all(BusState *bus)
 {
-    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
+    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
 }
 
 void qbus_reset_all_fn(void *opaque)
@@ -339,49 +339,70 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
     return NULL;
 }
 
-int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque)
+int qbus_walk_children(BusState *bus,
+                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+                       void *opaque)
 {
     BusChild *kid;
     int err;
 
-    if (busfn) {
-        err = busfn(bus, opaque);
+    if (pre_busfn) {
+        err = pre_busfn(bus, opaque);
         if (err) {
             return err;
         }
     }
 
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        err = qdev_walk_children(kid->child, devfn, busfn, opaque);
+        err = qdev_walk_children(kid->child,
+                                 pre_devfn, pre_busfn,
+                                 post_devfn, post_busfn, opaque);
         if (err < 0) {
             return err;
         }
     }
 
+    if (post_busfn) {
+        err = post_busfn(bus, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
     return 0;
 }
 
-int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque)
+int qdev_walk_children(DeviceState *dev,
+                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+                       void *opaque)
 {
     BusState *bus;
     int err;
 
-    if (devfn) {
-        err = devfn(dev, opaque);
+    if (pre_devfn) {
+        err = pre_devfn(dev, opaque);
         if (err) {
             return err;
         }
     }
 
     QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-        err = qbus_walk_children(bus, devfn, busfn, opaque);
+        err = qbus_walk_children(bus, pre_devfn, pre_busfn,
+                                 post_devfn, post_busfn, opaque);
         if (err < 0) {
             return err;
         }
     }
 
+    if (post_devfn) {
+        err = post_devfn(dev, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
     return 0;
 }
 
-- 
1.8.2


>From ec9930ec271a29041275284127d58104d58d4264 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 17 Dec 2012 10:50:16 +0100
Subject: [PATCH 4/4] qdev: switch reset to post-order

Post-order is the only sensible direction for the reset signals.
For example, suppose pre-order is used and the parent has some data
structures that cache children state (for example a list of active
requests).  When the reset method is invoked on the parent, these caches
could be in any state.

If post-order is used, on the other hand, these will be in a known state
when the reset method is invoked on the parent.

This change means that it is no longer possible to block the visit of
the devices, so the callback is changed to return void.  This is not
a problem, because PCI was returning 1 exactly in order to achieve the
same ordering that this patch implements.

PCI can then rely on the qdev core having sent a "reset signal" (whatever
that means) to the device, and only do the PCI-specific initialization
with pci_do_device_reset.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci.c       | 33 ++++++++++++++++++---------------
 hw/qdev-core.h |  2 +-
 hw/qdev.c      |  6 +++---
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 916f83c..b195d67 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -45,7 +45,7 @@
 static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *pcibus_get_dev_path(DeviceState *dev);
 static char *pcibus_get_fw_dev_path(DeviceState *dev);
-static int pcibus_reset(BusState *qbus);
+static void pcibus_reset(BusState *qbus);
 
 static Property pci_props[] = {
     DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
@@ -164,16 +164,10 @@ void pci_device_deassert_intx(PCIDevice *dev)
     }
 }
 
-/*
- * This function is called on #RST and FLR.
- * FLR if PCI_EXP_DEVCTL_BCR_FLR is set
- */
-void pci_device_reset(PCIDevice *dev)
+static void pci_do_device_reset(PCIDevice *dev)
 {
     int r;
 
-    qdev_reset_all(&dev->qdev);
-
     dev->irq_state = 0;
     pci_update_irq_status(dev);
     pci_device_deassert_intx(dev);
@@ -206,10 +200,23 @@ void pci_device_reset(PCIDevice *dev)
 }
 
 /*
+ * This function is called on #RST and FLR.
+ * FLR if PCI_EXP_DEVCTL_BCR_FLR is set
+ */
+void pci_device_reset(PCIDevice *dev)
+{
+    int r;
+
+    qdev_reset_all(&dev->qdev);
+    pci_do_device_reset(dev);
+}
+
+/*
  * Trigger pci bus reset under a given bus.
- * To be called on RST# assert.
+ * Called via qbus_reset_all on RST# assert, after the devices
+ * have been reset qdev_reset_all-ed already.
  */
-static int pcibus_reset(BusState *qbus)
+static void pcibus_reset(BusState *qbus)
 {
     PCIBus *bus = DO_UPCAST(PCIBus, qbus, qbus);
     int i;
@@ -219,13 +226,9 @@ static int pcibus_reset(BusState *qbus)
     }
     for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
         if (bus->devices[i]) {
-            pci_device_reset(bus->devices[i]);
+            pci_do_device_reset(bus->devices[i]);
         }
     }
-
-    /* topology traverse is done by pci_bus_reset().
-       Tell qbus/qdev walker not to traverse the tree */
-    return 1;
 }
 
 static void pci_host_bus_register(int domain, PCIBus *bus)
diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index 3d81969..e6197ee 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -95,7 +95,7 @@ struct BusClass {
      * bindings can be found at http://playground.sun.com/1275/bindings/.
      */
     char *(*get_fw_dev_path)(DeviceState *dev);
-    int (*reset)(BusState *bus);
+    void (*reset)(BusState *bus);
 };
 
 typedef struct BusChild {
diff --git a/hw/qdev.c b/hw/qdev.c
index 3090135..a1c1ffa 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -217,19 +217,19 @@ static int qbus_reset_one(BusState *bus, void *opaque)
 {
     BusClass *bc = BUS_GET_CLASS(bus);
     if (bc->reset) {
-        return bc->reset(bus);
+        bc->reset(bus);
     }
     return 0;
 }
 
 void qdev_reset_all(DeviceState *dev)
 {
-    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
+    qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
 }
 
 void qbus_reset_all(BusState *bus)
 {
-    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL);
+    qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
 }
 
 void qbus_reset_all_fn(void *opaque)
-- 
1.8.2