[Qemu-devel] [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Qemu-devel] [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Isaku Yamahata]

Here is v2. I updated the comments, and dropped the pci qdev reset patch.

Patch description:
The goal of this patch series is to implement secondary bus reset
emulation in pci-to-pci bridge.
At first, this patch series refactors qdev reset,
and then cleans up pci bus reset. Lastly implements pci bridge control
secondary bus reset bit.

This patch series is for pci bus reset, which is ported
from the following repo.
git://repo.or.cz/qemu/aliguori.git qdev-refactor

Changes v1 -> v2:
- update comment

Anthony Liguori (2):
  qbus: add functions to walk both devices and busses
  qdev: reset qdev along with qdev tree

Isaku Yamahata (4):
  qdev: introduce reset call back for qbus level
  qdev: introduce a helper function which triggers reset from a given
    device
  pci: make use of qdev reset frame work to pci bus reset.
  pci bridge: implement secondary bus reset

 hw/pci.c        |   33 ++++++++++++++++++--
 hw/pci.h        |    3 ++
 hw/pci_bridge.c |   12 +++++++-
 hw/qdev.c       |   87 +++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/qdev.h       |   18 +++++++++++
 vl.c            |    1 +
 6 files changed, 140 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH v2 1/6] qbus: add functions to walk both devices and busses

[Isaku Yamahata]

From: Anthony Liguori <anthony@codemonkey.ws>

There are some cases where you want to walk the busses, in particular, when
searching for a bus either by name or DeviceInfo.
Paolo suggested that we model the return values on how GCC's walkers work which
allows an actor to skip child transversal, or terminate walking with a positive
value that's returned as the qbus_walk_children's result.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

---
Changes v1 -> v2:
- update comments to match the implementation
---
 hw/qdev.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |   11 +++++++++++
 2 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 35858cb..11d845a 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -449,6 +449,52 @@ 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)
