[Qemu-devel] [PATCH] Reset system before loadvm

[Qemu-devel] [PATCH] Reset system before loadvm

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

In case we load the vmstate during incoming migration, we start from a
clean, default machine state as we went through system reset before. But
if we load from a snapshot, the machine can be in any state. That can
cause troubles if loading an older image which does not carry all state
information the executing QEMU requires. Almost no device takes care of
this scenario.

However, fixing this is trivial. We just need to issue a system reset
during loadvm as well.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 savevm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/savevm.c b/savevm.c
index 98b2422..5db01aa 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2074,6 +2074,7 @@ int load_vmstate(const char *name)
         return -EINVAL;
     }
 
+    qemu_system_reset();
     ret = qemu_loadvm_state(f);
 
     qemu_fclose(f);
-- 
1.7.1

Re: [Qemu-devel] [PATCH] Reset system before loadvm

[Avi Kivity]

On 06/11/2011 12:05 PM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> In case we load the vmstate during incoming migration, we start from a
> clean, default machine state as we went through system reset before. But
> if we load from a snapshot, the machine can be in any state. That can
> cause troubles if loading an older image which does not carry all state
> information the executing QEMU requires. Almost no device takes care of
> this scenario.
>
> However, fixing this is trivial. We just need to issue a system reset
> during loadvm as well.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>   savevm.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 98b2422..5db01aa 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2074,6 +2074,7 @@ int load_vmstate(const char *name)
>           return -EINVAL;
>       }
>
> +    qemu_system_reset();
>       ret = qemu_loadvm_state(f);
>
>       qemu_fclose(f);

Should we suppress the reset event sent out on the monitor?  After all, 
it's the result of an internal implementation choice, not something the 
user or the guest did.


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

Re: [Qemu-devel] [PATCH] Reset system before loadvm

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]

On 2011-06-12 19:13, Avi Kivity wrote:
> On 06/11/2011 12:05 PM, Jan Kiszka wrote:
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> In case we load the vmstate during incoming migration, we start from a
>> clean, default machine state as we went through system reset before. But
>> if we load from a snapshot, the machine can be in any state. That can
>> cause troubles if loading an older image which does not carry all state
>> information the executing QEMU requires. Almost no device takes care of
>> this scenario.
>>
>> However, fixing this is trivial. We just need to issue a system reset
>> during loadvm as well.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>   savevm.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 98b2422..5db01aa 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2074,6 +2074,7 @@ int load_vmstate(const char *name)
>>           return -EINVAL;
>>       }
>>
>> +    qemu_system_reset();
>>       ret = qemu_loadvm_state(f);
>>
>>       qemu_fclose(f);
> 
> Should we suppress the reset event sent out on the monitor?  After all,
> it's the result of an internal implementation choice, not something the
> user or the guest did.

We already issue this pattern during -loadvm or -incoming - or is the
monitor not yet connected at this point?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH] Reset system before loadvm

[Avi Kivity]

On 06/14/2011 09:19 AM, Jan Kiszka wrote:
> On 2011-06-12 19:13, Avi Kivity wrote:
> >  On 06/11/2011 12:05 PM, Jan Kiszka wrote:
> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
> >>
> >>  In case we load the vmstate during incoming migration, we start from a
> >>  clean, default machine state as we went through system reset before. But
> >>  if we load from a snapshot, the machine can be in any state. That can
> >>  cause troubles if loading an older image which does not carry all state
> >>  information the executing QEMU requires. Almost no device takes care of
> >>  this scenario.
> >>
> >>  However, fixing this is trivial. We just need to issue a system reset
> >>  during loadvm as well.
>

> >>  +    qemu_system_reset();
> >>        ret = qemu_loadvm_state(f);
> >>
> >>        qemu_fclose(f);
> >
> >  Should we suppress the reset event sent out on the monitor?  After all,
> >  it's the result of an internal implementation choice, not something the
> >  user or the guest did.
>
> We already issue this pattern during -loadvm or -incoming - or is the
> monitor not yet connected at this point?

