[Qemu-devel] [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Qemu-devel] [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Isaku Yamahata]

This patch set distinguish warm reset from cold reset by
introducing warm reset callback handler.
The first 4 patches are trivial clean up patches. The last patch of 5/5
is RFC patch.

The following thread arose cold reset vs warm reset issues.
http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00186.html
The summary is
- warm reset is wanted in qemu
  - Pressing the reset button is a warm reset on real machines
  - Sparc64 CPU uses different reset vector for warm and cold reset,
    so system_reset acts like a reset button
  - Bus reset can be implemented utilizing qdev frame work instead of
    implemeting it each bus layer independently.
- The modification should be incremental.
  Anthony would like to see that as an incremental addition to what we have
  today (like introducing a propagating warm reset callback) and thinking
  through what the actual behavior should and shouldn't be.


If the direction is okay, The next step would be a patch(set) for qdev which
would introduce qdev_cold_reset(), qdev_warm_reset(),
DeviceInfo::cold_reset and DeviceInfo::warm_reset
and would obsolete qdev_reset() and DeviceInfo::reset.


Isaku Yamahata (5):
  sysemu.h, vl.c: static'fy qemu_xxx_requested().
  vl.c: consolidate qemu_xxx_requested() logic.
  vl.c: consolidate qemu_system_xxx_request() logic.
  vl.c: factor out qemu_reguster/unregister_reset().
  RFC: distinguish warm reset from cold reset.

 hw/hw.h  |    7 +++
 sysemu.h |    8 ++--
 vl.c     |  163 ++++++++++++++++++++++++++++++++++++++++++++++----------------
 3 files changed, 133 insertions(+), 45 deletions(-)

[Qemu-devel] [PATCH 1/5] sysemu.h, vl.c: static'fy qemu_xxx_requested().

[Isaku Yamahata]

Make qemu_xxx_requested() helper function static
because they aren't used outside from vl.c.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 sysemu.h |    4 ----
 vl.c     |    8 ++++----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/sysemu.h b/sysemu.h
index a1f6466..7fc4e20 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -51,11 +51,7 @@ void cpu_disable_ticks(void);
 void qemu_system_reset_request(void);
 void qemu_system_shutdown_request(void);
 void qemu_system_powerdown_request(void);
-int qemu_shutdown_requested(void);
-int qemu_reset_requested(void);
-int qemu_powerdown_requested(void);
 extern qemu_irq qemu_system_powerdown;
-void qemu_system_reset(void);
 
 void qemu_add_exit_notifier(Notifier *notify);
 void qemu_remove_exit_notifier(Notifier *notify);
diff --git a/vl.c b/vl.c
index 91d1684..a77a171 100644
--- a/vl.c
+++ b/vl.c
@@ -1129,21 +1129,21 @@ static int powerdown_requested;
 int debug_requested;
 int vmstop_requested;
 
-int qemu_shutdown_requested(void)
+static int qemu_shutdown_requested(void)
 {
     int r = shutdown_requested;
     shutdown_requested = 0;
     return r;
 }
 
-int qemu_reset_requested(void)
+static int qemu_reset_requested(void)
 {
     int r = reset_requested;
     reset_requested = 0;
     return r;
 }
 
-int qemu_powerdown_requested(void)
+static int qemu_powerdown_requested(void)
 {
     int r = powerdown_requested;
     powerdown_requested = 0;
@@ -1186,7 +1186,7 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
     }
 }
 
-void qemu_system_reset(void)
+static void qemu_system_reset(void)
 {
     QEMUResetEntry *re, *nre;
 
-- 
1.7.1.1

[Qemu-devel] [PATCH 2/5] vl.c: consolidate qemu_xxx_requested() logic.

[Isaku Yamahata]

Don't repeat same logic in qemu_xxx_requested().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 vl.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/vl.c b/vl.c
index a77a171..aba3786 100644
--- a/vl.c
+++ b/vl.c
@@ -1129,39 +1129,36 @@ static int powerdown_requested;
 int debug_requested;
 int vmstop_requested;
 
-static int qemu_shutdown_requested(void)
+static int qemu_requested(int *requested)
 {
-    int r = shutdown_requested;
-    shutdown_requested = 0;
+    int r = *requested;
+    *requested = 0;
     return r;
 }
 
+static int qemu_shutdown_requested(void)
+{
+    return qemu_requested(&shutdown_requested);
+}
+
 static int qemu_reset_requested(void)
 {
-    int r = reset_requested;
-    reset_requested = 0;
-    return r;
+    return qemu_requested(&reset_requested);
 }
 
 static int qemu_powerdown_requested(void)
 {
-    int r = powerdown_requested;
-    powerdown_requested = 0;
-    return r;
+    return qemu_requested(&powerdown_requested);
 }
 
 static int qemu_debug_requested(void)
 {
-    int r = debug_requested;
-    debug_requested = 0;
-    return r;
+    return qemu_requested(&debug_requested);
 }
 
 static int qemu_vmstop_requested(void)
 {
-    int r = vmstop_requested;
-    vmstop_requested = 0;
-    return r;
+    return qemu_requested(&vmstop_requested);
 }
 
 void qemu_register_reset(QEMUResetHandler *func, void *opaque)
-- 
1.7.1.1

[Qemu-devel] [PATCH 3/5] vl.c: consolidate qemu_system_xxx_request() logic.

[Isaku Yamahata]

don't repeat same logic in qemu_system_xxx_request() logic.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 vl.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/vl.c b/vl.c
index aba3786..2a89f4f 100644
--- a/vl.c
+++ b/vl.c
@@ -1195,26 +1195,33 @@ static void qemu_system_reset(void)
     cpu_synchronize_all_post_reset();
 }
 
-void qemu_system_reset_request(void)
+static void qemu_system_request(int *requested)
+{
+    *requested = 1;
+    qemu_notify_event();
+}
+
+static void qemu_system_request_reboot_check(int *requested)
 {
     if (no_reboot) {
-        shutdown_requested = 1;
-    } else {
-        reset_requested = 1;
+        requested = &shutdown_requested;
     }
-    qemu_notify_event();
+    qemu_system_request(requested);
+}
+
+void qemu_system_reset_request(void)
+{
+    qemu_system_request_reboot_check(&reset_requested);
 }
 
 void qemu_system_shutdown_request(void)
 {
-    shutdown_requested = 1;
-    qemu_notify_event();
+    qemu_system_request(&shutdown_requested);
 }
 
 void qemu_system_powerdown_request(void)
 {
-    powerdown_requested = 1;
-    qemu_notify_event();
+    qemu_system_request(&powerdown_requested);
 }
 
 void main_loop_wait(int nonblocking)
-- 
1.7.1.1

[Qemu-devel] [PATCH 4/5] vl.c: factor out qemu_reguster/unregister_reset().

[Isaku Yamahata]

factor out qemu_reguster/unregister_reset() for later use.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 vl.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/vl.c b/vl.c
index 2a89f4f..a919a32 100644
--- a/vl.c
+++ b/vl.c
@@ -1121,7 +1121,8 @@ typedef struct QEMUResetEntry {
     void *opaque;
 } QEMUResetEntry;
 
-static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
+QTAILQ_HEAD(reset_handlers, QEMUResetEntry);
+static struct reset_handlers reset_handlers =
     QTAILQ_HEAD_INITIALIZER(reset_handlers);
 static int reset_requested;
 static int shutdown_requested;
@@ -1161,36 +1162,53 @@ static int qemu_vmstop_requested(void)
     return qemu_requested(&vmstop_requested);
 }
 
-void qemu_register_reset(QEMUResetHandler *func, void *opaque)
+static void qemu_register_reset_handler(QEMUResetHandler *func, void *opaque,
+                                        struct reset_handlers *handlers)
 {
     QEMUResetEntry *re = qemu_mallocz(sizeof(QEMUResetEntry));
 
     re->func = func;
     re->opaque = opaque;
-    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
+    QTAILQ_INSERT_TAIL(handlers, re, entry);
 }
 