+{
+    DeviceState *dev;
+    int err;
+
+    if (busfn) {
+        err = busfn(bus, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
+    QLIST_FOREACH(dev, &bus->children, sibling) {
+        err = qdev_walk_children(dev, devfn, busfn, opaque);
+        if (err < 0) {
+            return err;
+        }
+    }
+
+    return 0;
+}
+
+int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
+                       qbus_walkerfn *busfn, void *opaque)
+{
+    BusState *bus;
+    int err;
+
+    if (devfn) {
+        err = devfn(dev, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
+    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+        err = qbus_walk_children(bus, devfn, busfn, opaque);
+        if (err < 0) {
+            return err;
+        }
+    }
+
+    return 0;
+}
+
 static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                      const BusInfo *info)
 {
diff --git a/hw/qdev.h b/hw/qdev.h
index 579328a..550fd9b 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -173,9 +173,20 @@ BusState *qdev_get_parent_bus(DeviceState *dev);
 
 /*** BUS API. ***/
 
+/* Returns 0 to walk children, > 0 to skip walk, < 0 to terminate walk. */
+typedef int (qbus_walkerfn)(BusState *bus, void *opaque);
+typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
+
 void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
+/* 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);
 void qbus_free(BusState *bus);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
-- 
1.7.1.1

[Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Isaku Yamahata]

From: Anthony Liguori <anthony@codemonkey.ws>

This patch changes the reset handling so that qdev has no knowledge of the
global system reset.  Instead, a new bus/device level function is introduced
that allows all devices/buses on the bus/device to be reset using a depth
first transversal.

N.B. we have to expose the implicit system bus because we have various hacks
that result in an implicit system bus existing.  Instead, we ought to have an
explicitly created system bus that we can trigger reset from.  That's a topic
for a future patch though.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/qdev.c |   28 +++++++++++++++++++---------
 hw/qdev.h |    4 ++++
 vl.c      |    1 +
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 11d845a..92ccc8d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -257,13 +257,6 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     return qdev;
 }
 
-static void qdev_reset(void *opaque)
-{
-    DeviceState *dev = opaque;
-    if (dev->info->reset)
-        dev->info->reset(dev);
-}
-
 /* Initialize a device.  Device properties should be set before calling
    this function.  IRQs and MMIO regions should be connected/mapped after
    calling this function.
@@ -279,7 +272,6 @@ int qdev_init(DeviceState *dev)
         qdev_free(dev);
         return rc;
     }
-    qemu_register_reset(qdev_reset, dev);
     if (dev->info->vmsd) {
         vmstate_register_with_alias_id(dev, -1, dev->info->vmsd, dev,
                                        dev->instance_id_alias,
@@ -308,6 +300,25 @@ int qdev_unplug(DeviceState *dev)
     return dev->info->unplug(dev);
 }
 
+static int qdev_reset_one(DeviceState *dev, void *opaque)
+{
+    if (dev->info->reset) {
+        dev->info->reset(dev);
+    }
+
+    return 0;
+}
+
+BusState *sysbus_get_default(void)
+{
+    return main_system_bus;
+}
+
+void qbus_reset_all(BusState *bus)
+{
+    qbus_walk_children(bus, qdev_reset_one, NULL, NULL);
+}
+
 /* can be used as ->unplug() callback for the simple cases */
 int qdev_simple_unplug_cb(DeviceState *dev)
 {
@@ -351,7 +362,6 @@ void qdev_free(DeviceState *dev)
         if (dev->opts)
             qemu_opts_del(dev->opts);
     }
-    qemu_unregister_reset(qdev_reset, dev);
     QLIST_REMOVE(dev, sibling);
     for (prop = dev->info->props; prop && prop->name; prop++) {
         if (prop->info->free) {
diff --git a/hw/qdev.h b/hw/qdev.h
index 550fd9b..e5ed333 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -187,10 +187,14 @@ 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);
+void qbus_reset_all(BusState *bus);
 void qbus_free(BusState *bus);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
+/* This should go away once we get rid of the NULL bus hack */
+BusState *sysbus_get_default(void);
+
 /*** monitor commands ***/
 
 void do_info_qtree(Monitor *mon);
diff --git a/vl.c b/vl.c
index c58583d..135fdeb 100644
--- a/vl.c
+++ b/vl.c
@@ -2976,6 +2976,7 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
     qemu_system_reset();
     if (loadvm) {
         if (load_vmstate(loadvm) < 0) {
-- 
1.7.1.1

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Stefan Weil]

Am 19.11.2010 10:55, schrieb Isaku Yamahata:
> From: Anthony Liguori <anthony@codemonkey.ws>
>
> This patch changes the reset handling so that qdev has no knowledge of the
> global system reset. Instead, a new bus/device level function is 
> introduced
> that allows all devices/buses on the bus/device to be reset using a depth
> first transversal.
>
> N.B. we have to expose the implicit system bus because we have various 
> hacks
> that result in an implicit system bus existing. Instead, we ought to 
> have an
> explicitly created system bus that we can trigger reset from. That's a 
> topic
> for a future patch though.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> hw/qdev.c | 28 +++++++++++++++++++---------
> hw/qdev.h | 4 ++++
> vl.c | 1 +
> 3 files changed, 24 insertions(+), 9 deletions(-)


This patch (which is included in qemu master now) introduces
two regressions at least for MIPS system emulation:

The emulated MIPS machines mipssim and malta no longer work
because qemu tries a null pointer read access (caused by
sysbus_get_default returning NULL). Just run any of these
machines (big or little endian, 32 or 64 bit) with any mips
kernel or bios to get this crash.

This first regression can be fixed with a patch which I have
sent to qemu-devel.

The second regression also occurs with MIPS malta.
Networking no longer works with the default pcnet nic.

This is caused because the reset function for pcnet is no
longer called during system boot. The result in an invalid
mac address (all zero) and a non-working nic.

For this second regression I still have no simple solution.
Of course mips_malta.c should be converted to qdev which
would fix both problems (but only for malta system emulation).

Maybe other systems / devices don't get their reset functions
called during system boot, too.

Stefan Weil


>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 11d845a..92ccc8d 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -257,13 +257,6 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> return qdev;
> }
>
> -static void qdev_reset(void *opaque)
> -{
> - DeviceState *dev = opaque;
> - if (dev->info->reset)
> - dev->info->reset(dev);
> -}
> -
> /* Initialize a device. Device properties should be set before calling
> this function. IRQs and MMIO regions should be connected/mapped after
> calling this function.
> @@ -279,7 +272,6 @@ int qdev_init(DeviceState *dev)
> qdev_free(dev);
> return rc;
> }
> - qemu_register_reset(qdev_reset, dev);
> if (dev->info->vmsd) {
> vmstate_register_with_alias_id(dev, -1, dev->info->vmsd, dev,
> dev->instance_id_alias,
> @@ -308,6 +300,25 @@ int qdev_unplug(DeviceState *dev)
> return dev->info->unplug(dev);
> }
>
> +static int qdev_reset_one(DeviceState *dev, void *opaque)
> +{
> + if (dev->info->reset) {
> + dev->info->reset(dev);
> + }
> +
> + return 0;
> +}
> +
> +BusState *sysbus_get_default(void)
> +{
> + return main_system_bus;
> +}
> +
> +void qbus_reset_all(BusState *bus)
> +{
> + qbus_walk_children(bus, qdev_reset_one, NULL, NULL);
> +}
> +
> /* can be used as ->unplug() callback for the simple cases */
> int qdev_simple_unplug_cb(DeviceState *dev)
> {
> @@ -351,7 +362,6 @@ void qdev_free(DeviceState *dev)
> if (dev->opts)
> qemu_opts_del(dev->opts);
> }
> - qemu_unregister_reset(qdev_reset, dev);
> QLIST_REMOVE(dev, sibling);
> for (prop = dev->info->props; prop && prop->name; prop++) {
> if (prop->info->free) {
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 550fd9b..e5ed333 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -187,10 +187,14 @@ 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);
> +void qbus_reset_all(BusState *bus);
> void qbus_free(BusState *bus);
>
> #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
>
> +/* This should go away once we get rid of the NULL bus hack */
> +BusState *sysbus_get_default(void);
> +
> /*** monitor commands ***/
>
> void do_info_qtree(Monitor *mon);
> diff --git a/vl.c b/vl.c
> index c58583d..135fdeb 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2976,6 +2976,7 @@ int main(int argc, char **argv, char **envp)
> exit(1);
> }
>
> + qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
> qemu_system_reset();
> if (loadvm) {
> if (load_vmstate(loadvm) < 0) {

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Michael S. Tsirkin]

On Thu, Dec 16, 2010 at 08:31:21PM +0100, Stefan Weil wrote:
> Am 19.11.2010 10:55, schrieb Isaku Yamahata:
> >From: Anthony Liguori <anthony@codemonkey.ws>
> >
> >This patch changes the reset handling so that qdev has no knowledge of the
> >global system reset. Instead, a new bus/device level function is
> >introduced
> >that allows all devices/buses on the bus/device to be reset using a depth
> >first transversal.
> >
> >N.B. we have to expose the implicit system bus because we have
> >various hacks
> >that result in an implicit system bus existing. Instead, we ought
> >to have an
> >explicitly created system bus that we can trigger reset from.
> >That's a topic
> >for a future patch though.
> >
> >Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> >---
> >hw/qdev.c | 28 +++++++++++++++++++---------
> >hw/qdev.h | 4 ++++
> >vl.c | 1 +
> >3 files changed, 24 insertions(+), 9 deletions(-)
> 
> 
> This patch (which is included in qemu master now) introduces
> two regressions at least for MIPS system emulation:
> 
> The emulated MIPS machines mipssim and malta no longer work
> because qemu tries a null pointer read access (caused by
> sysbus_get_default returning NULL). Just run any of these
> machines (big or little endian, 32 or 64 bit) with any mips
> kernel or bios to get this crash.
> 
> This first regression can be fixed with a patch which I have
> sent to qemu-devel.
> 
> The second regression also occurs with MIPS malta.
> Networking no longer works with the default pcnet nic.
> 
> This is caused because the reset function for pcnet is no
> longer called during system boot. The result in an invalid
> mac address (all zero) and a non-working nic.
> 
> For this second regression I still have no simple solution.
> Of course mips_malta.c should be converted to qdev which
> would fix both problems (but only for malta system emulation).
> 
> Maybe other systems / devices don't get their reset functions
> called during system boot, too.
> 
> Stefan Weil

Ugh, I keep forgetting about the non-qdev systems.
Maybe it's a good way to finally make everyone to convert? If a system
maintainer can't be bothered to convert to qdev we can declare the
system unsupported :)

I think that as long as the system bus is qdev all pci devices are fine.


> >
> >diff --git a/hw/qdev.c b/hw/qdev.c
> >index 11d845a..92ccc8d 100644
> >--- a/hw/qdev.c
> >+++ b/hw/qdev.c
> >@@ -257,13 +257,6 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >return qdev;
> >}
> >
> >-static void qdev_reset(void *opaque)
> >-{
> >- DeviceState *dev = opaque;
> >- if (dev->info->reset)
> >- dev->info->reset(dev);
> >-}
> >-
> >/* Initialize a device. Device properties should be set before calling
> >this function. IRQs and MMIO regions should be connected/mapped after
> >calling this function.
> >@@ -279,7 +272,6 @@ int qdev_init(DeviceState *dev)
> >qdev_free(dev);
> >return rc;
> >}
> >- qemu_register_reset(qdev_reset, dev);
> >if (dev->info->vmsd) {
> >vmstate_register_with_alias_id(dev, -1, dev->info->vmsd, dev,
> >dev->instance_id_alias,
> >@@ -308,6 +300,25 @@ int qdev_unplug(DeviceState *dev)
> >return dev->info->unplug(dev);
> >}
> >
> >+static int qdev_reset_one(DeviceState *dev, void *opaque)
> >+{
> >+ if (dev->info->reset) {
> >+ dev->info->reset(dev);
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+BusState *sysbus_get_default(void)
> >+{
> >+ return main_system_bus;
> >+}
> >+
> >+void qbus_reset_all(BusState *bus)
> >+{
> >+ qbus_walk_children(bus, qdev_reset_one, NULL, NULL);
> >+}
> >+
> >/* can be used as ->unplug() callback for the simple cases */
> >int qdev_simple_unplug_cb(DeviceState *dev)
> >{
> >@@ -351,7 +362,6 @@ void qdev_free(DeviceState *dev)
> >if (dev->opts)
> >qemu_opts_del(dev->opts);
> >}
> >- qemu_unregister_reset(qdev_reset, dev);
> >QLIST_REMOVE(dev, sibling);
> >for (prop = dev->info->props; prop && prop->name; prop++) {
> >if (prop->info->free) {
> >diff --git a/hw/qdev.h b/hw/qdev.h
> >index 550fd9b..e5ed333 100644
> >--- a/hw/qdev.h
> >+++ b/hw/qdev.h
> >@@ -187,10 +187,14 @@ 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);
> >+void qbus_reset_all(BusState *bus);
> >void qbus_free(BusState *bus);
> >
> >#define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
> >
> >+/* This should go away once we get rid of the NULL bus hack */
> >+BusState *sysbus_get_default(void);
> >+
> >/*** monitor commands ***/
> >
> >void do_info_qtree(Monitor *mon);
> >diff --git a/vl.c b/vl.c
> >index c58583d..135fdeb 100644
> >--- a/vl.c
> >+++ b/vl.c
> >@@ -2976,6 +2976,7 @@ int main(int argc, char **argv, char **envp)
> >exit(1);
> >}
> >
> >+ qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
> >qemu_system_reset();
> >if (loadvm) {
> >if (load_vmstate(loadvm) < 0) {

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Anthony Liguori]

On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
> On Thu, Dec 16, 2010 at 08:31:21PM +0100, Stefan Weil wrote:
>    
>> Am 19.11.2010 10:55, schrieb Isaku Yamahata:
>>      
>>> From: Anthony Liguori<anthony@codemonkey.ws>
>>>
>>> This patch changes the reset handling so that qdev has no knowledge of the
>>> global system reset. Instead, a new bus/device level function is
>>> introduced
>>> that allows all devices/buses on the bus/device to be reset using a depth
>>> first transversal.
>>>
>>> N.B. we have to expose the implicit system bus because we have
>>> various hacks
>>> that result in an implicit system bus existing. Instead, we ought
>>> to have an
>>> explicitly created system bus that we can trigger reset from.
>>> That's a topic
>>> for a future patch though.
>>>
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp>
>>> ---
>>> hw/qdev.c | 28 +++++++++++++++++++---------
>>> hw/qdev.h | 4 ++++
>>> vl.c | 1 +
>>> 3 files changed, 24 insertions(+), 9 deletions(-)
>>>        
>>
>> This patch (which is included in qemu master now) introduces
>> two regressions at least for MIPS system emulation:
>>
>> The emulated MIPS machines mipssim and malta no longer work
>> because qemu tries a null pointer read access (caused by
>> sysbus_get_default returning NULL). Just run any of these
>> machines (big or little endian, 32 or 64 bit) with any mips
>> kernel or bios to get this crash.
>>
>> This first regression can be fixed with a patch which I have
>> sent to qemu-devel.
>>
>> The second regression also occurs with MIPS malta.
>> Networking no longer works with the default pcnet nic.
>>
>> This is caused because the reset function for pcnet is no
>> longer called during system boot. The result in an invalid
>> mac address (all zero) and a non-working nic.
>>
>> For this second regression I still have no simple solution.
>> Of course mips_malta.c should be converted to qdev which
>> would fix both problems (but only for malta system emulation).
>>
>> Maybe other systems / devices don't get their reset functions
>> called during system boot, too.
>>
>> Stefan Weil
>>      
> Ugh, I keep forgetting about the non-qdev systems.
> Maybe it's a good way to finally make everyone to convert? If a system
> maintainer can't be bothered to convert to qdev we can declare the
> system unsupported :)
>    

Honestly, I think that's a fair thing to do.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Peter Maydell]

On 17 December 2010 00:24, Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:
> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>> Ugh, I keep forgetting about the non-qdev systems.
>> Maybe it's a good way to finally make everyone to convert? If a system
>> maintainer can't be bothered to convert to qdev we can declare the
>> system unsupported :)

> Honestly, I think that's a fair thing to do.

I'm guessing that would be a transition/conversion to make for the
0.15 release, right?

-- PMM

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Anthony Liguori]

