[Qemu-devel] [PATCH] Fix duplicate device reset

[Qemu-devel] [PATCH] Fix duplicate device reset

[Stefan Weil]

qbus_reset_all_fn was registered twice, so a lot of device reset
functions were also called twice when QEMU started.

It is sufficient to call sysbus_get_default() which will
register qbus_reset_all_fn.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index fcd7395..fb2f6db 100644
--- a/vl.c
+++ b/vl.c
@@ -3301,7 +3301,7 @@ int main(int argc, char **argv, char **envp)
 
     /* 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());
+    sysbus_get_default();
     qemu_run_machine_init_done_notifiers();
 
     qemu_system_reset(VMRESET_SILENT);
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] Fix duplicate device reset

[Isaku Yamahata]

Thank you for addressing this. Similar patches were proposed and
weren't merged unfortunately.

The reason why the qdev_register_reset() in vl.c is to keep the reset order.
The reset for main_system_bus shouldn't registered by qbus_create_inplace().
But the check, bus != main_system_bus, doesn't work as intended because
main_system_bus is NULL in early qdev creation.
So there are possible ways for the fix.

- Don't care the reset order
  your patch +
  remove "if (bus != main_system_bus)" in qbus_create_inplace()

- keep the reset order
  - instantiate main_system_bus early.
    So the check, bus != main_system_bus in qbus_create_inplace(), will work.
  or
  - fix the check, bus != main_system_bus in qbus_create_inplace(), somehow

thanks,

On Mon, Jul 18, 2011 at 10:22:26PM +0200, Stefan Weil wrote:
> qbus_reset_all_fn was registered twice, so a lot of device reset
> functions were also called twice when QEMU started.
> 
> It is sufficient to call sysbus_get_default() which will
> register qbus_reset_all_fn.
> 
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  vl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index fcd7395..fb2f6db 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3301,7 +3301,7 @@ int main(int argc, char **argv, char **envp)
>  
>      /* 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());
> +    sysbus_get_default();
>      qemu_run_machine_init_done_notifiers();
>  
>      qemu_system_reset(VMRESET_SILENT);
> -- 
> 1.7.2.5
> 

-- 
yamahata

Re: [Qemu-devel] [PATCH] Fix duplicate device reset

[Stefan Weil]

Am 19.07.2011 04:39, schrieb Isaku Yamahata:
> Thank you for addressing this. Similar patches were proposed and
> weren't merged unfortunately.
>
> The reason why the qdev_register_reset() in vl.c is to keep the reset order.
> The reset for main_system_bus shouldn't registered by qbus_create_inplace().
> But the check, bus != main_system_bus, doesn't work as intended because
> main_system_bus is NULL in early qdev creation.
> So there are possible ways for the fix.
>
> - Don't care the reset order
>    your patch +
>    remove "if (bus != main_system_bus)" in qbus_create_inplace()
>
> - keep the reset order
>    - instantiate main_system_bus early.
>      So the check, bus != main_system_bus in qbus_create_inplace(), will work.
>    or
>    - fix the check, bus != main_system_bus in qbus_create_inplace(), somehow
>
> thanks,

Hi,

my patch does not remove sysbus_get_default(),
so the reset order is kept because main_system_bus
is instantiated by this call.

Cheers,
Stefan

Re: [Qemu-devel] [PATCH] Fix duplicate device reset

[Isaku Yamahata]

On Tue, Jul 19, 2011 at 07:56:41AM +0200, Stefan Weil wrote:
> Am 19.07.2011 04:39, schrieb Isaku Yamahata:
>> Thank you for addressing this. Similar patches were proposed and
>> weren't merged unfortunately.
>>
>> The reason why the qdev_register_reset() in vl.c is to keep the reset order.
>> The reset for main_system_bus shouldn't registered by qbus_create_inplace().
>> But the check, bus != main_system_bus, doesn't work as intended because
>> main_system_bus is NULL in early qdev creation.
>> So there are possible ways for the fix.
>>
>> - Don't care the reset order
>>    your patch +
>>    remove "if (bus != main_system_bus)" in qbus_create_inplace()
>>
>> - keep the reset order
>>    - instantiate main_system_bus early.
>>      So the check, bus != main_system_bus in qbus_create_inplace(), will work.
>>    or
>>    - fix the check, bus != main_system_bus in qbus_create_inplace(), somehow
>>
>> thanks,
>
> Hi,
>
> my patch does not remove sysbus_get_default(),
> so the reset order is kept because main_system_bus
> is instantiated by this call.

Yes, your patch doesn't change the order from the existing code.
I think it's not intended one.
During machine creation, someone may call sysbus_get_default().
So the reset for main_system_bus may not be the lastly registered.

The changeset of 80376c3f tries to keep the reset order, but failed.
That's the issue.
-- 
yamahata

Re: [Qemu-devel] [PATCH] Fix duplicate device reset

[Isaku Yamahata]

Hi, here is the patch. Can you please give it a try? 

>From 41039df3174fa46477c4faf93d13eab360dccc22 Mon Sep 17 00:00:00 2001
Message-Id: <41039df3174fa46477c4faf93d13eab360dccc22.1312196365.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Mon, 1 Aug 2011 19:56:42 +0900
Subject: [PATCH] qdev: Fix duplicate reset

qbus_reset_all_fn was registered twice, so a lot of device reset
functions were also called twice when QEMU started.
Which was introduced by 80376c3fc2c38fdd45354e4b0eb45031f35587ed
This patch fixes it by making the main_sytem_bus creation not register
reset handler.

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

diff --git a/hw/qdev.c b/hw/qdev.c
index b4ea8e1..6819537 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -36,6 +36,7 @@ static bool qdev_hot_removed = false;
 
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
+static void main_system_bus_create(void);
 
 DeviceInfo *device_info_list;
 
@@ -328,8 +329,7 @@ static int qdev_reset_one(DeviceState *dev, void *opaque)
 BusState *sysbus_get_default(void)
 {
     if (!main_system_bus) {
-        main_system_bus = qbus_create(&system_bus_info, NULL,
-                                      "main-system-bus");
+        main_system_bus_create();
     }
     return main_system_bus;
 }
@@ -784,6 +784,16 @@ BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
     return bus;
 }
 
+static void main_system_bus_create(void)
+{
+    /* assign main_system_bus before qbus_create_inplace()
+     * in order to make "if (bus != main_system_bus)" work */
+    main_system_bus = qemu_mallocz(system_bus_info.size);
+    main_system_bus->qdev_allocated = 1;
+    qbus_create_inplace(main_system_bus, &system_bus_info, NULL,
+                        "main-system-bus");
+}
+
 void qbus_free(BusState *bus)
 {
     DeviceState *dev;
-- 
1.7.1.1



-- 
yamahata

Re: [Qemu-devel] [PATCH] Fix duplicate device reset

[Stefan Weil]

Am 01.08.2011 13:00, schrieb Isaku Yamahata:
>
> Hi, here is the patch. Can you please give it a try?
>
> From 41039df3174fa46477c4faf93d13eab360dccc22 Mon Sep 17 00:00:00 2001
> Message-Id: 
> <41039df3174fa46477c4faf93d13eab360dccc22.1312196365.git.yamahata@valinux.co.jp>
> From: Isaku Yamahata <yamahata@valinux.co.jp>
> Date: Mon, 1 Aug 2011 19:56:42 +0900
> Subject: [PATCH] qdev: Fix duplicate reset
>
> qbus_reset_all_fn was registered twice, so a lot of device reset
> functions were also called twice when QEMU started.
> Which was introduced by 80376c3fc2c38fdd45354e4b0eb45031f35587ed
> This patch fixes it by making the main_sytem_bus creation not register

main_system_bus

> reset handler.
>
> Cc: Stefan Weil <weil@mail.berlios.de>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> hw/qdev.c | 14 ++++++++++++--
> 1 files changed, 12 insertions(+), 2 deletions(-)

Thanks. I tested your patch with i386-softmmu (bios only)
and with mipsel-softmmu (debian boot / malta).

All registered reset functions were called only once,
and qbus_reset_all_fn was the last one called.

I noticed that there are two functions named piix3_reset.
One might be renamed to piix3_ide_reset, but this is
not related to your patch. There are also two functions
piix4_reset.

Tested-by: Stefan Weil <weil@mail.berlios.de>