[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/i386: always create kvmclock device
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PATCH] target/i386: always create kvmclock device |
Date: |
Thu, 17 Sep 2020 18:44:10 +0100 |
User-agent: |
Mutt/1.14.6 (2020-07-11) |
* Vitaly Kuznetsov (vkuznets@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
> > * Vitaly Kuznetsov (vkuznets@redhat.com) wrote:
> >> 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 :-)
> >
> > When the migration stream is parsed, it'll try and find a "kvmclock"
> > device to pass the data it's reading to; if one doesn't exist it'll
> > fail.
>
> This may happen with an older machine type when the destination is
> running an unfixed QEMU and the source has the fix, right?
Yes I think so.
> The solution
> would be to introduce a flag for older machine types (or for new ones)
> like 'kvmclock_always'.
Yep sounds the normal answer.
(You might want to try it first to trigger the bug)
> > The other question is in the incoming direction from an older VM;
> > you'll have a kvm clock created here, but you won't load the kvm clock
> > state from the migration stream - what is this clock going to do?
>
> This is not really a problem I believe: the clock was absent on the
> source and things somehow worked for the guest so even if we don't
> initialize kvmclock properly on the destination nothing bad is expected.
OK.
Dave
> >
> > Dave
> >
> >> >
> >> >> 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
> >>
>
> --
> Vitaly
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
- [PATCH] target/i386: always create kvmclock device, Vitaly Kuznetsov, 2020/09/17
- Re: [PATCH] target/i386: always create kvmclock device, no-reply, 2020/09/17
- Re: [PATCH] target/i386: always create kvmclock device, Antoine Damhet, 2020/09/17
- Re: [PATCH] target/i386: always create kvmclock device, Vitaly Kuznetsov, 2020/09/17
- Re: [PATCH] target/i386: always create kvmclock device, Dr. David Alan Gilbert, 2020/09/17
- Re: [PATCH] target/i386: always create kvmclock device, Vitaly Kuznetsov, 2020/09/17
- Re: [PATCH] target/i386: always create kvmclock device,
Dr. David Alan Gilbert <=
- Re: [PATCH] target/i386: always create kvmclock device, Paolo Bonzini, 2020/09/18
- Re: [PATCH] target/i386: always create kvmclock device, Antoine Damhet, 2020/09/18
- Re: [PATCH] target/i386: always create kvmclock device, Dr. David Alan Gilbert, 2020/09/18
- Re: [PATCH] target/i386: always create kvmclock device, Vitaly Kuznetsov, 2020/09/22
- Re: [PATCH] target/i386: always create kvmclock device, Antoine Damhet, 2020/09/17
Re: [PATCH] target/i386: always create kvmclock device, Paolo Bonzini, 2020/09/18