On 12/17/2010 08:45 AM, Peter Maydell wrote:
> On 17 December 2010 00:24, Anthony Liguori<aliguori@linux.vnet.ibm.com>  wrote:
>    
>> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>>      
>>> Ugh, I keep forgetting about the non-qdev systems.
>>> Maybe it's a good way to finally make everyone to convert? If a system
>>> maintainer can't be bothered to convert to qdev we can declare the
>>> system unsupported :)
>>>        
>    
>> Honestly, I think that's a fair thing to do.
>>      
> I'm guessing that would be a transition/conversion to make for the
> 0.15 release, right?
>    

Yes.

Regards,

Anthony Liguori

> -- PMM
>    

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Markus Armbruster]

Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:

> On 12/17/2010 08:45 AM, Peter Maydell wrote:
>> On 17 December 2010 00:24, Anthony Liguori<aliguori@linux.vnet.ibm.com>  wrote:
>>    
>>> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>>>      
>>>> Ugh, I keep forgetting about the non-qdev systems.
>>>> Maybe it's a good way to finally make everyone to convert? If a system
>>>> maintainer can't be bothered to convert to qdev we can declare the
>>>> system unsupported :)
>>>>        
>>    
>>> Honestly, I think that's a fair thing to do.
>>>      
>> I'm guessing that would be a transition/conversion to make for the
>> 0.15 release, right?
>>    
>
> Yes.

Let's do it.

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Stefan Weil]

Am 20.12.2010 13:58, schrieb Markus Armbruster:
> Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:
>
>> On 12/17/2010 08:45 AM, Peter Maydell wrote:
>>> On 17 December 2010 00:24, Anthony 
>>> Liguori<aliguori@linux.vnet.ibm.com> wrote:
>>>
>>>> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>>>>
>>>>> Ugh, I keep forgetting about the non-qdev systems.
>>>>> Maybe it's a good way to finally make everyone to convert? If a system
>>>>> maintainer can't be bothered to convert to qdev we can declare the
>>>>> system unsupported :)
>>>>>
>>>
>>>> Honestly, I think that's a fair thing to do.
>>>>
>>> I'm guessing that would be a transition/conversion to make for the
>>> 0.15 release, right?
>>>
>>
>> Yes.
>
> Let's do it.


I already tried to convert malta / gt64xxx to qdev. While simple
devices are easily converted, I'm still struggling with gt64xxx.

There is some qdev documentation for end users (command line
parameters), but is there qdev documentation for developers, too?
If not, adding a qdev chapter to qemu-tech.texi (preferred), writing a
qdev wiki page, code templates (for systems / buses / bus controllers /
bus devices) or some other form of documentation would be a good start
for release 0.15 (imho).

Regards
Stefan

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Markus Armbruster]

Stefan Weil <weil@mail.berlios.de> writes:

> Am 20.12.2010 13:58, schrieb Markus Armbruster:
>> Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:
>>
>>> On 12/17/2010 08:45 AM, Peter Maydell wrote:
>>>> On 17 December 2010 00:24, Anthony
>>>> Liguori<aliguori@linux.vnet.ibm.com> wrote:
>>>>
>>>>> On 12/16/2010 05:48 PM, Michael S. Tsirkin wrote:
>>>>>
>>>>>> Ugh, I keep forgetting about the non-qdev systems.
>>>>>> Maybe it's a good way to finally make everyone to convert? If a system
>>>>>> maintainer can't be bothered to convert to qdev we can declare the
>>>>>> system unsupported :)
>>>>>>
>>>>
>>>>> Honestly, I think that's a fair thing to do.
>>>>>
>>>> I'm guessing that would be a transition/conversion to make for the
>>>> 0.15 release, right?
>>>>
>>>
>>> Yes.
>>
>> Let's do it.
>
>
> I already tried to convert malta / gt64xxx to qdev. While simple
> devices are easily converted, I'm still struggling with gt64xxx.

