Roman Kagan <rka...@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 <vkuzn...@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).
> (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).
--
Vitaly