-void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
+static void qemu_unregister_reset_handler(QEMUResetHandler *func, void *opaque,
+                                          struct reset_handlers *handlers)
 {
     QEMUResetEntry *re;
 
-    QTAILQ_FOREACH(re, &reset_handlers, entry) {
+    QTAILQ_FOREACH(re, handlers, entry) {
         if (re->func == func && re->opaque == opaque) {
-            QTAILQ_REMOVE(&reset_handlers, re, entry);
+            QTAILQ_REMOVE(handlers, re, entry);
             qemu_free(re);
             return;
         }
     }
 }
 
-static void qemu_system_reset(void)
+static void qemu_system_reset_handler(struct reset_handlers *handlers)
 {
     QEMUResetEntry *re, *nre;
 
     /* reset all devices */
-    QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
+    QTAILQ_FOREACH_SAFE(re, handlers, entry, nre) {
         re->func(re->opaque);
     }
+}
+
+void qemu_register_reset(QEMUResetHandler *func, void *opaque)
+{
+    qemu_register_reset_handler(func, opaque, &reset_handlers);
+}
+
+void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
+{
+    qemu_unregister_reset_handler(func, opaque, &reset_handlers);
+}
+
+static void qemu_system_reset(void)
+{
+    qemu_system_reset_handler(&reset_handlers);
     monitor_protocol_event(QEVENT_RESET, NULL);
     cpu_synchronize_all_post_reset();
 }
-- 
1.7.1.1

[Qemu-devel] [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Isaku Yamahata]

Distinguish warm reset from cold reset by introducing
cold/warm reset helper function instead of single reset routines.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/hw.h  |    7 +++++
 sysemu.h |    4 +++
 vl.c     |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 85 insertions(+), 15 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 4405092..6fb844e 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -269,6 +269,13 @@ void register_device_unmigratable(DeviceState *dev, const char *idstr,
 
 typedef void QEMUResetHandler(void *opaque);
 
+
+void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque);
+void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque);
+void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque);
+void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque);
+
+/* those two functions are obsoleted by cold/warm reset API. */
 void qemu_register_reset(QEMUResetHandler *func, void *opaque);
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
 
diff --git a/sysemu.h b/sysemu.h
index 7fc4e20..927892a 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -48,7 +48,11 @@ int64_t cpu_get_ticks(void);
 void cpu_enable_ticks(void);
 void cpu_disable_ticks(void);
 
+/* transitional compat = qemu_system_warm_reset_request() */
 void qemu_system_reset_request(void);
+
+void qemu_system_cold_reset_request(void);
+void qemu_system_warm_reset_request(void);
 void qemu_system_shutdown_request(void);
 void qemu_system_powerdown_request(void);
 extern qemu_irq qemu_system_powerdown;
diff --git a/vl.c b/vl.c
index a919a32..609d74c 100644
--- a/vl.c
+++ b/vl.c
@@ -1122,9 +1122,13 @@ typedef struct QEMUResetEntry {
 } QEMUResetEntry;
 
 QTAILQ_HEAD(reset_handlers, QEMUResetEntry);
-static struct reset_handlers reset_handlers =
-    QTAILQ_HEAD_INITIALIZER(reset_handlers);
-static int reset_requested;
+
+static struct reset_handlers cold_reset_handlers =
+    QTAILQ_HEAD_INITIALIZER(cold_reset_handlers);
+static struct reset_handlers warm_reset_handlers =
+    QTAILQ_HEAD_INITIALIZER(warm_reset_handlers);
+static int cold_reset_requested;
+static int warm_reset_requested;
 static int shutdown_requested;
 static int powerdown_requested;
 int debug_requested;
@@ -1142,9 +1146,14 @@ static int qemu_shutdown_requested(void)
     return qemu_requested(&shutdown_requested);
 }
 
-static int qemu_reset_requested(void)
+static int qemu_cold_reset_requested(void)
+{
+    return qemu_requested(&cold_reset_requested);
+}
+
+static int qemu_warm_reset_requested(void)
 {
-    return qemu_requested(&reset_requested);
+    return qemu_requested(&warm_reset_requested);
 }
 
 static int qemu_powerdown_requested(void)
@@ -1196,20 +1205,51 @@ static void qemu_system_reset_handler(struct reset_handlers *handlers)
     }
 }
 
+/* obsolated by qemu_register_cold/warm_reset() */
 void qemu_register_reset(QEMUResetHandler *func, void *opaque)
 {
-    qemu_register_reset_handler(func, opaque, &reset_handlers);
+    qemu_register_cold_reset(func, opaque);
+    qemu_register_warm_reset(func, opaque);
 }
 
+/* obsolated by qemu_unregister_cold/warm_reset() */
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
 {
-    qemu_unregister_reset_handler(func, opaque, &reset_handlers);
+    qemu_unregister_cold_reset(func, opaque);
+    qemu_unregister_warm_reset(func, opaque);
+}
+
+void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque)
+{
+    qemu_register_reset_handler(func, opaque, &cold_reset_handlers);
+}
+
+void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque)
+{
+    qemu_unregister_reset_handler(func, opaque, &cold_reset_handlers);
+}
+
+static void qemu_system_cold_reset(void)
+{
+    qemu_system_reset_handler(&cold_reset_handlers);
+    monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_COLD? */
+    cpu_synchronize_all_post_reset();
+}
+
+void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque)
+{
+    qemu_register_reset_handler(func, opaque, &warm_reset_handlers);
+}
+
+void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque)
+{
+    qemu_unregister_reset_handler(func, opaque, &warm_reset_handlers);
 }
 
-static void qemu_system_reset(void)
+static void qemu_system_warm_reset(void)
 {
-    qemu_system_reset_handler(&reset_handlers);
-    monitor_protocol_event(QEVENT_RESET, NULL);
+    qemu_system_reset_handler(&warm_reset_handlers);
+    monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_WARM? */
     cpu_synchronize_all_post_reset();
 }
 
@@ -1227,9 +1267,20 @@ static void qemu_system_request_reboot_check(int *requested)
     qemu_system_request(requested);
 }
 
+void qemu_system_cold_reset_request(void)
+{
+    qemu_system_request_reboot_check(&cold_reset_requested);
+}
+
+void qemu_system_warm_reset_request(void)
+{
+    qemu_system_request_reboot_check(&warm_reset_requested);
+}
+
+/* trantitional compat */
 void qemu_system_reset_request(void)
 {
-    qemu_system_request_reboot_check(&reset_requested);
+    qemu_system_request_reboot_check(&warm_reset_requested);
 }
 
 void qemu_system_shutdown_request(void)
@@ -1322,7 +1373,9 @@ static int vm_can_run(void)
 {
     if (powerdown_requested)
         return 0;
-    if (reset_requested)
+    if (cold_reset_requested)
+        return 0;
+    if (warm_reset_requested)
         return 0;
     if (shutdown_requested)
         return 0;
@@ -1368,9 +1421,15 @@ static void main_loop(void)
             } else
                 break;
         }