Specific problems?

> There is some qdev documentation for end users (command line
> parameters), but is there qdev documentation for developers, too?
> If not, adding a qdev chapter to qemu-tech.texi (preferred), writing a
> qdev wiki page, code templates (for systems / buses / bus controllers /
> bus devices) or some other form of documentation would be a good start
> for release 0.15 (imho).

It's not what you ask for, but perhaps it helps a bit anyway:
http://www.linux-kvm.org/wiki/images/f/fe/2010-forum-armbru-qdev.pdf

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Isaku Yamahata]

On Thu, Dec 16, 2010 at 08:31:21PM +0100, Stefan Weil wrote:
> The second regression also occurs with MIPS malta.
> Networking no longer works with the default pcnet nic.
>
> This is caused because the reset function for pcnet is no
> longer called during system boot. The result in an invalid
> mac address (all zero) and a non-working nic.
>
> For this second regression I still have no simple solution.
> Of course mips_malta.c should be converted to qdev which
> would fix both problems (but only for malta system emulation).
>
> Maybe other systems / devices don't get their reset functions
> called during system boot, too.

Arg, sorry for regression. How about the following?

diff --git a/hw/qdev.c b/hw/qdev.c
index 10e28df..774c53b 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -753,8 +753,11 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
     if (parent) {
         QLIST_INSERT_HEAD(&parent->child_bus, bus, sibling);
         parent->num_child_bus++;
+    } else if (bus != main_system_bus) {
+        /* TODO: once all device is qdevified,
+           reset handler for main_system_bus should also be registered here */
+        qemu_register_reset((void *)qbus_reset_all, bus);
     }
-
 }
 
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
diff --git a/vl.c b/vl.c
index c4d3fc0..2ec3989 100644
--- a/vl.c
+++ b/vl.c
@@ -3088,6 +3088,8 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    /* TODO: once all device is qdevified, this should be done by
+       qbus_create_inplace() */
     qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
     qemu_run_machine_init_done_notifiers();
 

-- 
yamahata

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Isaku Yamahata]

On Fri, Dec 17, 2010 at 01:29:42PM +0900, Isaku Yamahata wrote:
> On Thu, Dec 16, 2010 at 08:31:21PM +0100, Stefan Weil wrote:
> > The second regression also occurs with MIPS malta.
> > Networking no longer works with the default pcnet nic.
> >
> > This is caused because the reset function for pcnet is no
> > longer called during system boot. The result in an invalid
> > mac address (all zero) and a non-working nic.
> >
> > For this second regression I still have no simple solution.
> > Of course mips_malta.c should be converted to qdev which
> > would fix both problems (but only for malta system emulation).
> >
> > Maybe other systems / devices don't get their reset functions
> > called during system boot, too.
> 
> Arg, sorry for regression. How about the following?

The issue is, it is assumed that all qbuses, qdeves
are under main_system_bus.
But there are qbuses whose parent is NULL. So it is necessary
to trigger reset for those qbuses.

Ideally those buses should be moved under bus controller
device which is qdev. But it's not done yet.


> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 10e28df..774c53b 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -753,8 +753,11 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
>      if (parent) {
>          QLIST_INSERT_HEAD(&parent->child_bus, bus, sibling);
>          parent->num_child_bus++;
> +    } else if (bus != main_system_bus) {
> +        /* TODO: once all device is qdevified,
> +           reset handler for main_system_bus should also be registered here */
> +        qemu_register_reset((void *)qbus_reset_all, bus);
>      }
> -
>  }
>  
>  BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
> diff --git a/vl.c b/vl.c
> index c4d3fc0..2ec3989 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3088,6 +3088,8 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> +    /* TODO: once all device is qdevified, this should be done by
> +       qbus_create_inplace() */
>      qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
>      qemu_run_machine_init_done_notifiers();
>  
> 
> -- 
> yamahata
> 

-- 
yamahata

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Stefan Weil]

Am 17.12.2010 05:47, schrieb Isaku Yamahata:
> On Fri, Dec 17, 2010 at 01:29:42PM +0900, Isaku Yamahata wrote:
>> On Thu, Dec 16, 2010 at 08:31:21PM +0100, Stefan Weil wrote:
>>> The second regression also occurs with MIPS malta.
>>> Networking no longer works with the default pcnet nic.
>>>
>>> This is caused because the reset function for pcnet is no
>>> longer called during system boot. The result in an invalid
>>> mac address (all zero) and a non-working nic.
>>>
>>> For this second regression I still have no simple solution.
>>> Of course mips_malta.c should be converted to qdev which
>>> would fix both problems (but only for malta system emulation).
>>>
>>> Maybe other systems / devices don't get their reset functions
>>> called during system boot, too.
>>
>> Arg, sorry for regression. How about the following?
>
> The issue is, it is assumed that all qbuses, qdeves
> are under main_system_bus.
> But there are qbuses whose parent is NULL. So it is necessary
> to trigger reset for those qbuses.
>
> Ideally those buses should be moved under bus controller
> device which is qdev. But it's not done yet.
>
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index 10e28df..774c53b 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -753,8 +753,11 @@ void qbus_create_inplace(BusState *bus, BusInfo 
>> *info,
>> if (parent) {
>> QLIST_INSERT_HEAD(&parent->child_bus, bus, sibling);
>> parent->num_child_bus++;
>> + } else if (bus != main_system_bus) {
>> + /* TODO: once all device is qdevified,
>> + reset handler for main_system_bus should also be registered here */
>> + qemu_register_reset((void *)qbus_reset_all, bus);
>> }
>> -
>> }
>>
>> BusState *qbus_create(BusInfo *info, DeviceState *parent, const char 
>> *name)
>> diff --git a/vl.c b/vl.c
>> index c4d3fc0..2ec3989 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3088,6 +3088,8 @@ int main(int argc, char **argv, char **envp)
>> exit(1);
>> }
>>
>> + /* TODO: once all device is qdevified, this should be done by
>> + qbus_create_inplace() */
>> qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
>> qemu_run_machine_init_done_notifiers();
>>
>>
>> -- 
>> yamahata
>

I can confirm that this patch fixes the second regression:
the reset code for pcnet is called during system boot,
and networking with pcnet works fine again.

Could you please provide a signed patch which is ready for commit?

Michael, my patch (which fixes the first regression / crash)
is needed, too. Please add it to your git queue.

Stefan

Re: [Qemu-devel] [PATCH v2 2/6] qdev: reset qdev along with qdev tree

[Michael S. Tsirkin]

> Michael, my patch (which fixes the first regression / crash)
> is needed, too. Please add it to your git queue.
> 
> Stefan

Could you pls review/test the pci branch of my tree?
This should have all the necessary bits.

-- 
Thanks,
MST

[Qemu-devel] [PATCH v2 3/6] qdev: introduce reset call back for qbus level

[Isaku Yamahata]

and make it called via qbus_reset_all().
The qbus reset callback will be used by pci bus reset.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   10 +++++++++-
 hw/qdev.h |    2 ++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 92ccc8d..b76da07 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -314,9 +314,17 @@ BusState *sysbus_get_default(void)
     return main_system_bus;
 }
 
