[Qemu-devel] [PATCH] qbus: register reset handler for qbus whose parent is NULL

[Qemu-devel] [PATCH] qbus: register reset handler for qbus whose parent is NULL

[Isaku Yamahata]

Stefan Weil reported the regression caused by
ec990eb622ad46df5ddcb1e94c418c271894d416 as follows

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

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.
(On the other hand, if NULL is passed to qdev_create(), its parent bus
is main_system_bus.)
Ideally those buses should be moved under bus controller
device which is qdev. But it's not done yet.
So register qbus reset handler for qbus whose parent is NULL.

Reported-by: Stefan Weil <weil@mail.berlios.de>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Tested-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/qdev.c |    5 ++++-
 vl.c      |    2 ++
 2 files changed, 6 insertions(+), 1 deletions(-)

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

[Qemu-devel] Re: [PATCH] qbus: register reset handler for qbus whose parent is NULL

[Michael S. Tsirkin]

On Sun, Dec 19, 2010 at 10:22:50AM +0900, Isaku Yamahata wrote:
> Stefan Weil reported the regression caused by
> ec990eb622ad46df5ddcb1e94c418c271894d416 as follows
> 
> > 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).
> 
> 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.
> (On the other hand, if NULL is passed to qdev_create(), its parent bus
> is main_system_bus.)
> Ideally those buses should be moved under bus controller
> device which is qdev. But it's not done yet.
> So register qbus reset handler for qbus whose parent is NULL.
> 
> Reported-by: Stefan Weil <weil@mail.berlios.de>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Tested-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/qdev.c |    5 ++++-
>  vl.c      |    2 ++
>  2 files changed, 6 insertions(+), 1 deletions(-)
> 
> 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 */


Something like: 'TODO: remove the below once all bus devices are qdevified'?

> +        qemu_register_reset((void *)qbus_reset_all, bus);
>      }
> -
>  }
>  

Don't we need to unregister the reset on free?

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

all device is -> all system bus devices are

>      qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
>      qemu_run_machine_init_done_notifiers();


Actually the casts both here and above are pretty scary.
I think we need a wrapper that gets the proper arguments
and does the container_of properly.

>  
> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH] qbus: register reset handler for qbus whose parent is NULL

[Michael S. Tsirkin]

On Sun, Dec 19, 2010 at 10:22:50AM +0900, Isaku Yamahata wrote:
> Stefan Weil reported the regression caused by
> ec990eb622ad46df5ddcb1e94c418c271894d416 as follows
> 
> > 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).
> 
> 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.
> (On the other hand, if NULL is passed to qdev_create(), its parent bus
> is main_system_bus.)
> Ideally those buses should be moved under bus controller
> device which is qdev. But it's not done yet.
> So register qbus reset handler for qbus whose parent is NULL.
> 
> Reported-by: Stefan Weil <weil@mail.berlios.de>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Tested-by: Stefan Weil <weil@mail.berlios.de>

So - I think the following on top addresses the issues. Comments?
Compile-tested only.

diff --git a/hw/qdev.c b/hw/qdev.c
index 5dc20c8..e04bd4c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -328,8 +328,9 @@ void qdev_reset_all(DeviceState *dev)
     qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
 }
 
-void qbus_reset_all(BusState *bus)
+void qbus_reset_all(void *opaque)
 {
+    BusState *bus = opaque;
     qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
 }
 
@@ -755,9 +756,9 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
         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);
+        /* TODO: once all bus devices are qdevified,
+           only reset handler for main_system_bus should be registered here. */
+        qemu_register_reset(qbus_reset_all, bus);
     }
 }
 
@@ -781,6 +782,10 @@ void qbus_free(BusState *bus)
     if (bus->parent) {
         QLIST_REMOVE(bus, sibling);
         bus->parent->num_child_bus--;
+    } else {
+        /* TODO: once all bus devices are qdevified, only reset handler for
+         * main_system_bus should be unregistered here. */
+        qemu_unregister_reset(qbus_reset_all, bus);
     }
     qemu_free((void*)bus->name);
     if (bus->qdev_allocated) {
diff --git a/hw/qdev.h b/hw/qdev.h
index aaaf55a..1f0e02a 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -198,7 +198,7 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
 int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
                        qbus_walkerfn *busfn, void *opaque);
 void qdev_reset_all(DeviceState *dev);
