Re: [PATCH V1 10/32] kvmclock: restore paused KVM clock

[Steven Sistare]

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
>>