If the VM is paused when the KVM clock is serialized to a file, record
that the clock is valid, so the value will be reused rather than
overwritten after cprload with a new call to KVM_GET_CLOCK here:

kvmclock_vm_state_change()
    if (running)
        ...
    else
        if (s->clock_valid)
            return;         <-- instead, return here

        kvm_update_clock()
           kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data)  <-- overwritten

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/i386/kvm/clock.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 6428335..161991a 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -285,18 +285,22 @@ static int kvmclock_pre_save(void *opaque)
     if (!s->runstate_paused) {
         kvm_update_clock(s);
     }
+    if (!runstate_is_running()) {
+        s->clock_valid = true;
+    }
 
     return 0;
 }
 
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .pre_load = kvmclock_pre_load,
     .pre_save = kvmclock_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
+        VMSTATE_BOOL_V(clock_valid, KVMClockState, 2),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription * []) {
-- 
1.8.3.1

* Steve Sistare (steven.sistare@oracle.com) wrote:
> If the VM is paused when the KVM clock is serialized to a file, record
> that the clock is valid, so the value will be reused rather than
> overwritten after cprload with a new call to KVM_GET_CLOCK here:
> 
> kvmclock_vm_state_change()
>     if (running)
>         ...
>     else
>         if (s->clock_valid)
>             return;         <-- instead, return here
> 
>         kvm_update_clock()
>            kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data)  <-- overwritten
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  hw/i386/kvm/clock.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 6428335..161991a 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -285,18 +285,22 @@ static int kvmclock_pre_save(void *opaque)
>      if (!s->runstate_paused) {
>          kvm_update_clock(s);
>      }
> +    if (!runstate_is_running()) {
> +        s->clock_valid = true;
> +    }
>  
>      return 0;
>  }
>  
>  static const VMStateDescription kvmclock_vmsd = {
>      .name = "kvmclock",
> -    .version_id = 1,
> +    .version_id = 2,
>      .minimum_version_id = 1,
>      .pre_load = kvmclock_pre_load,
>      .pre_save = kvmclock_pre_save,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64(clock, KVMClockState),
> +        VMSTATE_BOOL_V(clock_valid, KVMClockState, 2),
>          VMSTATE_END_OF_LIST()

We always try and avoid bumping version_id unless we're
desperate because it breaks backwards migration.

Didn't you already know from the stored migration state
(in the globalstate) if the loaded VM was running?

It's also not clear to me why you're avoiding reloading the state;
have you preserved that some other way?

Dave

>      },
>      .subsections = (const VMStateDescription * []) {
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 9/11/2020 1:50 PM, Dr. David Alan Gilbert wrote:
> * Steve Sistare (steven.sistare@oracle.com) wrote:
>> If the VM is paused when the KVM clock is serialized to a file, record
>> that the clock is valid, so the value will be reused rather than
>> overwritten after cprload with a new call to KVM_GET_CLOCK here:
>>
>> kvmclock_vm_state_change()
>>     if (running)
>>         ...
>>     else
>>         if (s->clock_valid)
>>             return;         <-- instead, return here
>>
>>         kvm_update_clock()
>>            kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data)  <-- overwritten
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  hw/i386/kvm/clock.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
>> index 6428335..161991a 100644
>> --- a/hw/i386/kvm/clock.c
>> +++ b/hw/i386/kvm/clock.c
>> @@ -285,18 +285,22 @@ static int kvmclock_pre_save(void *opaque)
>>      if (!s->runstate_paused) {
>>          kvm_update_clock(s);
>>      }
>> +    if (!runstate_is_running()) {
>> +        s->clock_valid = true;
>> +    }
>>  
>>      return 0;
>>  }
>>  
>>  static const VMStateDescription kvmclock_vmsd = {
>>      .name = "kvmclock",
>> -    .version_id = 1,
>> +    .version_id = 2,
>>      .minimum_version_id = 1,
>>      .pre_load = kvmclock_pre_load,
>>      .pre_save = kvmclock_pre_save,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT64(clock, KVMClockState),
>> +        VMSTATE_BOOL_V(clock_valid, KVMClockState, 2),
>>          VMSTATE_END_OF_LIST()
> 
> We always try and avoid bumping version_id unless we're
> desperate because it breaks backwards migration.
> 
> Didn't you already know from the stored migration state
> (in the globalstate) if the loaded VM was running?
> 
> It's also not clear to me why you're avoiding reloading the state;
> have you preserved that some other way?

This patch was needed only for an early version of cprload which had some gratuitous
vmstate transitions.  I will happily drop this patch.

- Steve

>>      },
>>      .subsections = (const VMStateDescription * []) {
>> -- 
>> 1.8.3.1
>>