[Qemu-devel] [PATCH 1.5 0/3] qdev: switch reset to post-order

[Qemu-devel] [PATCH 1.5 0/3] qdev: switch reset to post-order

[Paolo Bonzini]

I was going to keep these for 1.6, but since they have been tested
by the user and myself, and they fix a regression, it can be worthwhile
to include these in 1.5.

qdev reset (qdev_reset_all/qbus_reset_all) is currently done in
pre-order.  This is not right, because it means the parent devices
cannot expect anything about the children devices' state during the
reset callback.

With the traversal done *before* invoking the bus-reset callback,
there is no way for the bus-reset callback to take care of the device
tree traversal.  This is currently done only for PCI, which requires
some adjustments.

This fixes a crash in resetting the LSI SCSI adapter, as reported and
tested by Claudio Bley.

Paolo Bonzini (3):
  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/core/qdev.c         | 47 ++++++++++++++++++++++++++++++++++-------------
 hw/pci/pci.c           | 37 +++++++++++++++++--------------------
 hw/pci/pci_bridge.c    |  2 +-
 include/hw/pci/pci.h   |  1 -
 include/hw/qdev-core.h | 15 ++++++++++-----
 5 files changed, 62 insertions(+), 40 deletions(-)

-- 
1.8.2

[Qemu-devel] [PATCH 1.5 1/3] pci: do not export pci_bus_reset

[Paolo Bonzini]

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.

This will be necessary once the traversal is done always in
qbus_reset_all *before* invoking pcibus_reset itself.

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d5257ed..8e9b983 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -214,8 +214,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++) {
@@ -226,11 +227,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/pci_bridge.c b/hw/pci/pci_bridge.c
index 24be6c5..6be116b 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -264,7 +264,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);
     }
 }
 
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 8d075ab..b994c73 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -377,7 +377,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);
-- 
1.8.2

[Qemu-devel] [PATCH 1.5 2/3] qdev: allow both pre- and post-order vists in qdev walking functions

[Paolo Bonzini]

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.

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

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4eb0134..280b641 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -239,12 +239,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)
@@ -342,49 +342,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;
 }
 
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index cf83d54..6f13254 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -240,10 +240,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);
 
 /**
-- 
1.8.2

[Qemu-devel] [PATCH 1.5 3/3] qdev: switch reset to post-order

[Paolo Bonzini]

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 the new function pci_do_device_reset, extracted
from pci_device_reset.  There is no change in the operation of FLR,
which used and still uses pci_device_reset.

Tested-by: Claudio Bley <cbley@av-test.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/qdev.c         |  6 +++---
 hw/pci/pci.c           | 31 ++++++++++++++++---------------
 include/hw/qdev-core.h |  2 +-
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 280b641..67db212 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -232,19 +232,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)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8e9b983..746862a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/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),
@@ -169,16 +169,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);
@@ -211,10 +205,21 @@ 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)
+{
+    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;
@@ -224,13 +229,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/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 6f13254..04ca54e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -144,7 +144,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);
     /* maximum devices allowed on the bus, 0: no limit. */
     int max_dev;
 };
-- 
1.8.2

Re: [Qemu-devel] [PATCH 1.5 0/3] qdev: switch reset to post-order

[Michael S. Tsirkin]

On Thu, May 02, 2013 at 11:38:36AM +0200, Paolo Bonzini wrote:
> I was going to keep these for 1.6, but since they have been tested
> by the user and myself, and they fix a regression, it can be worthwhile
> to include these in 1.5.

I remember there were some tricky issues last time
we tried something like this for PCI.

Seems too risky to me.
Let's find an LSI specific solution for 1.5 please.

> qdev reset (qdev_reset_all/qbus_reset_all) is currently done in
> pre-order.  This is not right, because it means the parent devices
> cannot expect anything about the children devices' state during the
> reset callback.
>
> With the traversal done *before* invoking the bus-reset callback,
> there is no way for the bus-reset callback to take care of the device
> tree traversal.  This is currently done only for PCI, which requires
> some adjustments.
> 
> This fixes a crash in resetting the LSI SCSI adapter, as reported and
> tested by Claudio Bley.
> 
> Paolo Bonzini (3):
>   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/core/qdev.c         | 47 ++++++++++++++++++++++++++++++++++-------------
>  hw/pci/pci.c           | 37 +++++++++++++++++--------------------
>  hw/pci/pci_bridge.c    |  2 +-
>  include/hw/pci/pci.h   |  1 -
>  include/hw/qdev-core.h | 15 ++++++++++-----
>  5 files changed, 62 insertions(+), 40 deletions(-)
> 
> -- 
> 1.8.2
> 

Re: [Qemu-devel] [PATCH 1.5 0/3] qdev: switch reset to post-order

[Paolo Bonzini]

Il 02/05/2013 12:35, Michael S. Tsirkin ha scritto:
> > I was going to keep these for 1.6, but since they have been tested
> > by the user and myself, and they fix a regression, it can be worthwhile
> > to include these in 1.5.
> 
> I remember there were some tricky issues last time
> we tried something like this for PCI.

PCI is already post-order though, isn't it?

> Let's find an LSI specific solution for 1.5 please.

Ok.

Paolo

Re: [Qemu-devel] [PATCH 1.5 0/3] qdev: switch reset to post-order

[Michael S. Tsirkin]

On Thu, May 02, 2013 at 01:01:20PM +0200, Paolo Bonzini wrote:
> Il 02/05/2013 12:35, Michael S. Tsirkin ha scritto:
> > > I was going to keep these for 1.6, but since they have been tested
> > > by the user and myself, and they fix a regression, it can be worthwhile
> > > to include these in 1.5.
> > 
> > I remember there were some tricky issues last time
> > we tried something like this for PCI.
> 
> PCI is already post-order though, isn't it?

For example, it resets the irq count before.
I really don't know why, and the reverse looks more
correct but let's tread carefully for 1.5.
For 1.6 maybe we'll be able to finally get rid
of the irq count.

> > Let's find an LSI specific solution for 1.5 please.
> 
> Ok.
> 
> Paolo