-        if (qemu_reset_requested()) {
+        if (qemu_warm_reset_requested()) {
+            pause_all_vcpus();
+            qemu_system_warm_reset();
+            resume_all_vcpus();
+        }
+        if (qemu_cold_reset_requested()) {
+            /* power cycle */
             pause_all_vcpus();
-            qemu_system_reset();
+            qemu_system_cold_reset();
             resume_all_vcpus();
         }
         if (qemu_powerdown_requested()) {
@@ -2992,7 +3051,7 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    qemu_system_reset();
+    qemu_system_cold_reset();
     if (loadvm) {
         if (load_vmstate(loadvm) < 0) {
             autostart = 0;
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Avi Kivity]

  On 08/30/2010 10:49 AM, Isaku Yamahata wrote:
> This patch set distinguish warm reset from cold reset by
> introducing warm reset callback handler.
> The first 4 patches are trivial clean up patches. The last patch of 5/5
> is RFC patch.
>
> The following thread arose cold reset vs warm reset issues.
> http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00186.html
> The summary is
> - warm reset is wanted in qemu
>    - Pressing the reset button is a warm reset on real machines
>    - Sparc64 CPU uses different reset vector for warm and cold reset,
>      so system_reset acts like a reset button
>    - Bus reset can be implemented utilizing qdev frame work instead of
>      implemeting it each bus layer independently.
> - The modification should be incremental.
>    Anthony would like to see that as an incremental addition to what we have
>    today (like introducing a propagating warm reset callback) and thinking
>    through what the actual behavior should and shouldn't be.
>
>
> If the direction is okay, The next step would be a patch(set) for qdev which
> would introduce qdev_cold_reset(), qdev_warm_reset(),
> DeviceInfo::cold_reset and DeviceInfo::warm_reset
> and would obsolete qdev_reset() and DeviceInfo::reset.
>

What would be the difference between warm and cold reset?  Former called 
on any reset, while the latter called on power up only?

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Isaku Yamahata]

On Mon, Aug 30, 2010 at 10:59:19AM +0300, Avi Kivity wrote:
>  On 08/30/2010 10:49 AM, Isaku Yamahata wrote:
>> This patch set distinguish warm reset from cold reset by
>> introducing warm reset callback handler.
>> The first 4 patches are trivial clean up patches. The last patch of 5/5
>> is RFC patch.
>>
>> The following thread arose cold reset vs warm reset issues.
>> http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00186.html
>> The summary is
>> - warm reset is wanted in qemu
>>    - Pressing the reset button is a warm reset on real machines
>>    - Sparc64 CPU uses different reset vector for warm and cold reset,
>>      so system_reset acts like a reset button
>>    - Bus reset can be implemented utilizing qdev frame work instead of
>>      implemeting it each bus layer independently.
>> - The modification should be incremental.
>>    Anthony would like to see that as an incremental addition to what we have
>>    today (like introducing a propagating warm reset callback) and thinking
>>    through what the actual behavior should and shouldn't be.
>>
>>
>> If the direction is okay, The next step would be a patch(set) for qdev which
>> would introduce qdev_cold_reset(), qdev_warm_reset(),
>> DeviceInfo::cold_reset and DeviceInfo::warm_reset
>> and would obsolete qdev_reset() and DeviceInfo::reset.
>>
>
> What would be the difference between warm and cold reset?  Former called  
> on any reset, while the latter called on power up only?

What I have in mind at the moment is,
warm reset callback is called on warm reset, not called on power up.
cold reset callback is called only on power up (and power cycle).

Hmm, should warm reset handler be called on power up?
Each cold reset callbacks can call corresponding warm reset handler
if necessary. So it would be redundant to call qdev_warm_reset() on power up.
Or would it be more robust to call warm reset in addition to cold reset
on power on?

Anyway, it should be documented. I'll add the comment on the next spin.
-- 
yamahata

[Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Paolo Bonzini]

On 08/30/2010 09:49 AM, Isaku Yamahata wrote:
> +/* those two functions are obsoleted by cold/warm reset API. */
> [qemu_register_reset/qemu_unregister_reset]

Are they?

They have a _lot_ of callers and most of the time you do not really care 
about cold vs. warm reset.  So, I think either you define a new API 
where you can request cold reset/warm reset/both, or qemu_register_reset 
is here to stay forever.

In general, I don't like the duplication you introduce between cold 
reset, warm reset, shutdown, powerdown, etc.  Maybe you can introduce a 
new "VMEvent" abstraction with functions like "request", "is requested", 
"register handler"?

It could also be interesting to convert everything to the Notifier API, 
if someone wants to play with Coccinelle...

Paolo

[Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Paolo Bonzini]

On 08/30/2010 11:38 AM, Isaku Yamahata wrote:
> Sounds good idea. I'll give it a try.
> Before touching the code, I'd like to split out those related functions
> and main_loop() from vl.c into a new file, main-loop.c or something like that.
> Any objection for splitting out vl.c for that?

That would be good.  Most if not all of cpus.c could go in the new file too.

Thanks!

Paolo

[Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Isaku Yamahata]

Thank you for comments.

On Mon, Aug 30, 2010 at 10:50:47AM +0200, Paolo Bonzini wrote:
> On 08/30/2010 09:49 AM, Isaku Yamahata wrote:
>> +/* those two functions are obsoleted by cold/warm reset API. */
>> [qemu_register_reset/qemu_unregister_reset]
>
> Are they?
>
> They have a _lot_ of callers and most of the time you do not really care  
> about cold vs. warm reset.  So, I think either you define a new API  
> where you can request cold reset/warm reset/both, or qemu_register_reset  
> is here to stay forever.

Then, let's keep qemu_register_reset() as for both cold/warm with
documentation/comments.


> In general, I don't like the duplication you introduce between cold  
> reset, warm reset, shutdown, powerdown, etc.  Maybe you can introduce a  
> new "VMEvent" abstraction with functions like "request", "is requested",  
> "register handler"?

Sounds good idea. I'll give it a try.
Before touching the code, I'd like to split out those related functions
and main_loop() from vl.c into a new file, main-loop.c or something like that.
Any objection for splitting out vl.c for that?


> It could also be interesting to convert everything to the Notifier API,  
> if someone wants to play with Coccinelle...
>
> Paolo
>

-- 
yamahata

Re: [Qemu-devel] Re: [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Gleb Natapov]

On Mon, Aug 30, 2010 at 05:35:20PM +0900, Isaku Yamahata wrote:
> On Mon, Aug 30, 2010 at 10:59:19AM +0300, Avi Kivity wrote:
> >  On 08/30/2010 10:49 AM, Isaku Yamahata wrote:
> >> This patch set distinguish warm reset from cold reset by
> >> introducing warm reset callback handler.
> >> The first 4 patches are trivial clean up patches. The last patch of 5/5
> >> is RFC patch.
> >>
> >> The following thread arose cold reset vs warm reset issues.
> >> http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00186.html
> >> The summary is
> >> - warm reset is wanted in qemu
> >>    - Pressing the reset button is a warm reset on real machines
> >>    - Sparc64 CPU uses different reset vector for warm and cold reset,
> >>      so system_reset acts like a reset button
> >>    - Bus reset can be implemented utilizing qdev frame work instead of
> >>      implemeting it each bus layer independently.
> >> - The modification should be incremental.
> >>    Anthony would like to see that as an incremental addition to what we have
> >>    today (like introducing a propagating warm reset callback) and thinking
> >>    through what the actual behavior should and shouldn't be.
> >>
> >>
> >> If the direction is okay, The next step would be a patch(set) for qdev which
> >> would introduce qdev_cold_reset(), qdev_warm_reset(),
> >> DeviceInfo::cold_reset and DeviceInfo::warm_reset
> >> and would obsolete qdev_reset() and DeviceInfo::reset.
> >>
> >
> > What would be the difference between warm and cold reset?  Former called  
> > on any reset, while the latter called on power up only?
> 
> What I have in mind at the moment is,
> warm reset callback is called on warm reset, not called on power up.
> cold reset callback is called only on power up (and power cycle).
> 
Why stop there. Why not implement proper power planes support. Some
devices are not powered down on S3/S4 suspend for instance.

--
			Gleb.

[Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/30/2010 02:49 AM, Isaku Yamahata wrote:
> Distinguish warm reset from cold reset by introducing
> cold/warm reset helper function instead of single reset routines.
>
> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp>
> ---
>   hw/hw.h  |    7 +++++
>   sysemu.h |    4 +++
>   vl.c     |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
>   3 files changed, 85 insertions(+), 15 deletions(-)
>
> diff --git a/hw/hw.h b/hw/hw.h
> index 4405092..6fb844e 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -269,6 +269,13 @@ void register_device_unmigratable(DeviceState *dev, const char *idstr,
>
>   typedef void QEMUResetHandler(void *opaque);
>
> +
> +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque);
> +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque);
> +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque);
> +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque);
>    

I was thinking that we should stick entirely within the qdev abstraction.

The patchset I sent out introduced a cold reset as a qdev property on 
the devices.

For warm reset, if I understand correctly, we need two things.  We need 
to 1) control propagation order and we need to 2) differentiate 
per-device between cold reset and warm reset.

For (2), I don't know that we truly do need it.  For something like PCI 
AER, wouldn't we just move the AER initialization to the qdev init 
function and then never change the AER registers during reset?

IOW, the only way to do a cold reset would be to destroy and recreate 
the device.

For (1), I still don't fully understand what's needed here.  Do we just 
care about doing a certain transversal order (like depth-first) or do we 
actually care about withholding the reset signal for devices if as Avi 
said, we SCSI devices don't get reset.

Changing transversal order is trivial.  I think we should be very clear 
though if we're going to introduce bus-specific mechanisms to modify 
transversal though.

Regards,

Anthony Liguori