-void qbus_reset_all(BusState *bus);
+void qbus_reset_all(void *opaque);
 void qbus_free(BusState *bus);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
diff --git a/vl.c b/vl.c
index 2ec3989..d5f094b 100644
--- a/vl.c
+++ b/vl.c
@@ -3088,9 +3088,9 @@ 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());
+    /* TODO: once all bus devices are qdevified, this should be done
+     * when bus is created by qdev.c */
+    qemu_register_reset(qbus_reset_all, sysbus_get_default());
     qemu_run_machine_init_done_notifiers();
 
     qemu_system_reset();

[Qemu-devel] Re: [PATCH] qbus: register reset handler for qbus whose parent is NULL

[Michael S. Tsirkin]

On Sun, Dec 19, 2010 at 03:19:22PM +0200, Michael S. Tsirkin wrote:
> On Sun, Dec 19, 2010 at 10:22:50AM +0900, Isaku Yamahata wrote:
> > Stefan Weil reported the regression caused by
> > ec990eb622ad46df5ddcb1e94c418c271894d416 as follows
> > 
> > > 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).
> > 
> > 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.
> > (On the other hand, if NULL is passed to qdev_create(), its parent bus
> > is main_system_bus.)
> > Ideally those buses should be moved under bus controller
> > device which is qdev. But it's not done yet.
> > So register qbus reset handler for qbus whose parent is NULL.
> > 
> > Reported-by: Stefan Weil <weil@mail.berlios.de>
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > Tested-by: Stefan Weil <weil@mail.berlios.de>
> 
> So - I think the following on top addresses the issues. Comments?
> Compile-tested only.

And maybe this on top:

diff --git a/hw/qdev.c b/hw/qdev.c
index e04bd4c..96aba18 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -782,7 +782,7 @@ void qbus_free(BusState *bus)
     if (bus->parent) {
         QLIST_REMOVE(bus, sibling);
         bus->parent->num_child_bus--;
-    } else {
+    } else if (bus != main_system_bus) {
         /* TODO: once all bus devices are qdevified, only reset handler for
          * main_system_bus should be unregistered here. */
         qemu_unregister_reset(qbus_reset_all, bus);

[Qemu-devel] Re: [PATCH] qbus: register reset handler for qbus whose parent is NULL

[Isaku Yamahata]

On Sun, Dec 19, 2010 at 03:24:32PM +0200, Michael S. Tsirkin wrote:
> On Sun, Dec 19, 2010 at 03:19:22PM +0200, Michael S. Tsirkin wrote:
> > On Sun, Dec 19, 2010 at 10:22:50AM +0900, Isaku Yamahata wrote:
> > > Stefan Weil reported the regression caused by
> > > ec990eb622ad46df5ddcb1e94c418c271894d416 as follows
> > > 
> > > > 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).
> > > 
> > > 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.
> > > (On the other hand, if NULL is passed to qdev_create(), its parent bus
> > > is main_system_bus.)
> > > Ideally those buses should be moved under bus controller
> > > device which is qdev. But it's not done yet.
> > > So register qbus reset handler for qbus whose parent is NULL.
> > > 
> > > Reported-by: Stefan Weil <weil@mail.berlios.de>
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > Tested-by: Stefan Weil <weil@mail.berlios.de>
> > 
> > So - I think the following on top addresses the issues. Comments?
> > Compile-tested only.
> 
> And maybe this on top:

I think main_system_bus is never freed. So assert(bus != main_system_bus)
is appropriate.


> diff --git a/hw/qdev.c b/hw/qdev.c
> index e04bd4c..96aba18 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -782,7 +782,7 @@ void qbus_free(BusState *bus)
>      if (bus->parent) {
>          QLIST_REMOVE(bus, sibling);
>          bus->parent->num_child_bus--;
> -    } else {
> +    } else if (bus != main_system_bus) {
>          /* TODO: once all bus devices are qdevified, only reset handler for
>           * main_system_bus should be unregistered here. */
>          qemu_unregister_reset(qbus_reset_all, bus);
> 

-- 
yamahata