[Qemu-devel] [PATCH] Register only one qbus_reset_all_fn() for system bus

[Qemu-devel] [PATCH] Register only one qbus_reset_all_fn() for system bus

[Dmitry Eremin-Solenikov]

Currently reset handler is registered for System bus twice: once during
bus creation and once in vl.c. Remove the second qemu_register_reset()
invocation. Also while we are at it, remove incorrect check at
qbus_create_inplace(): when system bus is created, main_system_bus is
NULL (as it's not yet created, it cannot be set), so the check is just
wrong.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 hw/qdev.c |    2 +-
 vl.c      |    3 ---
 2 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 1aa1ea0..0a3c8ce 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -762,7 +762,7 @@ 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) {
+    } else {
         /* TODO: once all bus devices are qdevified,
            only reset handler for main_system_bus should be registered here. */
         qemu_register_reset(qbus_reset_all_fn, bus);
diff --git a/vl.c b/vl.c
index 91be92e..24923db 100644
--- a/vl.c
+++ b/vl.c
@@ -3120,9 +3120,6 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    /* TODO: once all bus devices are qdevified, this should be done
-     * when bus is created by qdev.c */
-    qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
     qemu_run_machine_init_done_notifiers();
 
     qemu_system_reset();
-- 
1.7.2.3

[Qemu-devel] [PATCH] Register only one qbus_reset_all_fn() for system bus

[Dmitry Eremin-Solenikov]

Currently reset handler is registered for System bus twice: once during
bus creation and once in vl.c. Remove the second qemu_register_reset()
invocation. Also while we are at it, remove incorrect check at
qbus_create_inplace(): when system bus is created, main_system_bus is
NULL (as it's not yet created, it cannot be set), so the check is just
wrong.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 hw/qdev.c |    2 +-
 vl.c      |    3 ---
 2 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 1aa1ea0..0a3c8ce 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -762,7 +762,7 @@ 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) {
+    } else {
         /* TODO: once all bus devices are qdevified,
            only reset handler for main_system_bus should be registered here. */
         qemu_register_reset(qbus_reset_all_fn, bus);
diff --git a/vl.c b/vl.c
index 91be92e..24923db 100644
--- a/vl.c
+++ b/vl.c
@@ -3120,9 +3120,6 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    /* TODO: once all bus devices are qdevified, this should be done
-     * when bus is created by qdev.c */
-    qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
     qemu_run_machine_init_done_notifiers();
 
     qemu_system_reset();
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH] Register only one qbus_reset_all_fn() for system bus

[Aurelien Jarno]

On Mon, Feb 21, 2011 at 02:08:53AM +0300, Dmitry Eremin-Solenikov wrote:
> Currently reset handler is registered for System bus twice: once during
> bus creation and once in vl.c. Remove the second qemu_register_reset()
> invocation. Also while we are at it, remove incorrect check at
> qbus_create_inplace(): when system bus is created, main_system_bus is
> NULL (as it's not yet created, it cannot be set), so the check is just
> wrong.
> 
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
>  hw/qdev.c |    2 +-
>  vl.c      |    3 ---
>  2 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 1aa1ea0..0a3c8ce 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -762,7 +762,7 @@ 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) {
> +    } else {
>          /* TODO: once all bus devices are qdevified,
>             only reset handler for main_system_bus should be registered here. */
>          qemu_register_reset(qbus_reset_all_fn, bus);
> diff --git a/vl.c b/vl.c
> index 91be92e..24923db 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3120,9 +3120,6 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> -    /* TODO: once all bus devices are qdevified, this should be done
> -     * when bus is created by qdev.c */
> -    qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
>      qemu_run_machine_init_done_notifiers();
>  
>      qemu_system_reset();

Have you verified that all bus devices have been qdevified since this
code has been added? I wouldn't bet it is the case.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] Register only one qbus_reset_all_fn() for system bus

[Isaku Yamahata]

> Have you verified that all bus devices have been qdevified since this
> code has been added? I wouldn't bet it is the case.

I think his analysis is valid. So how about the following patch.

>From ee27041a238d51247e30100d1909066978cd8858 Mon Sep 17 00:00:00 2001
Message-Id: <ee27041a238d51247e30100d1909066978cd8858.1301703026.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Sat, 2 Apr 2011 09:04:54 +0900
Subject: [PATCH] qdev: Register only one qbus_reset_all_fn() for system bus

This is the revised version of Dmitry's.
his report is as follows and this patch fixes it.
> Currently reset handler is registered for System bus twice: once during
> bus creation and once in vl.c. Remove the second qemu_register_reset()
> invocation. Also while we are at it, remove incorrect check at
> qbus_create_inplace(): when system bus is created, main_system_bus is
> NULL (as it's not yet created, it cannot be set), so the check is just
> wrong.

the check bus == main_system_bus in qbus_create_inplace() was wrong
because sysbus_get_default() creates bus for main_system_bus and then
set main_system_bus to it.
So main_system_bus == NULL in qbus_create_inplace() when creating
main_system_bus. So this patch fixes the check whether creating
main_system_bus or not by seeing BusInfo.

Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/qdev.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 1aa1ea0..659de23 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -762,7 +762,12 @@ 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) {
+    } else if (info != &system_bus_info) {
+        /* see if bus == main_system_bus
+         * Here we can't check bus == main_system_bus because
+         * main_system_bus == NULL here before setting it by
+         * sysbus_get_default()
+         */
         /* TODO: once all bus devices are qdevified,
            only reset handler for main_system_bus should be registered here. */
         qemu_register_reset(qbus_reset_all_fn, bus);
-- 
1.7.1.1



-- 
yamahata

Re: [Qemu-devel] [PATCH] Register only one qbus_reset_all_fn() for system bus

[Dmitry Eremin-Solenikov]

On 4/2/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
>> Have you verified that all bus devices have been qdevified since this
>> code has been added? I wouldn't bet it is the case.
>
> I think his analysis is valid. So how about the following patch.

Could you please point me to an example of devices for which this check is
required.

> From ee27041a238d51247e30100d1909066978cd8858 Mon Sep 17 00:00:00 2001
> Message-Id:
> <ee27041a238d51247e30100d1909066978cd8858.1301703026.git.yamahata@valinux.co.jp>
> From: Isaku Yamahata <yamahata@valinux.co.jp>
> Date: Sat, 2 Apr 2011 09:04:54 +0900
> Subject: [PATCH] qdev: Register only one qbus_reset_all_fn() for system bus
>
> This is the revised version of Dmitry's.
> his report is as follows and this patch fixes it.
>> Currently reset handler is registered for System bus twice: once during
>> bus creation and once in vl.c. Remove the second qemu_register_reset()
>> invocation. Also while we are at it, remove incorrect check at
>> qbus_create_inplace(): when system bus is created, main_system_bus is
>> NULL (as it's not yet created, it cannot be set), so the check is just
>> wrong.
>
> the check bus == main_system_bus in qbus_create_inplace() was wrong
> because sysbus_get_default() creates bus for main_system_bus and then
> set main_system_bus to it.
> So main_system_bus == NULL in qbus_create_inplace() when creating
> main_system_bus. So this patch fixes the check whether creating
> main_system_bus or not by seeing BusInfo.
>
> Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/qdev.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 1aa1ea0..659de23 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -762,7 +762,12 @@ 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) {
> +    } else if (info != &system_bus_info) {
> +        /* see if bus == main_system_bus
> +         * Here we can't check bus == main_system_bus because
> +         * main_system_bus == NULL here before setting it by
> +         * sysbus_get_default()
> +         */
>          /* TODO: once all bus devices are qdevified,
>             only reset handler for main_system_bus should be registered
> here. */
>          qemu_register_reset(qbus_reset_all_fn, bus);
> --
> 1.7.1.1
>
>
>
> --
> yamahata
>


-- 
With best wishes
Dmitry

Re: [Qemu-devel] [PATCH] Register only one qbus_reset_all_fn() for system bus

[Isaku Yamahata]

On Sat, Apr 02, 2011 at 06:47:37PM +0400, Dmitry Eremin-Solenikov wrote:
> On 4/2/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> >> Have you verified that all bus devices have been qdevified since this
> >> code has been added? I wouldn't bet it is the case.
> >
> > I think his analysis is valid. So how about the following patch.
> 
> Could you please point me to an example of devices for which this check is
> required.

Although I don't have any example, I bet to not change the reset order.
If you check all the devices, it's good.
-- 
yamahata

Re: [Qemu-devel] [PATCH] Register only one qbus_reset_all_fn() for system bus

[Dmitry Eremin-Solenikov]

On 4/3/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> On Sat, Apr 02, 2011 at 06:47:37PM +0400, Dmitry Eremin-Solenikov wrote:
>> On 4/2/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
>> >> Have you verified that all bus devices have been qdevified since this
>> >> code has been added? I wouldn't bet it is the case.
>> >
>> > I think his analysis is valid. So how about the following patch.
>>
>> Could you please point me to an example of devices for which this check is
>> required.
>
> Although I don't have any example, I bet to not change the reset order.
> If you check all the devices, it's good.

The question is which devices to check as lots of devices are already
converted to qdev. Is it correct that we should check only devices
which register a child bus with parent device set, and the thing that we
should check is the fact that the parent reset function also causes
the bus reset?

-- 
With best wishes
Dmitry

Re: [Qemu-devel] [PATCH] Register only one qbus_reset_all_fn() for system bus

[Isaku Yamahata]

On Mon, Apr 04, 2011 at 03:58:39PM +0400, Dmitry Eremin-Solenikov wrote:
> On 4/3/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> > On Sat, Apr 02, 2011 at 06:47:37PM +0400, Dmitry Eremin-Solenikov wrote:
> >> On 4/2/11, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> >> >> Have you verified that all bus devices have been qdevified since this
> >> >> code has been added? I wouldn't bet it is the case.
> >> >
> >> > I think his analysis is valid. So how about the following patch.
> >>
> >> Could you please point me to an example of devices for which this check is
> >> required.
> >
> > Although I don't have any example, I bet to not change the reset order.
> > If you check all the devices, it's good.
> 
> The question is which devices to check as lots of devices are already
> converted to qdev. Is it correct that we should check only devices
> which register a child bus with parent device set, and the thing that we
> should check is the fact that the parent reset function also causes
> the bus reset?

qbus whose parent is NULL, non-qdev devices and qdev devices which
uses qemu_register_reset() instead of DeviceInfo::reset.
-- 
yamahata