> +/* those two functions are obsoleted by cold/warm reset API. */
>   void qemu_register_reset(QEMUResetHandler *func, void *opaque);
>   void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
>
> diff --git a/sysemu.h b/sysemu.h
> index 7fc4e20..927892a 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -48,7 +48,11 @@ int64_t cpu_get_ticks(void);
>   void cpu_enable_ticks(void);
>   void cpu_disable_ticks(void);
>
> +/* transitional compat = qemu_system_warm_reset_request() */
>   void qemu_system_reset_request(void);
> +
> +void qemu_system_cold_reset_request(void);
> +void qemu_system_warm_reset_request(void);
>   void qemu_system_shutdown_request(void);
>   void qemu_system_powerdown_request(void);
>   extern qemu_irq qemu_system_powerdown;
> diff --git a/vl.c b/vl.c
> index a919a32..609d74c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1122,9 +1122,13 @@ typedef struct QEMUResetEntry {
>   } QEMUResetEntry;
>
>   QTAILQ_HEAD(reset_handlers, QEMUResetEntry);
> -static struct reset_handlers reset_handlers =
> -    QTAILQ_HEAD_INITIALIZER(reset_handlers);
> -static int reset_requested;
> +
> +static struct reset_handlers cold_reset_handlers =
> +    QTAILQ_HEAD_INITIALIZER(cold_reset_handlers);
> +static struct reset_handlers warm_reset_handlers =
> +    QTAILQ_HEAD_INITIALIZER(warm_reset_handlers);
> +static int cold_reset_requested;
> +static int warm_reset_requested;
>   static int shutdown_requested;
>   static int powerdown_requested;
>   int debug_requested;
> @@ -1142,9 +1146,14 @@ static int qemu_shutdown_requested(void)
>       return qemu_requested(&shutdown_requested);
>   }
>
> -static int qemu_reset_requested(void)
> +static int qemu_cold_reset_requested(void)
> +{
> +    return qemu_requested(&cold_reset_requested);
> +}
> +
> +static int qemu_warm_reset_requested(void)
>   {
> -    return qemu_requested(&reset_requested);
> +    return qemu_requested(&warm_reset_requested);
>   }
>
>   static int qemu_powerdown_requested(void)
> @@ -1196,20 +1205,51 @@ static void qemu_system_reset_handler(struct reset_handlers *handlers)
>       }
>   }
>
> +/* obsolated by qemu_register_cold/warm_reset() */
>   void qemu_register_reset(QEMUResetHandler *func, void *opaque)
>   {
> -    qemu_register_reset_handler(func, opaque,&reset_handlers);
> +    qemu_register_cold_reset(func, opaque);
> +    qemu_register_warm_reset(func, opaque);
>   }
>
> +/* obsolated by qemu_unregister_cold/warm_reset() */
>   void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
>   {
> -    qemu_unregister_reset_handler(func, opaque,&reset_handlers);
> +    qemu_unregister_cold_reset(func, opaque);
> +    qemu_unregister_warm_reset(func, opaque);
> +}
> +
> +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque)
> +{
> +    qemu_register_reset_handler(func, opaque,&cold_reset_handlers);
> +}
> +
> +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque)
> +{
> +    qemu_unregister_reset_handler(func, opaque,&cold_reset_handlers);
> +}
> +
> +static void qemu_system_cold_reset(void)
> +{
> +    qemu_system_reset_handler(&cold_reset_handlers);
> +    monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_COLD? */
> +    cpu_synchronize_all_post_reset();
> +}
> +
> +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque)
> +{
> +    qemu_register_reset_handler(func, opaque,&warm_reset_handlers);
> +}
> +
> +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque)
> +{
> +    qemu_unregister_reset_handler(func, opaque,&warm_reset_handlers);
>   }
>
> -static void qemu_system_reset(void)
> +static void qemu_system_warm_reset(void)
>   {
> -    qemu_system_reset_handler(&reset_handlers);
> -    monitor_protocol_event(QEVENT_RESET, NULL);
> +    qemu_system_reset_handler(&warm_reset_handlers);
> +    monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_WARM? */
>       cpu_synchronize_all_post_reset();
>   }
>
> @@ -1227,9 +1267,20 @@ static void qemu_system_request_reboot_check(int *requested)
>       qemu_system_request(requested);
>   }
>
> +void qemu_system_cold_reset_request(void)
> +{
> +    qemu_system_request_reboot_check(&cold_reset_requested);
> +}
> +
> +void qemu_system_warm_reset_request(void)
> +{
> +    qemu_system_request_reboot_check(&warm_reset_requested);
> +}
> +
> +/* trantitional compat */
>   void qemu_system_reset_request(void)
>   {
> -    qemu_system_request_reboot_check(&reset_requested);
> +    qemu_system_request_reboot_check(&warm_reset_requested);
>   }
>
>   void qemu_system_shutdown_request(void)
> @@ -1322,7 +1373,9 @@ static int vm_can_run(void)
>   {
>       if (powerdown_requested)
>           return 0;
> -    if (reset_requested)
> +    if (cold_reset_requested)
> +        return 0;
> +    if (warm_reset_requested)
>           return 0;
>       if (shutdown_requested)
>           return 0;
> @@ -1368,9 +1421,15 @@ static void main_loop(void)
>               } else
>                   break;
>           }
> -        if (qemu_reset_requested()) {
> +        if (qemu_warm_reset_requested()) {
> +            pause_all_vcpus();
> +            qemu_system_warm_reset();
> +            resume_all_vcpus();
> +        }
> +        if (qemu_cold_reset_requested()) {
> +            /* power cycle */
>               pause_all_vcpus();
> -            qemu_system_reset();
> +            qemu_system_cold_reset();
>               resume_all_vcpus();
>           }
>           if (qemu_powerdown_requested()) {
> @@ -2992,7 +3051,7 @@ int main(int argc, char **argv, char **envp)
>           exit(1);
>       }
>
> -    qemu_system_reset();
> +    qemu_system_cold_reset();
>       if (loadvm) {
>           if (load_vmstate(loadvm)<  0) {
>               autostart = 0;
>    

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/30/2010 03:50 AM, Paolo Bonzini wrote:
> On 08/30/2010 09:49 AM, Isaku Yamahata wrote:
>> +/* those two functions are obsoleted by cold/warm reset API. */
>> [qemu_register_reset/qemu_unregister_reset]
>
> Are they?

Yes, but introduce more reset functions isn't the right approach.

Reset should be a method of the device tree, not a stand alone function.

Regards,

Anthony Liguori

>
> They have a _lot_ of callers and most of the time you do not really 
> care about cold vs. warm reset.  So, I think either you define a new 
> API where you can request cold reset/warm reset/both, or 
> qemu_register_reset is here to stay forever.
>
> In general, I don't like the duplication you introduce between cold 
> reset, warm reset, shutdown, powerdown, etc.  Maybe you can introduce 
> a new "VMEvent" abstraction with functions like "request", "is 
> requested", "register handler"?
>
> It could also be interesting to convert everything to the Notifier 
> API, if someone wants to play with Coccinelle...
>
> Paolo
>

[Qemu-devel] Re: [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Glauber Costa]

On Mon, Aug 30, 2010 at 05:35:20PM +0900, Isaku Yamahata wrote:
> On Mon, Aug 30, 2010 at 10:59:19AM +0300, Avi Kivity wrote:
> >  On 08/30/2010 10:49 AM, Isaku Yamahata wrote:
> >> This patch set distinguish warm reset from cold reset by
> >> introducing warm reset callback handler.
> >> The first 4 patches are trivial clean up patches. The last patch of 5/5
> >> is RFC patch.
> >>
> >> The following thread arose cold reset vs warm reset issues.
> >> http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00186.html
> >> The summary is
> >> - warm reset is wanted in qemu
> >>    - Pressing the reset button is a warm reset on real machines
> >>    - Sparc64 CPU uses different reset vector for warm and cold reset,
> >>      so system_reset acts like a reset button
> >>    - Bus reset can be implemented utilizing qdev frame work instead of
> >>      implemeting it each bus layer independently.
> >> - The modification should be incremental.
> >>    Anthony would like to see that as an incremental addition to what we have
> >>    today (like introducing a propagating warm reset callback) and thinking
> >>    through what the actual behavior should and shouldn't be.
> >>
> >>
> >> If the direction is okay, The next step would be a patch(set) for qdev which
> >> would introduce qdev_cold_reset(), qdev_warm_reset(),
> >> DeviceInfo::cold_reset and DeviceInfo::warm_reset
> >> and would obsolete qdev_reset() and DeviceInfo::reset.
> >>
> >
> > What would be the difference between warm and cold reset?  Former called  
> > on any reset, while the latter called on power up only?
> 
> What I have in mind at the moment is,
> warm reset callback is called on warm reset, not called on power up.
> cold reset callback is called only on power up (and power cycle).
> 
> Hmm, should warm reset handler be called on power up?
> Each cold reset callbacks can call corresponding warm reset handler
> if necessary. So it would be redundant to call qdev_warm_reset() on power up.
> Or would it be more robust to call warm reset in addition to cold reset
> on power on?
I guess a good way to do that would be making "reset" just mean warm reset,
and keep a single list. Those would be called on every kind of reset.
Alternatively, cards that want to have a different power-on code could
then be added to an amendment list, interfaced by a cold-reset API.

Re: [Qemu-devel] Re: [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/30/2010 06:19 AM, Gleb Natapov wrote:
> Why stop there. Why not implement proper power planes support. Some
> devices are not powered down on S3/S4 suspend for instance.
>    

What feature would it give us?  The mapping of D-state to S-state is 
defined by the ACPI tables, no?  So we're in total control of whether we 
would ever do this and I can't see the advantage of doing it.

Regards,

Anthony Liguori

> --
> 			Gleb.
>
>    

Re: [Qemu-devel] Re: [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Gleb Natapov]

On Mon, Aug 30, 2010 at 08:05:07AM -0500, Anthony Liguori wrote:
> On 08/30/2010 06:19 AM, Gleb Natapov wrote:
> >Why stop there. Why not implement proper power planes support. Some
> >devices are not powered down on S3/S4 suspend for instance.
> 
> What feature would it give us?  The mapping of D-state to S-state is
> defined by the ACPI tables, no?  So we're in total control of
> whether we would ever do this and I can't see the advantage of doing
> it.
> 
ACPI only describes how actual HW behaves. I don't understand why do you
bring ACPI here. RTC can be setup to wake up guest. Currently we reset
RTC on S3.

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Blue Swirl]

On Mon, Aug 30, 2010 at 11:19 AM, Gleb Natapov <gleb@redhat.com> wrote:
> On Mon, Aug 30, 2010 at 05:35:20PM +0900, Isaku Yamahata wrote:
>> On Mon, Aug 30, 2010 at 10:59:19AM +0300, Avi Kivity wrote:
>> >  On 08/30/2010 10:49 AM, Isaku Yamahata wrote:
>> >> This patch set distinguish warm reset from cold reset by
>> >> introducing warm reset callback handler.
>> >> The first 4 patches are trivial clean up patches. The last patch of 5/5
>> >> is RFC patch.
>> >>
>> >> The following thread arose cold reset vs warm reset issues.
>> >> http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00186.html
>> >> The summary is
>> >> - warm reset is wanted in qemu
>> >>    - Pressing the reset button is a warm reset on real machines
>> >>    - Sparc64 CPU uses different reset vector for warm and cold reset,
>> >>      so system_reset acts like a reset button
>> >>    - Bus reset can be implemented utilizing qdev frame work instead of
>> >>      implemeting it each bus layer independently.
>> >> - The modification should be incremental.
>> >>    Anthony would like to see that as an incremental addition to what we have
>> >>    today (like introducing a propagating warm reset callback) and thinking
>> >>    through what the actual behavior should and shouldn't be.
>> >>
>> >>
>> >> If the direction is okay, The next step would be a patch(set) for qdev which
>> >> would introduce qdev_cold_reset(), qdev_warm_reset(),
>> >> DeviceInfo::cold_reset and DeviceInfo::warm_reset
>> >> and would obsolete qdev_reset() and DeviceInfo::reset.
>> >>
>> >
>> > What would be the difference between warm and cold reset?  Former called
>> > on any reset, while the latter called on power up only?
>>
>> What I have in mind at the moment is,
>> warm reset callback is called on warm reset, not called on power up.
>> cold reset callback is called only on power up (and power cycle).
>>
> Why stop there. Why not implement proper power planes support. Some
> devices are not powered down on S3/S4 suspend for instance.

Is warm reset similar to what happens to these when resuming from
suspend? Or is there some additional signal to tell the device? Is
there a spec for these things?

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Blue Swirl]

On Mon, Aug 30, 2010 at 1:03 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/30/2010 03:50 AM, Paolo Bonzini wrote:
>>
>> On 08/30/2010 09:49 AM, Isaku Yamahata wrote:
>>>
>>> +/* those two functions are obsoleted by cold/warm reset API. */
>>> [qemu_register_reset/qemu_unregister_reset]
>>
>> Are they?
>
> Yes, but introduce more reset functions isn't the right approach.
>
> Reset should be a method of the device tree, not a stand alone function.

In theory the reset tree may be very different from device tree. In
practice the reset tree is probably very flat (global reset signal, a
few bus reset signals) so device tree approach may get the same
results.

IIRC on some HW ages ago the CPU could initiate an external device
reset (without resetting the CPU) but in that case the board caused
also CPU to be reset so it was useless.

One way to model the disjoint reset trees would be to use an explicit
qemu_irq signal for reset. It's a bit complex to set up compared to
the almost flat tree we want though.

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/30/2010 02:16 PM, Blue Swirl wrote:
> On Mon, Aug 30, 2010 at 1:03 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>    
>> On 08/30/2010 03:50 AM, Paolo Bonzini wrote:
>>      
>>> On 08/30/2010 09:49 AM, Isaku Yamahata wrote:
>>>        
>>>> +/* those two functions are obsoleted by cold/warm reset API. */
>>>> [qemu_register_reset/qemu_unregister_reset]
>>>>          
>>> Are they?
>>>        
>> Yes, but introduce more reset functions isn't the right approach.
>>
>> Reset should be a method of the device tree, not a stand alone function.
>>      
> In theory the reset tree may be very different from device tree. In
> practice the reset tree is probably very flat (global reset signal, a
> few bus reset signals) so device tree approach may get the same
> results.
>    

Well the device tree doesn't really have to be a tree :-)

My thinking if we need to support custom reset propagation is that we 
have the current reset() handler return 0 to propagate to children, < 0 
on error, and > 0 to not propagate to direct children just as we do with 
the walkers.

In the case of > 0, the device can choose to propagate to any device 
that it knows about independent of the default walking order.  This 
makes the device tree a directed graph whereas the transversal path can 
be arbitrarily custom.

The only questions in my mind are, do we truly need this and do we need 
more than a single type of reset.  We could make this almost arbitrarily 
complicated if we wanted to but we should try to keep things simply 
unless there's a compelling reason not to.

Regards,

Anthon Liguori

> IIRC on some HW ages ago the CPU could initiate an external device
> reset (without resetting the CPU) but in that case the board caused
> also CPU to be reset so it was useless.
>
> One way to model the disjoint reset trees would be to use an explicit
> qemu_irq signal for reset. It's a bit complex to set up compared to
> the almost flat tree we want though.
>
>    

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Blue Swirl]

On Mon, Aug 30, 2010 at 7:25 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/30/2010 02:16 PM, Blue Swirl wrote:
>>
>> On Mon, Aug 30, 2010 at 1:03 PM, Anthony Liguori<anthony@codemonkey.ws>
>>  wrote:
>>
>>>
>>> On 08/30/2010 03:50 AM, Paolo Bonzini wrote:
>>>
>>>>
>>>> On 08/30/2010 09:49 AM, Isaku Yamahata wrote:
>>>>
>>>>>
>>>>> +/* those two functions are obsoleted by cold/warm reset API. */
>>>>> [qemu_register_reset/qemu_unregister_reset]
>>>>>
>>>>
>>>> Are they?
>>>>
>>>
>>> Yes, but introduce more reset functions isn't the right approach.
>>>
>>> Reset should be a method of the device tree, not a stand alone function.
>>>
>>
>> In theory the reset tree may be very different from device tree. In
>> practice the reset tree is probably very flat (global reset signal, a
>> few bus reset signals) so device tree approach may get the same
>> results.
>>
>
> Well the device tree doesn't really have to be a tree :-)

True, but is the non-tree still always useful for other things besides
reset? Again, in theory.

> My thinking if we need to support custom reset propagation is that we have
> the current reset() handler return 0 to propagate to children, < 0 on error,
> and > 0 to not propagate to direct children just as we do with the walkers.
>
> In the case of > 0, the device can choose to propagate to any device that it
> knows about independent of the default walking order.  This makes the device
> tree a directed graph whereas the transversal path can be arbitrarily
> custom.

I'd rather not have that much knowledge about the reset tree in the devices.

> The only questions in my mind are, do we truly need this and do we need more
> than a single type of reset.  We could make this almost arbitrarily
> complicated if we wanted to but we should try to keep things simply unless
> there's a compelling reason not to.

Fully agreed, I think current model works. But I'm not opposed to a
more generic approach, like VM events, combining also power control
with reset. Though events would not help with the disjoint tree
problem.

With qemu_irq approach, each event would be replaced by a signal type
with a few instances. The devices would be as simple as now, but
wiring in the board level would be bloated.

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/30/2010 02:36 PM, Blue Swirl wrote:
> On Mon, Aug 30, 2010 at 7:25 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>    
>> On 08/30/2010 02:16 PM, Blue Swirl wrote:
>>      
>>> On Mon, Aug 30, 2010 at 1:03 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>   wrote:
>>>
>>>        
>>>> On 08/30/2010 03:50 AM, Paolo Bonzini wrote:
>>>>
>>>>          
>>>>> On 08/30/2010 09:49 AM, Isaku Yamahata wrote:
>>>>>
>>>>>            
>>>>>> +/* those two functions are obsoleted by cold/warm reset API. */
>>>>>> [qemu_register_reset/qemu_unregister_reset]
>>>>>>
>>>>>>              
>>>>> Are they?
>>>>>
>>>>>            
>>>> Yes, but introduce more reset functions isn't the right approach.
>>>>
>>>> Reset should be a method of the device tree, not a stand alone function.
>>>>
>>>>          
>>> In theory the reset tree may be very different from device tree. In
>>> practice the reset tree is probably very flat (global reset signal, a
>>> few bus reset signals) so device tree approach may get the same
>>> results.
>>>
>>>        
>> Well the device tree doesn't really have to be a tree :-)
>>      
> True, but is the non-tree still always useful for other things besides
> reset? Again, in theory.
>    

I think non-tree transversal are not the norm.  But I guess my point is, 
being a tree verses a directed graph is really just a state of mind :-)

