Register only one qbus_reset_all_fn() for system bus
PATCH 1/1 Register only one qbus_reset_all_fn() for system bus
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.
--- hw/qdev.c | 2 +- vl.c | 3 --- 2 files changed, 1 insertions(+), 4 deletions(-)
hw/qdev.c +1 -1
@@ -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);
vl.c +0 -3
@@ -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 ();
Signature · 1 line
1.7.2.3
Quoted 31 lines · Dmitry Eremin-Solenikov
Currently reset handler is registered for System bus twice: once during bus creation and once in
vl.c. Remove the secondqemu_register_reset()invocation. Also while we are at it, remove incorrect check atqbus_create_inplace(): when system bus is created,main_system_busis NULL (as it's not yet created, it cannot be set), so the check is just wrong.Signed-off-by Dmitry Eremin-Solenikov <[email protected]>--- hw/qdev.c | 2 +- vl.c | 3 --- 2 files changed, 1 insertions(+), 4 deletions(-)hw/qdev.c +1
-1@@ -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);vl.c +0
-0
vl.c int main(int argc, char **argv, char **envp) · line 3125
Quoted 9 lines
vl.c +0
-3@@ -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.
Signature · 2 lines
Aurelien Jarno GPG: 1024D/F1BCDB73 [email protected] http://www.aurel32.net
Quoted 2 lines
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.
Quoted 1 line
From
ee27041a238d51247e30100d1909066978cd8858Mon Sep 17 00:00:00 2001
Message-Id: <ee27041a238d51247e30100d1909066978cd8858.1301703026.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <[email protected]>
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.
Quoted 6 lines
Currently reset handler is registered for System bus twice: once during bus creation and once in
vl.c. Remove the secondqemu_register_reset()invocation. Also while we are at it, remove incorrect check atqbus_create_inplace(): when system bus is created,main_system_busis 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.
--- hw/qdev.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
hw/qdev.c +6 -1
@@ -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);
Signature · 6 lines
1.7.1.1 -- yamahata
Quoted 4 lines · Isaku Yamahata
Quoted 2 lines
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.
Quoted 56 lines
From
ee27041a238d51247e30100d1909066978cd8858Mon Sep 17 00:00:00 2001 Message-Id:
<ee27041a238d51247e30100d1909066978cd8858.1301703026.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <[email protected]>
Date: Sat, 2 Apr 2011 09:04:54 +0900
Subject: [PATCH] qdev: Register only oneqbus_reset_all_fn()for system busThis is the revised version of Dmitry's.
his report is as follows and this patch fixes it.Quoted 6 lines
Currently reset handler is registered for System bus twice: once during bus creation and once in
vl.c. Remove the secondqemu_register_reset()invocation. Also while we are at it, remove incorrect check atqbus_create_inplace(): when system bus is created,main_system_busis NULL (as it's not yet created, it cannot be set), so the check is just wrong.the check bus ==
main_system_businqbus_create_inplace()was wrong becausesysbus_get_default()creates bus formain_system_busand then setmain_system_busto it.
Somain_system_bus== NULL inqbus_create_inplace()when creatingmain_system_bus. So this patch fixes the check whether creatingmain_system_busor not by seeing BusInfo.Cc Dmitry Eremin-Solenikov <[email protected]>Signed-off-by Isaku Yamahata <[email protected]>--- hw/qdev.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)hw/qdev.c +6
-1@@ -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
Signature · 2 lines
With best wishes Dmitry
Quoted 8 lines · Dmitry Eremin-Solenikov
Quoted 4 lines · Isaku Yamahata
Quoted 2 lines
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.
Signature · 1 line
yamahata
Quoted 5 lines · Isaku Yamahata
Quoted 4 lines · Dmitry Eremin-Solenikov
Quoted 3 lines · Isaku Yamahata
Quoted 2 lines
Have you verified that all bus devices have been qdevified since this code has been added? I wouldn't bet it is the case.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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?
Signature · 2 lines
With best wishes Dmitry
Quoted 11 lines · Dmitry Eremin-Solenikov
Quoted 10 lines · Isaku Yamahata
Quoted 8 lines · Dmitry Eremin-Solenikov
Quoted 4 lines · Isaku Yamahata
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
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.
Signature · 1 line
yamahata