+static int qbus_reset_one(BusState *bus, void *opaque)
+{
+    if (bus->info->reset) {
+        return bus->info->reset(bus);
+    }
+    return 0;
+}
+
 void qbus_reset_all(BusState *bus)
 {
-    qbus_walk_children(bus, qdev_reset_one, NULL, NULL);
+    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
 }
 
 /* can be used as ->unplug() callback for the simple cases */
diff --git a/hw/qdev.h b/hw/qdev.h
index e5ed333..5ac084f 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -49,12 +49,14 @@ struct DeviceState {
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
 typedef char *(*bus_get_dev_path)(DeviceState *dev);
+typedef int (qbus_resetfn)(BusState *bus);
 
 struct BusInfo {
     const char *name;
     size_t size;
     bus_dev_printfn print_dev;
     bus_get_dev_path get_dev_path;
+    qbus_resetfn *reset;
     Property *props;
 };
 
-- 
1.7.1.1

[Qemu-devel] [PATCH v2 4/6] qdev: introduce a helper function which triggers reset from a given device

[Isaku Yamahata]

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |    5 +++++
 hw/qdev.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index b76da07..b65b63e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -322,6 +322,11 @@ static int qbus_reset_one(BusState *bus, void *opaque)
     return 0;
 }
 
+void qdev_reset_all(DeviceState *dev)
+{
+    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
+}
+
 void qbus_reset_all(BusState *bus)
 {
     qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
diff --git a/hw/qdev.h b/hw/qdev.h
index 5ac084f..3fac364 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -189,6 +189,7 @@ 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);
+void qdev_reset_all(DeviceState *dev);
 void qbus_reset_all(BusState *bus);
 void qbus_free(BusState *bus);
 
-- 
1.7.1.1

[Qemu-devel] [PATCH v2 5/6] pci: make use of qdev reset frame work to pci bus reset.

[Isaku Yamahata]

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/pci.c |   33 +++++++++++++++++++++++++++++----
 hw/pci.h |    3 +++
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 962886e..51b7857 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -43,12 +43,14 @@
 
 static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *pcibus_get_dev_path(DeviceState *dev);
+static int pcibus_reset(BusState *qbus);
 
 struct BusInfo pci_bus_info = {
     .name       = "PCI",
     .size       = sizeof(PCIBus),
     .print_dev  = pcibus_dev_print,
     .get_dev_path = pcibus_get_dev_path,
+    .reset      = pcibus_reset,
     .props      = (Property[]) {
         DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
         DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
@@ -133,7 +135,7 @@ static void pci_update_irq_status(PCIDevice *dev)
     }
 }
 
-static void pci_device_reset(PCIDevice *dev)
+void pci_device_reset_default(PCIDevice *dev)
 {
     int r;
 
@@ -161,9 +163,24 @@ static void pci_device_reset(PCIDevice *dev)
     pci_update_mappings(dev);
 }
 
-static void pci_bus_reset(void *opaque)
+static void pci_device_reset(PCIDevice *dev)
+{
+    if (!dev->qdev.info) {
+        /* not all pci devices haven't been qdev'fied yet
+           TODO: remove this when all pci devices are qdev'fied. */
+        pci_device_reset_default(dev);
+    } else {
+        qdev_reset_all(&dev->qdev);
+        pci_device_reset_default(dev);
+    }
+}
+
+/*
+ * Trigger pci bus reset under a given bus.
+ * This functions emulates RST#.
+ */
+void pci_bus_reset(PCIBus *bus)
 {
-    PCIBus *bus = opaque;
     int i;
 
     for (i = 0; i < bus->nirq; i++) {
@@ -176,6 +193,15 @@ static void pci_bus_reset(void *opaque)
     }
 }
 
+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 */
+    return 1;
+}
+
 static void pci_host_bus_register(int domain, PCIBus *bus)
 {
     struct PCIHostBus *host;
@@ -230,7 +256,6 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
     pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported */
 
     vmstate_register(NULL, -1, &vmstate_pcibus, bus);
-    qemu_register_reset(pci_bus_reset, bus);
 }
 
 PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min)
diff --git a/hw/pci.h b/hw/pci.h
index 7100804..280a2f8 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -225,6 +225,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                          void *irq_opaque, int devfn_min, int nirq);
 
+void pci_bus_reset(PCIBus *bus);
+void pci_device_reset_default(PCIDevice *dev);
+
 void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base);
 
 PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
-- 
1.7.1.1

[Qemu-devel] [PATCH v2 6/6] pci bridge: implement secondary bus reset

[Isaku Yamahata]

Emulates secondary bus reset when secondary bus reset bit
is written from 0 to 1.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/pci_bridge.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index 58cc2e4..618a81e 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -139,6 +139,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
 void pci_bridge_write_config(PCIDevice *d,
                              uint32_t address, uint32_t val, int len)
 {
+    PCIBridge *s = container_of(d, PCIBridge, dev);
+    uint16_t bridge_control = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
+    uint16_t bridge_control_new;
+
     pci_default_write_config(d, address, val, len);
 
     if (/* io base/limit */
@@ -147,9 +151,15 @@ void pci_bridge_write_config(PCIDevice *d,
         /* memory base/limit, prefetchable base/limit and
            io base/limit upper 16 */
         ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
-        PCIBridge *s = container_of(d, PCIBridge, dev);
         pci_bridge_update_mappings(&s->sec_bus);
     }
+
+    bridge_control_new = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
+    if (!(bridge_control & PCI_BRIDGE_CTL_BUS_RESET) &&
+        (bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET)) {
+        /* 0 -> 1 */
+        pci_bus_reset(&s->sec_bus);
+    }
 }
 
 void pci_bridge_disable_base_limit(PCIDevice *dev)
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH v2 6/6] pci bridge: implement secondary bus reset

[Michael S. Tsirkin]

On Fri, Nov 19, 2010 at 06:56:03PM +0900, Isaku Yamahata wrote:
> Emulates secondary bus reset when secondary bus reset bit
> is written from 0 to 1.
> 

Interesting. This is not exactly what happens on real hardware
though: there, RST# stays asserted until bit is cleared.
So for example attempts to scan the secondary bus will
return nothing.
Can implement this later, but please add a comment here.

> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/pci_bridge.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> index 58cc2e4..618a81e 100644
> --- a/hw/pci_bridge.c
> +++ b/hw/pci_bridge.c
> @@ -139,6 +139,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
>  void pci_bridge_write_config(PCIDevice *d,
>                               uint32_t address, uint32_t val, int len)
>  {
> +    PCIBridge *s = container_of(d, PCIBridge, dev);
> +    uint16_t bridge_control = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> +    uint16_t bridge_control_new;

I'd prefer shorter names for local variables.

> +
>      pci_default_write_config(d, address, val, len);
>  
>      if (/* io base/limit */
> @@ -147,9 +151,15 @@ void pci_bridge_write_config(PCIDevice *d,
>          /* memory base/limit, prefetchable base/limit and
>             io base/limit upper 16 */
>          ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
> -        PCIBridge *s = container_of(d, PCIBridge, dev);
>          pci_bridge_update_mappings(&s->sec_bus);
>      }
> +
> +    bridge_control_new = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> +    if (!(bridge_control & PCI_BRIDGE_CTL_BUS_RESET) &&
> +        (bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET)) {

Equivalent but shorter:
	~bridge_control & bridge_control_new & PCI_BRIDGE_CTL_BUS_RESET

> +        /* 0 -> 1 */
> +        pci_bus_reset(&s->sec_bus);
> +    }
>  }
>  
>  void pci_bridge_disable_base_limit(PCIDevice *dev)
> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Michael S. Tsirkin]

