| Message ID | 4DF32FC6.3040607@web.de |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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.
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
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).
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.
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);