Re: [PATCH] target/i386: always create kvmclock device

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Antoine Damhet <antoine.damhet@blade-group.com> writes:

> On Thu, Sep 17, 2020 at 01:13:06PM +0200, Vitaly Kuznetsov wrote:
>> QEMU's kvmclock device is only created when KVM PV feature bits for
>> kvmclock (KVM_FEATURE_CLOCKSOURCE/KVM_FEATURE_CLOCKSOURCE2) are
>> exposed to the guest. With 'kvm=off' cpu flag the device is not
>> created and we don't call KVM_GET_CLOCK/KVM_SET_CLOCK upon migration.
>> It was reported that without these call at least Hyper-V TSC page
>> clocksouce (which can be enabled independently) gets broken after
>> migration.
>> 
>> Switch to creating kvmclock QEMU device unconditionally, it seems
>> to always make sense to call KVM_GET_CLOCK/KVM_SET_CLOCK on migration.
>> Use KVM_CAP_ADJUST_CLOCK check instead of CPUID feature bits.
>> 
>> Reported-by: Antoine Damhet <antoine.damhet@blade-group.com>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  hw/i386/kvm/clock.c    | 6 +-----
>>  target/i386/kvm.c      | 5 +++++
>>  target/i386/kvm_i386.h | 1 +
>>  3 files changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
>> index 64283358f91d..526c9ea5172b 100644
>> --- a/hw/i386/kvm/clock.c
>> +++ b/hw/i386/kvm/clock.c
>> @@ -330,11 +330,7 @@ static const TypeInfo kvmclock_info = {
>>  /* Note: Must be called after VCPU initialization. */
>>  void kvmclock_create(void)
>>  {
>> -    X86CPU *cpu = X86_CPU(first_cpu);
>> -
>> -    if (kvm_enabled() &&
>> -        cpu->env.features[FEAT_KVM] & ((1ULL << KVM_FEATURE_CLOCKSOURCE) |
>> -                                       (1ULL << KVM_FEATURE_CLOCKSOURCE2))) {
>> +    if (kvm_enabled() && kvm_has_adjust_clock()) {
>
> Shouldn't the old check used when machine type <= 5.1 in order to avoid
> migration incompatibility ?

Hm, when the check fails we just don't create the device and no error is
reported, so even if we have kvmclock data in the migration stream but
fail to create it migration will still succeed, right? (not a migration
expert here :-)

>
>>          sysbus_create_simple(TYPE_KVM_CLOCK, -1, NULL);
>>      }
>>  }
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 4a8b3a41c1bc..20b31b65307b 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -143,6 +143,11 @@ bool kvm_has_adjust_clock_stable(void)
>>      return (ret == KVM_CLOCK_TSC_STABLE);
>>  }
>>  
>> +bool kvm_has_adjust_clock(void)
>> +{
>> +    return kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK);
>> +}
>> +
>>  bool kvm_has_exception_payload(void)
>>  {
>>      return has_exception_payload;
>> diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h
>> index 064b8798a26c..0fce4e51d2d6 100644
>> --- a/target/i386/kvm_i386.h
>> +++ b/target/i386/kvm_i386.h
>> @@ -34,6 +34,7 @@
>>  
>>  bool kvm_allows_irq0_override(void);
>>  bool kvm_has_smm(void);
>> +bool kvm_has_adjust_clock(void);
>>  bool kvm_has_adjust_clock_stable(void);
>>  bool kvm_has_exception_payload(void);
>>  void kvm_synchronize_all_tsc(void);
>> -- 
>> 2.25.4
>> 
>> 

-- 
Vitaly