If we use a visitor pattern and then allow a visitor to redirect the 
recursion, then assuming the nodes have links to other nodes that don't 
fall in the normal tree hierarchy, it should just work.

OTOH, if we really designed it as a graph we would need to keep a 
visited history which generally is a pain in the butt.  I don't want to 
do that but I think what we have now is good enough.

>> My thinking if we need to support custom reset propagation is that we have
>> the current reset() handler return 0 to propagate to children,<  0 on error,
>> and>  0 to not propagate to direct children just as we do with the walkers.
>>
>> In the case of>  0, the device can choose to propagate to any device that it
>> knows about independent of the default walking order.  This makes the device
>> tree a directed graph whereas the transversal path can be arbitrarily
>> custom.
>>      
> I'd rather not have that much knowledge about the reset tree in the devices.
>    

Aren't the devices the precise place where that knowledge should 
reside?  Is the reset tree really every implemented entirely based on 
complex wiring that exists outside of any type of bus topology?

>> The only questions in my mind are, do we truly need this and do we need more
>> than a single type of reset.  We could make this almost arbitrarily
>> complicated if we wanted to but we should try to keep things simply unless
>> there's a compelling reason not to.
>>      
> Fully agreed, I think current model works. But I'm not opposed to a
> more generic approach, like VM events, combining also power control
> with reset. Though events would not help with the disjoint tree
> problem.
>
> With qemu_irq approach, each event would be replaced by a signal type
> with a few instances. The devices would be as simple as now, but
> wiring in the board level would be bloated.
>    