On Fri, Nov 19, 2010 at 06:55:57PM +0900, Isaku Yamahata wrote:
> Here is v2. I updated the comments, and dropped the pci qdev reset patch.
> 
> Patch description:
> The goal of this patch series is to implement secondary bus reset
> emulation in pci-to-pci bridge.
> At first, this patch series refactors qdev reset,
> and then cleans up pci bus reset. Lastly implements pci bridge control
> secondary bus reset bit.
> 
> This patch series is for pci bus reset, which is ported
> from the following repo.
> git://repo.or.cz/qemu/aliguori.git qdev-refactor

I've put the series on my pci branch, tweaking patches 5 and 6 in the
process.  Out of time to compile-tested only for now.
We do need to fix up secondary bus reset, so it's a needed bugfix.
Gerd, Anthony - any comments esp. on the qdev part?

> Changes v1 -> v2:
> - update comment
> 
> Anthony Liguori (2):
>   qbus: add functions to walk both devices and busses
>   qdev: reset qdev along with qdev tree
> 
> Isaku Yamahata (4):
>   qdev: introduce reset call back for qbus level
>   qdev: introduce a helper function which triggers reset from a given
>     device
>   pci: make use of qdev reset frame work to pci bus reset.
>   pci bridge: implement secondary bus reset
> 
>  hw/pci.c        |   33 ++++++++++++++++++--
>  hw/pci.h        |    3 ++
>  hw/pci_bridge.c |   12 +++++++-
>  hw/qdev.c       |   87 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  hw/qdev.h       |   18 +++++++++++
>  vl.c            |    1 +
>  6 files changed, 140 insertions(+), 14 deletions(-)

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Isaku Yamahata]

On Mon, Nov 22, 2010 at 09:54:02AM +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 19, 2010 at 06:55:57PM +0900, Isaku Yamahata wrote:
> > Here is v2. I updated the comments, and dropped the pci qdev reset patch.
> > 
> > Patch description:
> > The goal of this patch series is to implement secondary bus reset
> > emulation in pci-to-pci bridge.
> > At first, this patch series refactors qdev reset,
> > and then cleans up pci bus reset. Lastly implements pci bridge control
> > secondary bus reset bit.
> > 
> > This patch series is for pci bus reset, which is ported
> > from the following repo.
> > git://repo.or.cz/qemu/aliguori.git qdev-refactor
> 
> I've put the series on my pci branch, tweaking patches 5 and 6 in the
> process.  Out of time to compile-tested only for now.

Thank you. The tweaking looks good.
Do you still want me to send another patch to add a comment on RST#?
-- 
yamahata

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Michael S. Tsirkin]

On Mon, Nov 22, 2010 at 07:43:37PM +0900, Isaku Yamahata wrote:
> On Mon, Nov 22, 2010 at 09:54:02AM +0200, Michael S. Tsirkin wrote:
> > On Fri, Nov 19, 2010 at 06:55:57PM +0900, Isaku Yamahata wrote:
> > > Here is v2. I updated the comments, and dropped the pci qdev reset patch.
> > > 
> > > Patch description:
> > > The goal of this patch series is to implement secondary bus reset
> > > emulation in pci-to-pci bridge.
> > > At first, this patch series refactors qdev reset,
> > > and then cleans up pci bus reset. Lastly implements pci bridge control
> > > secondary bus reset bit.
> > > 
> > > This patch series is for pci bus reset, which is ported
> > > from the following repo.
> > > git://repo.or.cz/qemu/aliguori.git qdev-refactor
> > 
> > I've put the series on my pci branch, tweaking patches 5 and 6 in the
> > process.  Out of time to compile-tested only for now.
> 
> Thank you. The tweaking looks good.
> Do you still want me to send another patch to add a comment on RST#?

Probably not. I'm not sure I understand what the conventional
PCI spec says: should devices be reset on 0->1
transition, or kept in reset state until this bit is cleared?
And it seems quite clear that in the express
case the reset is only for 0->1 transition.
I've asked for clarification from pcisig.

> -- 
> yamahata

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Michael S. Tsirkin]

On Mon, Nov 22, 2010 at 07:43:37PM +0900, Isaku Yamahata wrote:
> On Mon, Nov 22, 2010 at 09:54:02AM +0200, Michael S. Tsirkin wrote:
> > On Fri, Nov 19, 2010 at 06:55:57PM +0900, Isaku Yamahata wrote:
> > > Here is v2. I updated the comments, and dropped the pci qdev reset patch.
> > > 
> > > Patch description:
> > > The goal of this patch series is to implement secondary bus reset
> > > emulation in pci-to-pci bridge.
> > > At first, this patch series refactors qdev reset,
> > > and then cleans up pci bus reset. Lastly implements pci bridge control
> > > secondary bus reset bit.
> > > 
> > > This patch series is for pci bus reset, which is ported
> > > from the following repo.
> > > git://repo.or.cz/qemu/aliguori.git qdev-refactor
> > 
> > I've put the series on my pci branch, tweaking patches 5 and 6 in the
> > process.  Out of time to compile-tested only for now.
> 
> Thank you. The tweaking looks good.
> Do you still want me to send another patch to add a comment on RST#?

OK, I got response, and yes, we need to fix code to avoid claiming
transactions by devices on the secondary bus while secondary bus is in
RST# for both pci and express.

> -- 
> yamahata

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Michael S. Tsirkin]

On Tue, Nov 23, 2010 at 12:53:12AM +0200, Michael S. Tsirkin wrote:
> On Mon, Nov 22, 2010 at 07:43:37PM +0900, Isaku Yamahata wrote:
> > On Mon, Nov 22, 2010 at 09:54:02AM +0200, Michael S. Tsirkin wrote:
> > > On Fri, Nov 19, 2010 at 06:55:57PM +0900, Isaku Yamahata wrote:
> > > > Here is v2. I updated the comments, and dropped the pci qdev reset patch.
> > > > 
> > > > Patch description:
> > > > The goal of this patch series is to implement secondary bus reset
> > > > emulation in pci-to-pci bridge.
> > > > At first, this patch series refactors qdev reset,
> > > > and then cleans up pci bus reset. Lastly implements pci bridge control
> > > > secondary bus reset bit.
> > > > 
> > > > This patch series is for pci bus reset, which is ported
> > > > from the following repo.
> > > > git://repo.or.cz/qemu/aliguori.git qdev-refactor
> > > 
> > > I've put the series on my pci branch, tweaking patches 5 and 6 in the
> > > process.  Out of time to compile-tested only for now.
> > 
> > Thank you. The tweaking looks good.
> > Do you still want me to send another patch to add a comment on RST#?
> 
> OK, I got response, and yes, we need to fix code to avoid claiming
> transactions by devices on the secondary bus while secondary bus is in
> RST# for both pci and express.

I guess the following fixes it?

--->

pci: don't access bus while it's reset