I believe it is not.  But regardless, we shouldn't add more incorrect 
behaviour.

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

Re: [Qemu-devel] [PATCH] Reset system before loadvm

[Jan Kiszka]

On 2011-06-14 12:50, Avi Kivity wrote:
> On 06/14/2011 09:19 AM, Jan Kiszka wrote:
>> On 2011-06-12 19:13, Avi Kivity wrote:
>> >  On 06/11/2011 12:05 PM, Jan Kiszka wrote:
>> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
>> >>
>> >>  In case we load the vmstate during incoming migration, we start
>> from a
>> >>  clean, default machine state as we went through system reset
>> before. But
>> >>  if we load from a snapshot, the machine can be in any state. That can
>> >>  cause troubles if loading an older image which does not carry all
>> state
>> >>  information the executing QEMU requires. Almost no device takes
>> care of
>> >>  this scenario.
>> >>
>> >>  However, fixing this is trivial. We just need to issue a system reset
>> >>  during loadvm as well.
>>
> 
>> >>  +    qemu_system_reset();
>> >>        ret = qemu_loadvm_state(f);
>> >>
>> >>        qemu_fclose(f);
>> >
>> >  Should we suppress the reset event sent out on the monitor?  After
>> all,
>> >  it's the result of an internal implementation choice, not something
>> the
>> >  user or the guest did.
>>
>> We already issue this pattern during -loadvm or -incoming - or is the
>> monitor not yet connected at this point?
> 
> I believe it is not.  But regardless, we shouldn't add more incorrect
> behaviour.

It depends on how the reset event is defined in QMP. As I see it, there
is nothing stated about reset reasons or sources. So emitting
information about the actually happening reset can't be incorrect. Just
like emitting the information about the VM stop/start around loadvm.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] Reset system before loadvm

[Avi Kivity]

On 06/14/2011 01:56 PM, Jan Kiszka wrote:
> >
> >  I believe it is not.  But regardless, we shouldn't add more incorrect
> >  behaviour.
>
> It depends on how the reset event is defined in QMP. As I see it, there
> is nothing stated about reset reasons or sources. So emitting
> information about the actually happening reset can't be incorrect. Just
> like emitting the information about the VM stop/start around loadvm.

I don't think so.  Theoretically we could stop the vm, save a bit of 
state, reset it, and load the state back.  Did a reset occur?  Not from 
the user's point of view.

If a reset event is interesting (personally I don't think it is, so 
much, perhaps just for logging purposes), we should restrict it to user 
visible events (so it means either the user pressed the reset button or 
the guest reset itself).

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

Re: [Qemu-devel] [PATCH] Reset system before loadvm

[Luiz Capitulino]

On Tue, 14 Jun 2011 14:14:58 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 06/14/2011 01:56 PM, Jan Kiszka wrote:
> > >
> > >  I believe it is not.  But regardless, we shouldn't add more incorrect
> > >  behaviour.
> >
> > It depends on how the reset event is defined in QMP. As I see it, there
> > is nothing stated about reset reasons or sources. So emitting
> > information about the actually happening reset can't be incorrect. Just
> > like emitting the information about the VM stop/start around loadvm.
> 
> I don't think so.  Theoretically we could stop the vm, save a bit of 
> state, reset it, and load the state back.  Did a reset occur?  Not from 
> the user's point of view.
> 
> If a reset event is interesting (personally I don't think it is, so 
> much, perhaps just for logging purposes), we should restrict it to user 
> visible events (so it means either the user pressed the reset button or 
> the guest reset itself).

Yes, that's right. If the documentation (or even the code) is not
precise enough, we have to fix it.

Re: [Qemu-devel] [PATCH] Reset system before loadvm

[Peter Maydell]