A signal/slot infrastructure would be a better way to express this sort 
of thing because at the wire level, you'll quickly encounter line 
repeaters, muxers, and demuxers.

But I think we're far ahead of ourselves.  I think we'll solve the 
current problem merely by taking the approach in my previous patch and 
simply making something like PCI AER initialization part of the init() 
routine and not part of the reset() routine.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Isaku Yamahata]

On Mon, Aug 30, 2010 at 07:59:22AM -0500, Anthony Liguori wrote:
> On 08/30/2010 02:49 AM, Isaku Yamahata wrote:
>> Distinguish warm reset from cold reset by introducing
>> cold/warm reset helper function instead of single reset routines.
>>
>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp>
>> ---
>>   hw/hw.h  |    7 +++++
>>   sysemu.h |    4 +++
>>   vl.c     |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
>>   3 files changed, 85 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/hw.h b/hw/hw.h
>> index 4405092..6fb844e 100644
>> --- a/hw/hw.h
>> +++ b/hw/hw.h
>> @@ -269,6 +269,13 @@ void register_device_unmigratable(DeviceState *dev, const char *idstr,
>>
>>   typedef void QEMUResetHandler(void *opaque);
>>
>> +
>> +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque);
>> +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque);
>> +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque);
>> +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque);
>>    
>
> I was thinking that we should stick entirely within the qdev abstraction.
>
> The patchset I sent out introduced a cold reset as a qdev property on  
> the devices.
>
> For warm reset, if I understand correctly, we need two things.  We need  
> to 1) control propagation order and we need to 2) differentiate  
> per-device between cold reset and warm reset.
>
> For (2), I don't know that we truly do need it.  For something like PCI  
> AER, wouldn't we just move the AER initialization to the qdev init  
> function and then never change the AER registers during reset?
>
> IOW, the only way to do a cold reset would be to destroy and recreate  
> the device.

I'm lost here. Then, what should qdev_reset() do?
So far you have strongly claimed that qdev_reset() should be cold reset.
(the current implementation is imperfect though)
So I had to create the patch that distinguishes warm reset from cold reset.
But here you suggest that qdev_reset() be warm reset and only way to
cold-reset be destroy/re-create.

Can you elaborate on what qdev_reset() API should do?
If qdev_reset() is cold reset, something like qdev_warm_reset()
is necessary.
If qdev_reset() is warm reset and the only way to cold-reset is
destroy/re-create, I'm fine with it.

> For (1), I still don't fully understand what's needed here.  Do we just  
> care about doing a certain transversal order (like depth-first) or do we  
> actually care about withholding the reset signal for devices if as Avi  
> said, we SCSI devices don't get reset.
>
> Changing transversal order is trivial.  I think we should be very clear  
> though if we're going to introduce bus-specific mechanisms to modify  
> transversal though.
>
> Regards,
>
> Anthony Liguori
>
>> +/* those two functions are obsoleted by cold/warm reset API. */
>>   void qemu_register_reset(QEMUResetHandler *func, void *opaque);
>>   void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
>>
>> diff --git a/sysemu.h b/sysemu.h
>> index 7fc4e20..927892a 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -48,7 +48,11 @@ int64_t cpu_get_ticks(void);
>>   void cpu_enable_ticks(void);
>>   void cpu_disable_ticks(void);
>>
>> +/* transitional compat = qemu_system_warm_reset_request() */
>>   void qemu_system_reset_request(void);
>> +
>> +void qemu_system_cold_reset_request(void);
>> +void qemu_system_warm_reset_request(void);
>>   void qemu_system_shutdown_request(void);
>>   void qemu_system_powerdown_request(void);
>>   extern qemu_irq qemu_system_powerdown;
>> diff --git a/vl.c b/vl.c
>> index a919a32..609d74c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1122,9 +1122,13 @@ typedef struct QEMUResetEntry {
>>   } QEMUResetEntry;
>>
>>   QTAILQ_HEAD(reset_handlers, QEMUResetEntry);
>> -static struct reset_handlers reset_handlers =
>> -    QTAILQ_HEAD_INITIALIZER(reset_handlers);
>> -static int reset_requested;
>> +
>> +static struct reset_handlers cold_reset_handlers =
>> +    QTAILQ_HEAD_INITIALIZER(cold_reset_handlers);
>> +static struct reset_handlers warm_reset_handlers =
>> +    QTAILQ_HEAD_INITIALIZER(warm_reset_handlers);
>> +static int cold_reset_requested;
>> +static int warm_reset_requested;
>>   static int shutdown_requested;
>>   static int powerdown_requested;
>>   int debug_requested;
>> @@ -1142,9 +1146,14 @@ static int qemu_shutdown_requested(void)
>>       return qemu_requested(&shutdown_requested);
>>   }
>>
>> -static int qemu_reset_requested(void)
>> +static int qemu_cold_reset_requested(void)
>> +{
>> +    return qemu_requested(&cold_reset_requested);
>> +}
>> +
>> +static int qemu_warm_reset_requested(void)
>>   {
>> -    return qemu_requested(&reset_requested);
>> +    return qemu_requested(&warm_reset_requested);
>>   }
>>
>>   static int qemu_powerdown_requested(void)
>> @@ -1196,20 +1205,51 @@ static void qemu_system_reset_handler(struct reset_handlers *handlers)
>>       }
>>   }
>>
>> +/* obsolated by qemu_register_cold/warm_reset() */
>>   void qemu_register_reset(QEMUResetHandler *func, void *opaque)
>>   {
>> -    qemu_register_reset_handler(func, opaque,&reset_handlers);
>> +    qemu_register_cold_reset(func, opaque);
>> +    qemu_register_warm_reset(func, opaque);
>>   }
>>
>> +/* obsolated by qemu_unregister_cold/warm_reset() */
>>   void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
>>   {
>> -    qemu_unregister_reset_handler(func, opaque,&reset_handlers);
>> +    qemu_unregister_cold_reset(func, opaque);
>> +    qemu_unregister_warm_reset(func, opaque);
>> +}
>> +
>> +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque)
>> +{
>> +    qemu_register_reset_handler(func, opaque,&cold_reset_handlers);
>> +}
>> +
>> +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque)
>> +{
>> +    qemu_unregister_reset_handler(func, opaque,&cold_reset_handlers);
>> +}
>> +
>> +static void qemu_system_cold_reset(void)
>> +{
>> +    qemu_system_reset_handler(&cold_reset_handlers);
>> +    monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_COLD? */
>> +    cpu_synchronize_all_post_reset();
>> +}
>> +
>> +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque)
>> +{
>> +    qemu_register_reset_handler(func, opaque,&warm_reset_handlers);
>> +}
>> +
>> +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque)
>> +{
>> +    qemu_unregister_reset_handler(func, opaque,&warm_reset_handlers);
>>   }
>>
>> -static void qemu_system_reset(void)
>> +static void qemu_system_warm_reset(void)
>>   {
>> -    qemu_system_reset_handler(&reset_handlers);
>> -    monitor_protocol_event(QEVENT_RESET, NULL);
>> +    qemu_system_reset_handler(&warm_reset_handlers);
>> +    monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_WARM? */
>>       cpu_synchronize_all_post_reset();
>>   }
>>
>> @@ -1227,9 +1267,20 @@ static void qemu_system_request_reboot_check(int *requested)
>>       qemu_system_request(requested);
>>   }
>>
>> +void qemu_system_cold_reset_request(void)
>> +{
>> +    qemu_system_request_reboot_check(&cold_reset_requested);
>> +}
>> +
>> +void qemu_system_warm_reset_request(void)
>> +{
>> +    qemu_system_request_reboot_check(&warm_reset_requested);
>> +}
>> +
>> +/* trantitional compat */
>>   void qemu_system_reset_request(void)
>>   {
>> -    qemu_system_request_reboot_check(&reset_requested);
>> +    qemu_system_request_reboot_check(&warm_reset_requested);
>>   }
>>
>>   void qemu_system_shutdown_request(void)
>> @@ -1322,7 +1373,9 @@ static int vm_can_run(void)
>>   {
>>       if (powerdown_requested)
>>           return 0;
>> -    if (reset_requested)
>> +    if (cold_reset_requested)
>> +        return 0;
>> +    if (warm_reset_requested)
>>           return 0;
>>       if (shutdown_requested)
>>           return 0;
>> @@ -1368,9 +1421,15 @@ static void main_loop(void)
>>               } else
>>                   break;
>>           }
>> -        if (qemu_reset_requested()) {
>> +        if (qemu_warm_reset_requested()) {
>> +            pause_all_vcpus();
>> +            qemu_system_warm_reset();
>> +            resume_all_vcpus();
>> +        }
>> +        if (qemu_cold_reset_requested()) {
>> +            /* power cycle */
>>               pause_all_vcpus();
>> -            qemu_system_reset();
>> +            qemu_system_cold_reset();
>>               resume_all_vcpus();
>>           }
>>           if (qemu_powerdown_requested()) {
>> @@ -2992,7 +3051,7 @@ int main(int argc, char **argv, char **envp)
>>           exit(1);
>>       }
>>
>> -    qemu_system_reset();
>> +    qemu_system_cold_reset();
>>       if (loadvm) {
>>           if (load_vmstate(loadvm)<  0) {
>>               autostart = 0;
>>    
>
>

-- 
yamahata

Re: [Qemu-devel] Re: [PATCH 0/5] RFC: distinguish warm reset from cold reset.

[Gleb Natapov]

On Mon, Aug 30, 2010 at 07:07:11PM +0000, Blue Swirl wrote:
> On Mon, Aug 30, 2010 at 11:19 AM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Mon, Aug 30, 2010 at 05:35:20PM +0900, Isaku Yamahata wrote:
> >> On Mon, Aug 30, 2010 at 10:59:19AM +0300, Avi Kivity wrote:
> >> >  On 08/30/2010 10:49 AM, Isaku Yamahata wrote:
> >> >> This patch set distinguish warm reset from cold reset by
> >> >> introducing warm reset callback handler.
> >> >> The first 4 patches are trivial clean up patches. The last patch of 5/5
> >> >> is RFC patch.
> >> >>
> >> >> The following thread arose cold reset vs warm reset issues.
> >> >> http://lists.nongnu.org/archive/html/qemu-devel/2010-08/msg00186.html
> >> >> The summary is
> >> >> - warm reset is wanted in qemu
> >> >>    - Pressing the reset button is a warm reset on real machines
> >> >>    - Sparc64 CPU uses different reset vector for warm and cold reset,
> >> >>      so system_reset acts like a reset button
> >> >>    - Bus reset can be implemented utilizing qdev frame work instead of
> >> >>      implemeting it each bus layer independently.
> >> >> - The modification should be incremental.
> >> >>    Anthony would like to see that as an incremental addition to what we have
> >> >>    today (like introducing a propagating warm reset callback) and thinking
> >> >>    through what the actual behavior should and shouldn't be.
> >> >>
> >> >>
> >> >> If the direction is okay, The next step would be a patch(set) for qdev which
> >> >> would introduce qdev_cold_reset(), qdev_warm_reset(),
> >> >> DeviceInfo::cold_reset and DeviceInfo::warm_reset
> >> >> and would obsolete qdev_reset() and DeviceInfo::reset.
> >> >>
> >> >
> >> > What would be the difference between warm and cold reset?  Former called
> >> > on any reset, while the latter called on power up only?
> >>
> >> What I have in mind at the moment is,
> >> warm reset callback is called on warm reset, not called on power up.
> >> cold reset callback is called only on power up (and power cycle).
> >>
> > Why stop there. Why not implement proper power planes support. Some
> > devices are not powered down on S3/S4 suspend for instance.
> 
> Is warm reset similar to what happens to these when resuming from
> suspend?
I would say cold reset is more similar to what happens during resume
from suspend for most devices since during suspend they do not have
power at all.

>           Or is there some additional signal to tell the device? Is
> there a spec for these things?
Things are different from chipset to chipset. In piix4 spec there is
chapter called Power Planes which describe power planes available and
what is powered by them.

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/30/2010 09:58 PM, Isaku Yamahata wrote:
>> I was thinking that we should stick entirely within the qdev abstraction.
>>
>> The patchset I sent out introduced a cold reset as a qdev property on
>> the devices.
>>
>> For warm reset, if I understand correctly, we need two things.  We need
>> to 1) control propagation order and we need to 2) differentiate
>> per-device between cold reset and warm reset.
>>
>> For (2), I don't know that we truly do need it.  For something like PCI
>> AER, wouldn't we just move the AER initialization to the qdev init
>> function and then never change the AER registers during reset?
>>
>> IOW, the only way to do a cold reset would be to destroy and recreate
>> the device.
>>      
> I'm lost here. Then, what should qdev_reset() do?
>    

I don't know, that's what I'm trying to understand.

As of this moment, you've convinced me that it should be a warm reset.  
However, I'm not yet convinced that we need to allow buses to change the 
propagation path of the warm reset.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Gleb Natapov]