Devices on a bus should not respond while RST#
is asserted, which is controlled by PCI_BRIDGE_CTL_BUS_RESET.
Simply skip such a bus when propagating the configuration cycle.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/pci.c b/hw/pci.c
index d02f980..60bdded 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1554,7 +1554,9 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 
     /* try child bus */
     if (!bus->parent_dev /* host pci bridge */ ||
-        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
+        (!(pci_get_word(bus->parent_dev->config + PCI_BRIDGE_CONTROL) &
+           PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
+         bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
          bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
         for (; bus; bus = sec) {
             QLIST_FOREACH(sec, &bus->child, sibling) {

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Isaku Yamahata]

On Tue, Nov 23, 2010 at 08:10:26PM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 23, 2010 at 12:53:12AM +0200, Michael S. Tsirkin wrote:
> > On Mon, Nov 22, 2010 at 07:43:37PM +0900, Isaku Yamahata wrote:
> > > On Mon, Nov 22, 2010 at 09:54:02AM +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Nov 19, 2010 at 06:55:57PM +0900, Isaku Yamahata wrote:
> > > > > Here is v2. I updated the comments, and dropped the pci qdev reset patch.
> > > > > 
> > > > > Patch description:
> > > > > The goal of this patch series is to implement secondary bus reset
> > > > > emulation in pci-to-pci bridge.
> > > > > At first, this patch series refactors qdev reset,
> > > > > and then cleans up pci bus reset. Lastly implements pci bridge control
> > > > > secondary bus reset bit.
> > > > > 
> > > > > This patch series is for pci bus reset, which is ported
> > > > > from the following repo.
> > > > > git://repo.or.cz/qemu/aliguori.git qdev-refactor
> > > > 
> > > > I've put the series on my pci branch, tweaking patches 5 and 6 in the
> > > > process.  Out of time to compile-tested only for now.
> > > 
> > > Thank you. The tweaking looks good.
> > > Do you still want me to send another patch to add a comment on RST#?
> > 
> > OK, I got response, and yes, we need to fix code to avoid claiming
> > transactions by devices on the secondary bus while secondary bus is in
> > RST# for both pci and express.
> 
> I guess the following fixes it?

Almost. The following if claouse also needs modification.

commit 120fe782a198b787e13a9d4681afae8dc66dee50
Author: "Michael S. Tsirkin" <mst@redhat.com>
Date:   Wed Nov 24 11:34:59 2010 +0900

    pci: don't access bus while it's reset
    
    Devices on a bus should not respond while RST#
    is asserted, which is controlled by PCI_BRIDGE_CTL_BUS_RESET.
    Simply skip such a bus when propagating the configuration cycle.
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
    Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff --git a/hw/pci.c b/hw/pci.c
index d02f980..e7df612 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1554,7 +1554,9 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 
     /* try child bus */
     if (!bus->parent_dev /* host pci bridge */ ||
-        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
+        (!(pci_get_word(bus->parent_dev->config + PCI_BRIDGE_CONTROL) &
+           PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
+         bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
          bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
         for (; bus; bus = sec) {
             QLIST_FOREACH(sec, &bus->child, sibling) {
@@ -1562,8 +1564,11 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
                 if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
                     return sec;
                 }
-                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
-                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
+                if (pci_get_word(sec->parent_dev->config +
+                                 PCI_BRIDGE_CONTROL) & PCI_BRIDGE_CTL_BUS_RESET
+                    /* Don't walk the bus if it's reset. */ ||
+                    bus_num < sec->parent_dev->config[PCI_SECONDARY_BUS] ||
+                    sec->parent_dev->config[PCI_SUBORDINATE_BUS] < bus_num) {
                     break;
                 }
             }

-- 
yamahata

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Michael S. Tsirkin]

On Wed, Nov 24, 2010 at 11:37:25AM +0900, Isaku Yamahata wrote:
> On Tue, Nov 23, 2010 at 08:10:26PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 23, 2010 at 12:53:12AM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Nov 22, 2010 at 07:43:37PM +0900, Isaku Yamahata wrote:
> > > > On Mon, Nov 22, 2010 at 09:54:02AM +0200, Michael S. Tsirkin wrote:
> > > > > On Fri, Nov 19, 2010 at 06:55:57PM +0900, Isaku Yamahata wrote:
> > > > > > Here is v2. I updated the comments, and dropped the pci qdev reset patch.
> > > > > > 
> > > > > > Patch description:
> > > > > > The goal of this patch series is to implement secondary bus reset
> > > > > > emulation in pci-to-pci bridge.
> > > > > > At first, this patch series refactors qdev reset,
> > > > > > and then cleans up pci bus reset. Lastly implements pci bridge control
> > > > > > secondary bus reset bit.
> > > > > > 
> > > > > > This patch series is for pci bus reset, which is ported
> > > > > > from the following repo.
> > > > > > git://repo.or.cz/qemu/aliguori.git qdev-refactor
> > > > > 
> > > > > I've put the series on my pci branch, tweaking patches 5 and 6 in the
> > > > > process.  Out of time to compile-tested only for now.
> > > > 
> > > > Thank you. The tweaking looks good.
> > > > Do you still want me to send another patch to add a comment on RST#?
> > > 
> > > OK, I got response, and yes, we need to fix code to avoid claiming
> > > transactions by devices on the secondary bus while secondary bus is in
> > > RST# for both pci and express.
> > 
> > I guess the following fixes it?
> 
> Almost. The following if claouse also needs modification.
> 
> commit 120fe782a198b787e13a9d4681afae8dc66dee50
> Author: "Michael S. Tsirkin" <mst@redhat.com>
> Date:   Wed Nov 24 11:34:59 2010 +0900
> 
>     pci: don't access bus while it's reset
>     
>     Devices on a bus should not respond while RST#
>     is asserted, which is controlled by PCI_BRIDGE_CTL_BUS_RESET.
>     Simply skip such a bus when propagating the configuration cycle.
>     
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>     Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index d02f980..e7df612 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1554,7 +1554,9 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>  
>      /* try child bus */
>      if (!bus->parent_dev /* host pci bridge */ ||
> -        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> +        (!(pci_get_word(bus->parent_dev->config + PCI_BRIDGE_CONTROL) &
> +           PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
> +         bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
>           bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
>          for (; bus; bus = sec) {
>              QLIST_FOREACH(sec, &bus->child, sibling) {
> @@ -1562,8 +1564,11 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>                  if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
>                      return sec;
>                  }
> -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> -                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
> +                if (pci_get_word(sec->parent_dev->config +
> +                                 PCI_BRIDGE_CONTROL) & PCI_BRIDGE_CTL_BUS_RESET
> +                    /* Don't walk the bus if it's reset. */ ||
> +                    bus_num < sec->parent_dev->config[PCI_SECONDARY_BUS] ||
> +                    sec->parent_dev->config[PCI_SUBORDINATE_BUS] < bus_num) {
>                      break;
>                  }
>              }

Right. So let's add an inline helper to avoid code duplication here?

pci: fix bus walk under secondary bus reset

Take into account secondary bus reset bit for
bus walk: devices behind a reset bus should not
respond to configuration cycles.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


diff --git a/hw/pci.c b/hw/pci.c
index d02f980..0c15b13 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1540,6 +1540,16 @@ void pci_bridge_update_mappings(PCIBus *b)
     }
 }
 
+/* Whether a given bus number is in range of the secondary
+ * bus of the given bridge device. */
+static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
+{
+    return !(pci_get_word(dev->config + PCI_BRIDGE_CONTROL) &
+             PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
+        dev->config[PCI_SECONDARY_BUS] < bus_num &&
+        bus_num <= dev->config[PCI_SUBORDINATE_BUS];
+}
+
 PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 {
     PCIBus *sec;
@@ -1552,20 +1562,21 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
         return bus;
     }
 
+    /* Consider all bus numbers in range for the host pci bridge. */
+    if (bus->parent_dev &&
+        !pci_secondary_bus_in_range(bus->parent_dev, bus_num)) {
+        return NULL;
+    }
+
     /* try child bus */
-    if (!bus->parent_dev /* host pci bridge */ ||
-        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
-         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
-        for (; bus; bus = sec) {
-            QLIST_FOREACH(sec, &bus->child, sibling) {
-                assert(sec->parent_dev);
-                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
-                    return sec;
-                }
-                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
-                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
-                    break;
-                }
+    for (; bus; bus = sec) {
+        QLIST_FOREACH(sec, &bus->child, sibling) {
+            assert(sec->parent_dev);
+            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
+                return sec;
+            }
+            if (pci_secondary_bus_in_range(sec->parent_dev, bus_num)) {
+                break;
             }
         }
     }

> -- 
> yamahata

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Isaku Yamahata]

On Wed, Nov 24, 2010 at 07:27:58AM +0200, Michael S. Tsirkin wrote:
> Right. So let's add an inline helper to avoid code duplication here?
> 
> pci: fix bus walk under secondary bus reset
> 
> Take into account secondary bus reset bit for
> bus walk: devices behind a reset bus should not
> respond to configuration cycles.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index d02f980..0c15b13 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1540,6 +1540,16 @@ void pci_bridge_update_mappings(PCIBus *b)
>      }
>  }
>  
> +/* Whether a given bus number is in range of the secondary
> + * bus of the given bridge device. */
> +static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
> +{
> +    return !(pci_get_word(dev->config + PCI_BRIDGE_CONTROL) &
> +             PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
> +        dev->config[PCI_SECONDARY_BUS] < bus_num &&
> +        bus_num <= dev->config[PCI_SUBORDINATE_BUS];
> +}
> +
>  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>  {
>      PCIBus *sec;
> @@ -1552,20 +1562,21 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>          return bus;
>      }
>  
> +    /* Consider all bus numbers in range for the host pci bridge. */
> +    if (bus->parent_dev &&
> +        !pci_secondary_bus_in_range(bus->parent_dev, bus_num)) {
> +        return NULL;
> +    }
> +
>      /* try child bus */
> -    if (!bus->parent_dev /* host pci bridge */ ||
> -        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> -         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
> -        for (; bus; bus = sec) {
> -            QLIST_FOREACH(sec, &bus->child, sibling) {
> -                assert(sec->parent_dev);
> -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> -                    return sec;
> -                }
> -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> -                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
> -                    break;
> -                }
> +    for (; bus; bus = sec) {
> +        QLIST_FOREACH(sec, &bus->child, sibling) {
> +            assert(sec->parent_dev);
> +            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> +                return sec;
> +            }
> +            if (pci_secondary_bus_in_range(sec->parent_dev, bus_num)) {

This condition should be "if (!pci_...)"


> +                break;
>              }
>          }
>      }
> 
> > -- 
> > yamahata
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Michael S. Tsirkin]

On Wed, Nov 24, 2010 at 04:15:14PM +0900, Isaku Yamahata wrote:
> On Wed, Nov 24, 2010 at 07:27:58AM +0200, Michael S. Tsirkin wrote:
> > Right. So let's add an inline helper to avoid code duplication here?
> > 
> > pci: fix bus walk under secondary bus reset
> > 
> > Take into account secondary bus reset bit for
> > bus walk: devices behind a reset bus should not
> > respond to configuration cycles.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index d02f980..0c15b13 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1540,6 +1540,16 @@ void pci_bridge_update_mappings(PCIBus *b)
> >      }
> >  }
> >  
> > +/* Whether a given bus number is in range of the secondary
> > + * bus of the given bridge device. */
> > +static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
> > +{
> > +    return !(pci_get_word(dev->config + PCI_BRIDGE_CONTROL) &
> > +             PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
> > +        dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > +        bus_num <= dev->config[PCI_SUBORDINATE_BUS];
> > +}
> > +
> >  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> >  {
> >      PCIBus *sec;
> > @@ -1552,20 +1562,21 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> >          return bus;
> >      }
> >  
> > +    /* Consider all bus numbers in range for the host pci bridge. */
> > +    if (bus->parent_dev &&
> > +        !pci_secondary_bus_in_range(bus->parent_dev, bus_num)) {
> > +        return NULL;
> > +    }
> > +
> >      /* try child bus */
> > -    if (!bus->parent_dev /* host pci bridge */ ||
> > -        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > -         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
> > -        for (; bus; bus = sec) {
> > -            QLIST_FOREACH(sec, &bus->child, sibling) {
> > -                assert(sec->parent_dev);
> > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> > -                    return sec;
> > -                }
> > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > -                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
> > -                    break;
> > -                }
> > +    for (; bus; bus = sec) {
> > +        QLIST_FOREACH(sec, &bus->child, sibling) {
> > +            assert(sec->parent_dev);
> > +            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> > +                return sec;
> > +            }
> > +            if (pci_secondary_bus_in_range(sec->parent_dev, bus_num)) {
> 
> This condition should be "if (!pci_...)"

Why? We are looking for a device on the given bus that claims the given
bus_num.  If we find one, break out of the inner loop and walk down to
the child.

> 
> > +                break;
> >              }
> >          }
> >      }
> > 
> > > -- 
> > > yamahata
> > 
> 
> -- 
> yamahata

[Qemu-devel] Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

[Isaku Yamahata]

On Wed, Nov 24, 2010 at 12:59:43PM +0200, Michael S. Tsirkin wrote:
> > > @@ -1552,20 +1562,21 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> > >          return bus;
> > >      }
> > >  
> > > +    /* Consider all bus numbers in range for the host pci bridge. */
> > > +    if (bus->parent_dev &&
> > > +        !pci_secondary_bus_in_range(bus->parent_dev, bus_num)) {
> > > +        return NULL;
> > > +    }
> > > +
> > >      /* try child bus */
> > > -    if (!bus->parent_dev /* host pci bridge */ ||
> > > -        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > > -         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
> > > -        for (; bus; bus = sec) {
> > > -            QLIST_FOREACH(sec, &bus->child, sibling) {
> > > -                assert(sec->parent_dev);
> > > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> > > -                    return sec;
> > > -                }
> > > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > > -                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
> > > -                    break;
> > > -                }
> > > +    for (; bus; bus = sec) {
> > > +        QLIST_FOREACH(sec, &bus->child, sibling) {
> > > +            assert(sec->parent_dev);
> > > +            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> > > +                return sec;
> > > +            }
> > > +            if (pci_secondary_bus_in_range(sec->parent_dev, bus_num)) {
> > 
> > This condition should be "if (!pci_...)"
> 
> Why? We are looking for a device on the given bus that claims the given
> bus_num.  If we find one, break out of the inner loop and walk down to
> the child.

You're right. Sorry for noise.
-- 
yamahata