Re: [Qemu-devel] [PATCH v2 1/2] i386/kvm: add support for Hyper-V reenlightenment MSRs

[Roman Kagan <rkagan@virtuozzo.com>]

On Mon, Mar 19, 2018 at 06:29:08PM +0100, Vitaly Kuznetsov wrote:
> Roman Kagan <rkagan@virtuozzo.com> writes:
> 
> > On Fri, Mar 16, 2018 at 06:00:19PM +0100, Vitaly Kuznetsov wrote:
> >> KVM recently gained support for Hyper-V Reenlightenment MSRs which are
> >> required to make KVM-on-Hyper-V enable TSC page clocksource to its guests
> >> when INVTSC is not passed to it (and it is not passed by default in Qemu
> >> as it effectively blocks migration).
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >> Changes since v1:
> >> - add vmstate_msr_hyperv_reenlightenment subsection to vmstate_x86_cpu
> >>   [Paolo Bonzini]
> >> ---
> >>  target/i386/cpu.h          |  3 +++
> >>  target/i386/hyperv-proto.h |  9 ++++++++-
> >>  target/i386/kvm.c          | 33 +++++++++++++++++++++++++++++++++
> >>  target/i386/machine.c      | 24 ++++++++++++++++++++++++
> >>  4 files changed, 68 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> >> index 2e2bab5ff3..0b1b556a56 100644
> >> --- a/target/i386/cpu.h
> >> +++ b/target/i386/cpu.h
> >> @@ -1174,6 +1174,9 @@ typedef struct CPUX86State {
> >>      uint64_t msr_hv_synic_sint[HV_SINT_COUNT];
> >>      uint64_t msr_hv_stimer_config[HV_STIMER_COUNT];
> >>      uint64_t msr_hv_stimer_count[HV_STIMER_COUNT];
> >> +    uint64_t msr_hv_reenlightenment_control;
> >> +    uint64_t msr_hv_tsc_emulation_control;
> >> +    uint64_t msr_hv_tsc_emulation_status;
> >>  
> >>      uint64_t msr_rtit_ctrl;
> >>      uint64_t msr_rtit_status;
> >> diff --git a/target/i386/hyperv-proto.h b/target/i386/hyperv-proto.h
> >> index cb4d7f2b7a..93352ebd2a 100644
> >> --- a/target/i386/hyperv-proto.h
> >> +++ b/target/i386/hyperv-proto.h
> >> @@ -35,7 +35,7 @@
> >>  #define HV_RESET_AVAILABLE           (1u << 7)
> >>  #define HV_REFERENCE_TSC_AVAILABLE   (1u << 9)
> >>  #define HV_ACCESS_FREQUENCY_MSRS     (1u << 11)
> >> -
> >> +#define HV_ACCESS_REENLIGHTENMENTS_CONTROL  (1u << 13)
> >>  
> >>  /*
> >>   * HV_CPUID_FEATURES.EDX bits
> >> @@ -129,6 +129,13 @@
> >>  #define HV_X64_MSR_CRASH_CTL                    0x40000105
> >>  #define HV_CRASH_CTL_NOTIFY                     (1ull << 63)
> >>  
> >> +/*
> >> + * Reenlightenment notification MSRs
> >> + */
> >> +#define HV_X64_MSR_REENLIGHTENMENT_CONTROL      0x40000106
> >> +#define HV_X64_MSR_TSC_EMULATION_CONTROL        0x40000107
> >> +#define HV_X64_MSR_TSC_EMULATION_STATUS         0x40000108
> >> +
> >>  /*
> >>   * Hypercall status code
> >>   */
> >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> >> index d23fff12f5..accf50eac3 100644
> >> --- a/target/i386/kvm.c
> >> +++ b/target/i386/kvm.c
> >> @@ -90,6 +90,7 @@ static bool has_msr_hv_runtime;
> >>  static bool has_msr_hv_synic;
> >>  static bool has_msr_hv_stimer;
> >>  static bool has_msr_hv_frequencies;
> >> +static bool has_msr_hv_reenlightenment;
> >>  static bool has_msr_xss;
> >>  static bool has_msr_spec_ctrl;
> >>  static bool has_msr_smi_count;
> >> @@ -653,6 +654,11 @@ static int hyperv_handle_properties(CPUState *cs)
> >>              env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
> >>              env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
> >>          }
> >> +
> >> +        if (has_msr_hv_reenlightenment) {
> >> +            env->features[FEAT_HYPERV_EAX] |=
> >> +                HV_ACCESS_REENLIGHTENMENTS_CONTROL;
> >> +        }
> >
> > Can you please add a matching comment to the definition of
> > feature_word_info[FEAT_HYPERV_EAX].feat_names[]?
> >
> 
> Sure, missed that.
> 
> > Also there appears to be no cpu property to turn this on/off, does it?
> > It's enabled based only on the support in the KVM it's running against.
> > So I guess we may have a problem migrating between the hosts with
> > different KVM versions, one supporting it and the other not.
> 
> Currently nested workloads don't migrate so I decided to take the
> opportunity and squeeze the new feature in without adding a new
> hv_reenlightenment cpu property (which would have to be added to libvirt
> at least).

Well, it (== L1) doesn't migrate with L2 running inside, but it
certainly does without.

> 
> > (This is also a problem with has_msr_hv_frequencies, and is in general a
> > long-standing issue of hv_* properties being done differently from the
> > rest of CPUID features.)
> 
> Suggestions? (To be honest I don't really like us adding new hv_*
> property for every new Hyper-V feature we support. I doubt anyone needs
> 'partial' Hyper-V emulation. It would be nice to have a single versioned
> 'hv' feature implying everything. We may then forbid migrations to older
> hv versions. But I don't really know the history of why we decided to go
> with a separate hv_* for every feature we add).

IIRC those hv_* features were added before the generic CPUID feature bit
naming scheme was introduced.  We probably need to actually add the
respective feature names to those .feat_names arrays, introduce a
compatibility translation to/from the existing hv_* features, and
address the conflicts, but I vaguely remember an attempt to do so ended
up nowhere...

I guess for the time being we can go on with extending hv_* features,
but will need to do a conversion in (preferrably not too distant)
future.  But I'd rather have Eduardo's or Paolo's word on this.

Thanks,
Roman.