On 11 June 2011 10:05, Jan Kiszka <jan.kiszka@web.de> wrote:
> @@ -2074,6 +2074,7 @@ int load_vmstate(const char *name)
>         return -EINVAL;
>     }
>
> +    qemu_system_reset();
>     ret = qemu_loadvm_state(f);

This means that if we're doing a load because the user
passed -loadvm on the command line we'll end up doing
qemu_system_reset() twice (once in vl.c and then again
here). Does that matter?

-- PMM

Re: [Qemu-devel] [PATCH] Reset system before loadvm

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On 2011-06-12 19:42, Peter Maydell wrote:
> On 11 June 2011 10:05, Jan Kiszka <jan.kiszka@web.de> wrote:
>> @@ -2074,6 +2074,7 @@ int load_vmstate(const char *name)
>>         return -EINVAL;
>>     }
>>
>> +    qemu_system_reset();
>>     ret = qemu_loadvm_state(f);
> 
> This means that if we're doing a load because the user
> passed -loadvm on the command line we'll end up doing
> qemu_system_reset() twice (once in vl.c and then again
> here). Does that matter?

I don't think it's worth optimizing. E.g. we already reset every PCI
device twice during normal reset.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH 1/2] Allow silent system resets

[Jan Kiszka]

This allows qemu_system_reset to be issued silently for internal
purposes, ie. without sending out a monitor event. Convert the system
reset after startup to the silent mode.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 sysemu.h  |    5 ++++-
 vl.c      |   10 ++++++----
 xen-all.c |    2 +-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/sysemu.h b/sysemu.h
index 7e70daa..d3013f5 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -34,6 +34,9 @@ void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
 #define VMSTOP_LOADVM    7
 #define VMSTOP_MIGRATE   8
 
+#define VMRESET_SILENT   false
+#define VMRESET_REPORT   true
+
 void vm_start(void);
 void vm_stop(int reason);
 
@@ -50,7 +53,7 @@ int qemu_powerdown_requested(void);
 void qemu_system_killed(int signal, pid_t pid);
 void qemu_kill_report(void);
 extern qemu_irq qemu_system_powerdown;
-void qemu_system_reset(void);
+void qemu_system_reset(bool report);
 
 void qemu_add_exit_notifier(Notifier *notify);
 void qemu_remove_exit_notifier(Notifier *notify);
diff --git a/vl.c b/vl.c
index d7f905d..91380d2 100644
--- a/vl.c
+++ b/vl.c
@@ -1249,7 +1249,7 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
     }
 }
 
-void qemu_system_reset(void)
+void qemu_system_reset(bool report)
 {
     QEMUResetEntry *re, *nre;
 
@@ -1257,7 +1257,9 @@ void qemu_system_reset(void)
     QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
         re->func(re->opaque);
     }
-    monitor_protocol_event(QEVENT_RESET, NULL);
+    if (report) {
+        monitor_protocol_event(QEVENT_RESET, NULL);
+    }
     cpu_synchronize_all_post_reset();
 }
 
@@ -1399,7 +1401,7 @@ static void main_loop(void)
         if (qemu_reset_requested()) {
             pause_all_vcpus();
             cpu_synchronize_all_states();
-            qemu_system_reset();
+            qemu_system_reset(VMRESET_REPORT);
             resume_all_vcpus();
         }
         if (qemu_powerdown_requested()) {
@@ -3272,7 +3274,7 @@ int main(int argc, char **argv, char **envp)
     qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
     qemu_run_machine_init_done_notifiers();
 
-    qemu_system_reset();
+    qemu_system_reset(VMRESET_SILENT);
     if (loadvm) {
         if (load_vmstate(loadvm) < 0) {
             autostart = 0;
diff --git a/xen-all.c b/xen-all.c
index 0eac202..41fd98a 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -452,7 +452,7 @@ static void cpu_handle_ioreq(void *opaque)
                 destroy_hvm_domain();
             }
             if (qemu_reset_requested_get()) {
-                qemu_system_reset();
+                qemu_system_reset(VMRESET_REPORT);
             }
         }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] Allow silent system resets