On Tue, Aug 31, 2010 at 08:08:17AM -0500, Anthony Liguori wrote:
> On 08/30/2010 09:58 PM, Isaku Yamahata wrote:
> >>I was thinking that we should stick entirely within the qdev abstraction.
> >>
> >>The patchset I sent out introduced a cold reset as a qdev property on
> >>the devices.
> >>
> >>For warm reset, if I understand correctly, we need two things.  We need
> >>to 1) control propagation order and we need to 2) differentiate
> >>per-device between cold reset and warm reset.
> >>
> >>For (2), I don't know that we truly do need it.  For something like PCI
> >>AER, wouldn't we just move the AER initialization to the qdev init
> >>function and then never change the AER registers during reset?
> >>
> >>IOW, the only way to do a cold reset would be to destroy and recreate
> >>the device.
> >I'm lost here. Then, what should qdev_reset() do?
> 
> I don't know, that's what I'm trying to understand.
> 
> As of this moment, you've convinced me that it should be a warm
> reset.  However, I'm not yet convinced that we need to allow buses
> to change the propagation path of the warm reset.
> 
System_reset should do cold reset like it does now.
 
--
			Gleb.

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/31/2010 08:14 AM, Gleb Natapov wrote:
> System_reset should do cold reset like it does now.

Why?

Regards,

Anthony Liguori

>
> --
> 			Gleb.
>    

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Gleb Natapov]

On Tue, Aug 31, 2010 at 08:20:51AM -0500, Anthony Liguori wrote:
> On 08/31/2010 08:14 AM, Gleb Natapov wrote:
> >System_reset should do cold reset like it does now.
> 
> Why?
> 
Because I should not be forced to restart qemu to bring devices to
initial state.

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/31/2010 08:21 AM, Gleb Natapov wrote:
> On Tue, Aug 31, 2010 at 08:20:51AM -0500, Anthony Liguori wrote:
>    
>> On 08/31/2010 08:14 AM, Gleb Natapov wrote:
>>      
>>> System_reset should do cold reset like it does now.
>>>        
>> Why?
>>
>>      
> Because I should not be forced to restart qemu to bring devices to
> initial state.
>    

IOW, you use system_reset for debugging purposes to reset the device model.

Point taken but functionally speaking, system_reset should map to a 
RESET signal and from what I can tell in this thread, that's a warm reset.

Regards,

Anthony Liguori

> --
> 			Gleb.
>    

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Avi Kivity]

  On 08/31/2010 04:26 PM, Anthony Liguori wrote:
> On 08/31/2010 08:21 AM, Gleb Natapov wrote:
>> On Tue, Aug 31, 2010 at 08:20:51AM -0500, Anthony Liguori wrote:
>>> On 08/31/2010 08:14 AM, Gleb Natapov wrote:
>>>> System_reset should do cold reset like it does now.
>>> Why?
>>>
>> Because I should not be forced to restart qemu to bring devices to
>> initial state.
>
> IOW, you use system_reset for debugging purposes to reset the device 
> model.
>
> Point taken but functionally speaking, system_reset should map to a 
> RESET signal and from what I can tell in this thread, that's a warm 
> reset.
>

Note, for most devices there's no difference.  x86 has INIT and RESET, 
with the keyboard controller RESET signal sometimes wired to INIT, and 
RAM doesn't have RESET.  Otherwise most devices don't see a difference.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/31/2010 08:29 AM, Avi Kivity wrote:
> Note, for most devices there's no difference.  x86 has INIT and RESET, 
> with the keyboard controller RESET signal sometimes wired to INIT, and 
> RAM doesn't have RESET.  Otherwise most devices don't see a difference.

Yes, that's why I'm wondering if we can just get away with using a 
simple reset() callback and for the handful of devices that don't do a 
full reset, they can just move the state unaffected by warm reset to 
->init().

For cold reset, I'd rather approach it as a device destroy + create.  
This means that given a DeviceState, we need to collect enough 
information to recreate the device.  I'm not 100% sure we have that 
today but if we solve that problem, it means we can migrate the device 
tree during migration which is a feature I'd really like to see.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Gleb Natapov]

