[Qemu-devel] [PATCH 0/4] qdev: switch reset to post-order, clean up PCI reset

[Qemu-devel] [PATCH 0/4] qdev: switch reset to post-order, clean up PCI reset

[Paolo Bonzini]

PCI is handling resetting of its devices before the bus is reset,
but this is only necessary because qdev is broken and usually does
pre-order reset.  Post-order is a much better definition.  Drop
the unnecessary flexibility that lets bus decide the reset order,
convert to post-order, and make PCI use common code for reset.

Paolo Bonzini (4):
  pci: do not export pci_bus_reset
  pci: clean up resetting of IRQs
  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           |   42 ++++++++++++++++++++----------------------
 hw/pci/pci_bridge.c    |    2 +-
 include/hw/pci/pci.h   |    1 -
 include/hw/qdev-core.h |   15 ++++++++++-----
 5 files changed, 65 insertions(+), 42 deletions(-)

[Qemu-devel] [PATCH 1/4] 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.

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 bd084c7..ac3244b 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -210,8 +210,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++) {
@@ -222,11 +223,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 307e076..06831a2 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -268,7 +268,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 ccec2ba..32f1419 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -376,7 +376,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, PCIBus *rootbus,
                         const char *default_model,
-- 
1.7.1

[Qemu-devel] [PATCH 2/4] pci: clean up resetting of IRQs

[Paolo Bonzini]

pci_device_reset will deassert the INTX pins, and this will make the
irq_count array all-zeroes.  Check that this is the case, and remove
the existing loop which might even unsync irq_count and irq_state.

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index ac3244b..0efc544 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -215,15 +215,16 @@ static int pcibus_reset(BusState *qbus)
     PCIBus *bus = DO_UPCAST(PCIBus, qbus, qbus);
     int i;
 
-    for (i = 0; i < bus->nirq; i++) {
-        bus->irq_count[i] = 0;
-    }
     for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
         if (bus->devices[i]) {
             pci_device_reset(bus->devices[i]);
         }
     }
 
+    for (i = 0; i < bus->nirq; i++) {
+        assert(bus->irq_count[i] == 0);
+    }
+
     /* topology traverse is done by pci_bus_reset().
        Tell qbus/qdev walker not to traverse the tree */
     return 1;
-- 
1.7.1

