[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(-)

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

[Michael S. Tsirkin]

On Thu, Oct 03, 2013 at 03:46:11PM +0200, 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.

Need to go carefully here. I remember a bunch of targets
were relying on reset in this order, though don't
have the detail right now.

What kind of testing did this patchset go through?

> 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(-)

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

[Paolo Bonzini]

Il 03/10/2013 15:54, Michael S. Tsirkin ha scritto:
> On Thu, Oct 03, 2013 at 03:46:11PM +0200, 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.
> 
> Need to go carefully here. I remember a bunch of targets
> were relying on reset in this order, though don't
> have the detail right now.

The main change is that sub-devices, which basically exist only for USB,
virtio, and SCSI, are reset _before_ the corresponding parent device
(the USB host controller, virtio proxy device, or SCSI HBA).

For SCSI, this patch fixes bugs that I'm currently working around in
virtio-scsi (see the manual reset in virtio_scsi_reset, that manually
resets the subdevices).

USB and virtio are not using qdev reset at all, so they are unaffected
by the patches.  They rely on the parent device's own reset method to
propagate the reset.  virtio already uses post-order, so it could be
changed to use qdev reset, but I don't plan to do it unless it fixes
bugs.  USB is more complicated and I don't really understand it.

For PCI, the change is that all devices are reset first, and then the
interrupts state and config space are reset for all devices.

> What kind of testing did this patchset go through?

For each PCI device I tried creating a VM with an instance of it (a few
devices at a time), and did VM resets.  Earlier versions were tested by
the guy who reported the SCSI problems.

Paolo

>> 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(-)
> 
> 

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

[Michael S. Tsirkin]

On Thu, Oct 03, 2013 at 05:58:30PM +0200, Paolo Bonzini wrote:
> Il 03/10/2013 15:54, Michael S. Tsirkin ha scritto:
> > On Thu, Oct 03, 2013 at 03:46:11PM +0200, 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.
> > 
> > Need to go carefully here. I remember a bunch of targets
> > were relying on reset in this order, though don't
> > have the detail right now.
> 
> The main change is that sub-devices, which basically exist only for USB,
> virtio, and SCSI, are reset _before_ the corresponding parent device
> (the USB host controller, virtio proxy device, or SCSI HBA).
> 
> For SCSI, this patch fixes bugs that I'm currently working around in
> virtio-scsi (see the manual reset in virtio_scsi_reset, that manually
> resets the subdevices).
> 
> USB and virtio are not using qdev reset at all, so they are unaffected
> by the patches.  They rely on the parent device's own reset method to
> propagate the reset.  virtio already uses post-order, so it could be
> changed to use qdev reset, but I don't plan to do it unless it fixes
> bugs.  USB is more complicated and I don't really understand it.
> 
> For PCI, the change is that all devices are reset first, and then the
> interrupts state and config space are reset for all devices.
> 
> > What kind of testing did this patchset go through?
> 
> For each PCI device I tried creating a VM with an instance of it (a few
> devices at a time), and did VM resets.  Earlier versions were tested by
> the guy who reported the SCSI problems.
> 
> Paolo

x86 kvm only?

> >> 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(-)
> > 
> > 

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

[Paolo Bonzini]

Il 03/10/2013 18:54, Michael S. Tsirkin ha scritto:
> > For each PCI device I tried creating a VM with an instance of it (a few
> > devices at a time), and did VM resets.  Earlier versions were tested by
> > the guy who reported the SCSI problems.
> 
> x86 kvm only?

Yes.

Paolo

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

[Michael S. Tsirkin]

On Thu, Oct 03, 2013 at 06:53:10PM +0200, Paolo Bonzini wrote:
> Il 03/10/2013 18:54, Michael S. Tsirkin ha scritto:
> > > For each PCI device I tried creating a VM with an instance of it (a few
> > > devices at a time), and did VM resets.  Earlier versions were tested by
> > > the guy who reported the SCSI problems.
> > 
> > x86 kvm only?
> 
> Yes.
> 
> Paolo

Hmm, I'm not sure that's enough for this kind of change.

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

[Paolo Bonzini]

Il 06/10/2013 20:28, Michael S. Tsirkin ha scritto:
> > > > For each PCI device I tried creating a VM with an instance of it (a few
> > > > devices at a time), and did VM resets.  Earlier versions were tested by
> > > > the guy who reported the SCSI problems.
> > > 
> > > x86 kvm only?
> > 
> > Yes.
> 
> Hmm, I'm not sure that's enough for this kind of change.

I'll do more tests though, from looking at the source code, I'm not sure
what could happen depending on the host bridge.  To make it clearer, the
difference is that pre-patch you have

    device-level reset for device 1
    bus-level reset for device 1
    device-level reset for device 2
    bus-level reset for device 2
    device-level reset for device 3
    bus-level reset for device 3

and afterwards you have

    device-level reset for device 1
    device-level reset for device 2
    device-level reset for device 3
    bus-level reset for device 1
    bus-level reset for device 2
    bus-level reset for device 3

I could also preserve exactly the same semantics if you prefer.  The
patch is a bit more complicated, but it's doable.

Paolo

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

[Paolo Bonzini]

Il 06/10/2013 22:34, Paolo Bonzini ha scritto:
> Il 06/10/2013 20:28, Michael S. Tsirkin ha scritto:
>>>>> For each PCI device I tried creating a VM with an instance of it (a few
>>>>> devices at a time), and did VM resets.  Earlier versions were tested by
>>>>> the guy who reported the SCSI problems.
>>>>
>>>> x86 kvm only?
>>>
>>> Yes.
>>
>> Hmm, I'm not sure that's enough for this kind of change.
> 
> I'll do more tests though, from looking at the source code, I'm not sure
> what could happen depending on the host bridge.

Did more tests, PPC g3beige and PPC64 mac99 both work.

I also tested resetting the secondary bus of a PCI bridge (via setpci),
and it also works as expected.

Finally, I looked more at the history of the code to justify patch 2.

Initially, zeroing the irq_state was added in commit 6eaa684 (Add
pci_bus_reset() function., 2009-06-17) to deal with this issue:

>> Shouldn't each device's reset function bring its line low, thus zeroing  
>> the irq_state naturally?
>>
>> If not, we have a bug somewhere.  Note we have exactly the same issue  
>> with save/restore.
>>
> They should, but I haven't found one that does.

More registers were then cleared by pci_device_reset in your commit
c0b1905 (qemu/pci: reset device registers on bus reset, 2009-09-16).
Deasserting interrupts explicitly came in later as part of PCI bus and
FLR support in commit 4c92325 (pci: deassert intx on reset.,
2011-01-20).  That should be the point where the code starts following
the invariant of patch 2.

Paolo