[Luiz Capitulino]

On Tue, 14 Jun 2011 18:29:43 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> This allows qemu_system_reset to be issued silently for internal
> purposes, ie. without sending out a monitor event. Convert the system
> reset after startup to the silent mode.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Only this patch is monitor related, but I've applied the two in the
monitor branch.

Thanks.

> ---
>  sysemu.h  |    5 ++++-
>  vl.c      |   10 ++++++----
>  xen-all.c |    2 +-
>  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/sysemu.h b/sysemu.h
> index 7e70daa..d3013f5 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -34,6 +34,9 @@ void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
>  #define VMSTOP_LOADVM    7
>  #define VMSTOP_MIGRATE   8
>  
> +#define VMRESET_SILENT   false
> +#define VMRESET_REPORT   true
> +
>  void vm_start(void);
>  void vm_stop(int reason);
>  
> @@ -50,7 +53,7 @@ int qemu_powerdown_requested(void);
>  void qemu_system_killed(int signal, pid_t pid);
>  void qemu_kill_report(void);
>  extern qemu_irq qemu_system_powerdown;
> -void qemu_system_reset(void);
> +void qemu_system_reset(bool report);
>  
>  void qemu_add_exit_notifier(Notifier *notify);
>  void qemu_remove_exit_notifier(Notifier *notify);
> diff --git a/vl.c b/vl.c
> index d7f905d..91380d2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1249,7 +1249,7 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
>      }
>  }
>  
> -void qemu_system_reset(void)
> +void qemu_system_reset(bool report)
>  {
>      QEMUResetEntry *re, *nre;
>  
> @@ -1257,7 +1257,9 @@ void qemu_system_reset(void)
>      QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
>          re->func(re->opaque);
>      }
> -    monitor_protocol_event(QEVENT_RESET, NULL);
> +    if (report) {
> +        monitor_protocol_event(QEVENT_RESET, NULL);
> +    }
>      cpu_synchronize_all_post_reset();
>  }
>  
> @@ -1399,7 +1401,7 @@ static void main_loop(void)
>          if (qemu_reset_requested()) {
>              pause_all_vcpus();
>              cpu_synchronize_all_states();
> -            qemu_system_reset();
> +            qemu_system_reset(VMRESET_REPORT);
>              resume_all_vcpus();
>          }
>          if (qemu_powerdown_requested()) {
> @@ -3272,7 +3274,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
>      qemu_run_machine_init_done_notifiers();
>  
> -    qemu_system_reset();
> +    qemu_system_reset(VMRESET_SILENT);
>      if (loadvm) {
>          if (load_vmstate(loadvm) < 0) {
>              autostart = 0;
> diff --git a/xen-all.c b/xen-all.c
> index 0eac202..41fd98a 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -452,7 +452,7 @@ static void cpu_handle_ioreq(void *opaque)
>                  destroy_hvm_domain();
>              }
>              if (qemu_reset_requested_get()) {
> -                qemu_system_reset();
> +                qemu_system_reset(VMRESET_REPORT);
>              }
>          }
>  

[Qemu-devel] [PATCH v2 2/2] Reset system before loadvm

[Jan Kiszka]

In case we load the vmstate during incoming migration, we start from a
clean, default machine state as we went through system reset before. But
if we load from a snapshot, the machine can be in any state. That can
cause troubles if loading an older image which does not carry all state
information the executing QEMU requires. Hardly any device takes care of
this scenario.

However, fixing this is trivial. We just need to issue a system reset
during loadvm as well.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 savevm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/savevm.c b/savevm.c
index 939845c..8139bc7 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2073,6 +2073,7 @@ int load_vmstate(const char *name)
         return -EINVAL;
     }
 
+    qemu_system_reset(VMRESET_SILENT);
     ret = qemu_loadvm_state(f);
 
     qemu_fclose(f);
-- 
1.7.1