[Qemu-devel] [PATCH 3/4] 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.

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 758de9f..1c114b7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -240,12 +240,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)
@@ -343,49 +343,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 d840f06..21ea2c6 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -274,10 +274,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.7.1

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

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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/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 758de9f..1c114b7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -240,12 +240,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)
> @@ -343,49 +343,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)
>  {

It's either pre or post right ? I also thought that the traversal 
applies to the whole hierarchy not just buses or devices individually..
Why not just have a single parameter that says pre or post, not much 
difference but atleast one parameter less.

Bandan

>      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 d840f06..21ea2c6 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -274,10 +274,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);
>  
>  /**

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

[Paolo Bonzini]

Il 07/12/2013 00:27, Bandan Das ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> 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/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 758de9f..1c114b7 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -240,12 +240,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)
>> @@ -343,49 +343,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)
>>  {
> 
> It's either pre or post right ? I also thought that the traversal 
> applies to the whole hierarchy not just buses or devices individually..
> Why not just have a single parameter that says pre or post, not much 
> difference but atleast one parameter less.

This is a generic walk function.

For reset you want post-order, but in other cases pre-order may make
more sense, for example realize.

Paolo

> Bandan
> 
>>      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 d840f06..21ea2c6 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -274,10 +274,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);
>>  
>>  /**

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

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 07/12/2013 00:27, Bandan Das ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> 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/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 758de9f..1c114b7 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -240,12 +240,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)
>>> @@ -343,49 +343,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)
>>>  {
>> 
>> It's either pre or post right ? I also thought that the traversal 
>> applies to the whole hierarchy not just buses or devices individually..
>> Why not just have a single parameter that says pre or post, not much 
>> difference but atleast one parameter less.
>
> This is a generic walk function.
> For reset you want post-order, but in other cases pre-order may make
> more sense, for example realize.

Sorry, not sure if I get it. What I meant was will this work ?

 int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque)
+                       qbus_walkerfn *busfn, bool ispostorder, void *opaque)
 {
     BusChild *kid;
     int err;
 
-    if (busfn) {
+    if (!ispostorder && busfn) {
         err = busfn(bus, opaque);
         if (err) {
             return err;
@@ -351,17 +351,24 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
     }
 
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        err = qdev_walk_children(kid->child, devfn, busfn, opaque);
+        err = qdev_walk_children(kid->child, devfn, busfn, ispostorder, opaque);
         if (err < 0) {
             return err;
         }
     }
 
+    if (ispostorder && busfn) {
+        err = busfn(bus, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
     return 0;
 }

Or there's a case where we would like to traverse pre for parent and post
for children's buses (or something similar)..

> Paolo
>
>> Bandan
>> 
>>>      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 d840f06..21ea2c6 100644
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -274,10 +274,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);
>>>  
>>>  /**

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

[Paolo Bonzini]

Il 09/12/2013 18:56, Bandan Das ha scritto:
>> > This is a generic walk function.
>> > For reset you want post-order, but in other cases pre-order may make
>> > more sense, for example realize.
> Sorry, not sure if I get it. What I meant was will this work ?
> 
>  int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
> -                       qbus_walkerfn *busfn, void *opaque)
> +                       qbus_walkerfn *busfn, bool ispostorder, void *opaque)

Yes, but it is a bit less flexible.

> Or there's a case where we would like to traverse pre for parent and post
> for children's buses (or something similar)..

Probably not, but there may be a case where you want both pre and post
(i.e. before and after).  For example a "display tree" functionality
where the post-order callbacks simply decrement the current indentation.

Paolo

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

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 09/12/2013 18:56, Bandan Das ha scritto:
>>> > This is a generic walk function.
>>> > For reset you want post-order, but in other cases pre-order may make
>>> > more sense, for example realize.
>> Sorry, not sure if I get it. What I meant was will this work ?
>> 
>>  int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
>> -                       qbus_walkerfn *busfn, void *opaque)
>> +                       qbus_walkerfn *busfn, bool ispostorder, void *opaque)
>
> Yes, but it is a bit less flexible.

Well, my motivation was that very soon we will have an infinite number of
arguments to the walk functions (oh wait! we already have) :)

>> Or there's a case where we would like to traverse pre for parent and post
>> for children's buses (or something similar)..
>
> Probably not, but there may be a case where you want both pre and post
> (i.e. before and after).  For example a "display tree" functionality
> where the post-order callbacks simply decrement the current indentation.

Oh so there is such a case. Ok, got it now, thanks!

> Paolo

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

[Paolo Bonzini]

Il 09/12/2013 18:56, Bandan Das ha scritto:
>> > This is a generic walk function.
>> > For reset you want post-order, but in other cases pre-order may make
>> > more sense, for example realize.
> Sorry, not sure if I get it. What I meant was will this work ?
> 
>  int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
> -                       qbus_walkerfn *busfn, void *opaque)
> +                       qbus_walkerfn *busfn, bool ispostorder, void *opaque)

Yes, but it is a bit less flexible.

If we want to remove the flexibility, I'd rather have two separate
functions for pre- and post-order, not a new "bool" parameter.

> Or there's a case where we would like to traverse pre for parent and post
> for children's buses (or something similar)..

Probably not, but there may be a case where you want both pre and post
(i.e. before and after).  For example a "display tree" functionality
where the post-order callbacks simply decrement the current indentation.

Paolo

[Qemu-devel] [PATCH 4/4] 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 pci_do_device_reset.

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 1c114b7..9ba8ab1 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -233,19 +233,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 0efc544..e10d74b 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -46,7 +46,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),
@@ -165,16 +165,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);
@@ -207,27 +201,34 @@ 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;
 
     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]);
         }
     }
 
     for (i = 0; i < bus->nirq; i++) {
         assert(bus->irq_count[i] == 0);
     }
-
-    /* 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(PCIBus *bus, DeviceState *parent)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 21ea2c6..409fd71 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -178,7 +178,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.7.1

Re: [Qemu-devel] [PATCH 4/4] qdev: switch reset to post-order

[Michael S. Tsirkin]

On Fri, Dec 06, 2013 at 05:54:27PM +0100, Paolo Bonzini wrote:
> 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/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 1c114b7..9ba8ab1 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -233,19 +233,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 0efc544..e10d74b 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -46,7 +46,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),
> @@ -165,16 +165,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);
> @@ -207,27 +201,34 @@ 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;
>  
>      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]);
>          }
>      }
>  
>      for (i = 0; i < bus->nirq; i++) {
>          assert(bus->irq_count[i] == 0);
>      }
> -
> -    /* 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(PCIBus *bus, DeviceState *parent)
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 21ea2c6..409fd71 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -178,7 +178,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.7.1


This seems to break a bunch of stuff:

/scm/qemu/hw/s390x/virtio-ccw.c: In function
‘virtual_css_bus_class_init’:
/scm/qemu/hw/s390x/virtio-ccw.c:47:14: error: assignment from
incompatible pointer type [-Werror]
     k->reset = virtual_css_bus_reset;
              ^

Re: [Qemu-devel] [PATCH 0/4] qdev: switch reset to post-order, clean up PCI reset

[Michael S. Tsirkin]

On Fri, Dec 06, 2013 at 05:54:23PM +0100, Paolo Bonzini wrote:
> PCI is handling resetting of its devices before the bus is reset,
> but this is only necessary because qdev is broken and usually does
> pre-order reset.  Post-order is a much better definition.  Drop
> the unnecessary flexibility that lets bus decide the reset order,
> convert to post-order, and make PCI use common code for reset.
> 
> Paolo Bonzini (4):
>   pci: do not export pci_bus_reset
>   pci: clean up resetting of IRQs
>   qdev: allow both pre- and post-order vists in qdev walking functions
>   qdev: switch reset to post-order

Applied, thanks!

I had to fix the last patch as it broke virtio ccw though.

Also, waiting for tests to start passing on master
again before I send pull request.

>  hw/core/qdev.c         |   47 ++++++++++++++++++++++++++++++++++-------------
>  hw/pci/pci.c           |   42 ++++++++++++++++++++----------------------
>  hw/pci/pci_bridge.c    |    2 +-
>  include/hw/pci/pci.h   |    1 -
>  include/hw/qdev-core.h |   15 ++++++++++-----
>  5 files changed, 65 insertions(+), 42 deletions(-)

Re: [Qemu-devel] [PATCH 0/4] qdev: switch reset to post-order, clean up PCI reset

[Paolo Bonzini]

Il 19/12/2013 20:15, Michael S. Tsirkin ha scritto:
> On Fri, Dec 06, 2013 at 05:54:23PM +0100, Paolo Bonzini wrote:
>> PCI is handling resetting of its devices before the bus is reset,
>> but this is only necessary because qdev is broken and usually does
>> pre-order reset.  Post-order is a much better definition.  Drop
>> the unnecessary flexibility that lets bus decide the reset order,
>> convert to post-order, and make PCI use common code for reset.
>>
>> Paolo Bonzini (4):
>>   pci: do not export pci_bus_reset
>>   pci: clean up resetting of IRQs
>>   qdev: allow both pre- and post-order vists in qdev walking functions
>>   qdev: switch reset to post-order
> 
> Applied, thanks!
> 
> I had to fix the last patch as it broke virtio ccw though.

Awesome news before the Christmas break!  Thanks! :)

Paolo

[Qemu-devel] [PATCH 0/4] qdev: switch reset to post-order, clean up PCI reset

[Paolo Bonzini]

PCI is handling resetting of its devices before the bus is reset,
but this is only necessary because qdev is broken and usually does
pre-order reset.  Post-order is a much better definition.  Drop
the unnecessary flexibility that lets bus decide the reset order,
convert to post-order, and make PCI use common code for reset.

Paolo Bonzini (4):
  pci: do not export pci_bus_reset
  pci: clean up resetting of IRQs
  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           |   42 ++++++++++++++++++++----------------------
 hw/pci/pci_bridge.c    |    2 +-
 include/hw/pci/pci.h   |    1 -
 include/hw/qdev-core.h |   15 ++++++++++-----
 5 files changed, 65 insertions(+), 42 deletions(-)

[Qemu-devel] [PATCH 3/4] 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.

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 758de9f..1c114b7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -240,12 +240,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)
@@ -343,49 +343,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 d840f06..21ea2c6 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -274,10 +274,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.7.1