On Tue, Aug 31, 2010 at 04:29:43PM +0300, Avi Kivity wrote:
>  On 08/31/2010 04:26 PM, Anthony Liguori wrote:
> >On 08/31/2010 08:21 AM, Gleb Natapov wrote:
> >>On Tue, Aug 31, 2010 at 08:20:51AM -0500, Anthony Liguori wrote:
> >>>On 08/31/2010 08:14 AM, Gleb Natapov wrote:
> >>>>System_reset should do cold reset like it does now.
> >>>Why?
> >>>
> >>Because I should not be forced to restart qemu to bring devices to
> >>initial state.
> >
> >IOW, you use system_reset for debugging purposes to reset the
> >device model.
> >
> >Point taken but functionally speaking, system_reset should map to
> >a RESET signal and from what I can tell in this thread, that's a
> >warm reset.
> >
> 
> Note, for most devices there's no difference.  x86 has INIT and
> RESET, with the keyboard controller RESET signal sometimes wired to
> INIT, and RAM doesn't have RESET.  Otherwise most devices don't see
> a difference.
> 
Actually soft reset is defined as INIT in PIIX4 spec:

Bits 1 and 2 in this register are used by PIIX4 to generate a hard reset
or a soft reset. During a hard reset, PIIX4 asserts CPURST, PCIRST#, and
RSTDRV, as well as reset its core and suspend well logic. During
a soft reset, PIIX4 asserts INIT.

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Avi Kivity]

  On 08/31/2010 04:34 PM, Anthony Liguori wrote:
> On 08/31/2010 08:29 AM, Avi Kivity wrote:
>> Note, for most devices there's no difference.  x86 has INIT and 
>> RESET, with the keyboard controller RESET signal sometimes wired to 
>> INIT, and RAM doesn't have RESET.  Otherwise most devices don't see a 
>> difference.
>
> Yes, that's why I'm wondering if we can just get away with using a 
> simple reset() callback and for the handful of devices that don't do a 
> full reset, they can just move the state unaffected by warm reset to 
> ->init().
>

This seems reasonable.

> For cold reset, I'd rather approach it as a device destroy + create.  
> This means that given a DeviceState, we need to collect enough 
> information to recreate the device.  I'm not 100% sure we have that 
> today but if we solve that problem, it means we can migrate the device 
> tree during migration which is a feature I'd really like to see.

Why do we need a cold reset at all?  it doesn't map to anything.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/31/2010 08:46 AM, Avi Kivity wrote:
>  On 08/31/2010 04:34 PM, Anthony Liguori wrote:
>> On 08/31/2010 08:29 AM, Avi Kivity wrote:
>>> Note, for most devices there's no difference.  x86 has INIT and 
>>> RESET, with the keyboard controller RESET signal sometimes wired to 
>>> INIT, and RAM doesn't have RESET.  Otherwise most devices don't see 
>>> a difference.
>>
>> Yes, that's why I'm wondering if we can just get away with using a 
>> simple reset() callback and for the handful of devices that don't do 
>> a full reset, they can just move the state unaffected by warm reset 
>> to ->init().
>>
>
> This seems reasonable.

But I'm still not sure whether the reset signal can be deliver based on 
a pre-order transversal or whether a custom transversal was required 
that each bus participates in.

>> For cold reset, I'd rather approach it as a device destroy + create.  
>> This means that given a DeviceState, we need to collect enough 
>> information to recreate the device.  I'm not 100% sure we have that 
>> today but if we solve that problem, it means we can migrate the 
>> device tree during migration which is a feature I'd really like to see.
>
> Why do we need a cold reset at all?  it doesn't map to anything.

That's why I'm suggesting a second-class approach to implementing it if 
someone really wants it.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Gleb Natapov]

On Tue, Aug 31, 2010 at 08:58:06AM -0500, Anthony Liguori wrote:
> On 08/31/2010 08:46 AM, Avi Kivity wrote:
> > On 08/31/2010 04:34 PM, Anthony Liguori wrote:
> >>On 08/31/2010 08:29 AM, Avi Kivity wrote:
> >>>Note, for most devices there's no difference.  x86 has INIT
> >>>and RESET, with the keyboard controller RESET signal sometimes
> >>>wired to INIT, and RAM doesn't have RESET.  Otherwise most
> >>>devices don't see a difference.
> >>
> >>Yes, that's why I'm wondering if we can just get away with using
> >>a simple reset() callback and for the handful of devices that
> >>don't do a full reset, they can just move the state unaffected
> >>by warm reset to ->init().
> >>
> >
> >This seems reasonable.
> 
> But I'm still not sure whether the reset signal can be deliver based
> on a pre-order transversal or whether a custom transversal was
> required that each bus participates in.
> 
The thing is in qemu reset of one device can affect state of other
device. Think about device that updates its interrupt line during reset
and this affects pic/ioapic/apic. Real HW does not have a problem that
we have here.

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Avi Kivity]

  On 08/31/2010 04:58 PM, Anthony Liguori wrote:
> On 08/31/2010 08:46 AM, Avi Kivity wrote:
>>  On 08/31/2010 04:34 PM, Anthony Liguori wrote:
>>> On 08/31/2010 08:29 AM, Avi Kivity wrote:
>>>> Note, for most devices there's no difference.  x86 has INIT and 
>>>> RESET, with the keyboard controller RESET signal sometimes wired to 
>>>> INIT, and RAM doesn't have RESET.  Otherwise most devices don't see 
>>>> a difference.
>>>
>>> Yes, that's why I'm wondering if we can just get away with using a 
>>> simple reset() callback and for the handful of devices that don't do 
>>> a full reset, they can just move the state unaffected by warm reset 
>>> to ->init().
>>>
>>
>> This seems reasonable.
>
> But I'm still not sure whether the reset signal can be deliver based 
> on a pre-order transversal or whether a custom transversal was 
> required that each bus participates in.

There's no such thing as a global reset.  There's a trace on the board 
that's called RESET that's connected to many chip's RESET input (but 
perhaps not all).  A PCI bus controller will likely propagate its RESET 
input into the PCI reset signal, however it's called.  So individual 
RESET traces are connected by chips, which have different conditions for 
asserting them.

So yes, we need a custom traversal.  Some bus controllers will block off 
RESET completely, some will pass it through, some can reset the devices 
on their bus independently of the controller's RESET input, if it has one.

(but I think we're drifting off into pointlessness here)

>
>>> For cold reset, I'd rather approach it as a device destroy + 
>>> create.  This means that given a DeviceState, we need to collect 
>>> enough information to recreate the device.  I'm not 100% sure we 
>>> have that today but if we solve that problem, it means we can 
>>> migrate the device tree during migration which is a feature I'd 
>>> really like to see.
>>
>> Why do we need a cold reset at all?  it doesn't map to anything.
>
> That's why I'm suggesting a second-class approach to implementing it 
> if someone really wants it.

Sure.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Anthony Liguori]

On 08/31/2010 09:03 AM, Avi Kivity wrote:
> There's no such thing as a global reset.  There's a trace on the board 
> that's called RESET that's connected to many chip's RESET input (but 
> perhaps not all).  A PCI bus controller will likely propagate its 
> RESET input into the PCI reset signal, however it's called.  So 
> individual RESET traces are connected by chips, which have different 
> conditions for asserting them.
>
> So yes, we need a custom traversal.  Some bus controllers will block 
> off RESET completely, some will pass it through, some can reset the 
> devices on their bus independently of the controller's RESET input, if 
> it has one.
>
> (but I think we're drifting off into pointlessness here)

That's my source of confusion.  I know hardware does a custom 
transversal but from a functional perspective, do we care?

So far, no one has answered this with, "yes, because device XX 
propagates in a specific ordering of child a, b, c, d..." which I take 
to mean that the answer is no.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Avi Kivity]

  On 08/31/2010 06:00 PM, Anthony Liguori wrote:
> On 08/31/2010 09:03 AM, Avi Kivity wrote:
>> There's no such thing as a global reset.  There's a trace on the 
>> board that's called RESET that's connected to many chip's RESET input 
>> (but perhaps not all).  A PCI bus controller will likely propagate 
>> its RESET input into the PCI reset signal, however it's called.  So 
>> individual RESET traces are connected by chips, which have different 
>> conditions for asserting them.
>>
>> So yes, we need a custom traversal.  Some bus controllers will block 
>> off RESET completely, some will pass it through, some can reset the 
>> devices on their bus independently of the controller's RESET input, 
>> if it has one.
>>
>> (but I think we're drifting off into pointlessness here)
>
> That's my source of confusion.  I know hardware does a custom 
> transversal but from a functional perspective, do we care?
>
> So far, no one has answered this with, "yes, because device XX 
> propagates in a specific ordering of child a, b, c, d..." which I take 
> to mean that the answer is no.
>

I don't think there's an issue with the order of propagation, just 
whether propagation takes place or not.

I'd make all bus models explicitly propagate reset if they want to.