[Qemu-devel] [PATCH v2 0/2] i386/kvm: TSC page clocksource for Hyper-V-on-KVM fixes

[Qemu-devel] [PATCH v2 0/2] i386/kvm: TSC page clocksource for Hyper-V-on-KVM fixes

[Vitaly Kuznetsov]

Changes since v1:
- add vmstate_msr_hyperv_reenlightenment subsection to vmstate_x86_cpu
  [Paolo Bonzini]
- rebase.

Previously, Ladi was working on enabling TSC page clocksource for nested
Hyper-V-on-KVM workloads. He found out that if Hyper-V frequency MSRs are
exposed to L1 as well as INVTSC flag Hyper-V enables TSC page clocksource
to its guests. Qemu doesn't pass INVTSC by default as it is a migration
blocker.

I found out there's a different way to make Hyper-V like us: expose
Reenlightenment MSRs to it. KVM doesn't fully support the feature as
we're still unable to migrate nested environments but rudimentary support
we have there (kvm/queue only currently) is enough.

Enable Hyper-V reenlightenment MSRs and expose frequency MSRs even without
INVTSC to make things work.

Vitaly Kuznetsov (2):
  i386/kvm: add support for Hyper-V reenlightenment MSRs
  i386/kvm: lower requirements for Hyper-V frequency MSRs exposure

 target/i386/cpu.h          |  3 +++
 target/i386/hyperv-proto.h |  9 ++++++++-
 target/i386/kvm.c          | 35 ++++++++++++++++++++++++++++++++++-
 target/i386/machine.c      | 24 ++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 2 deletions(-)

-- 
2.14.3

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

[Vitaly Kuznetsov]

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;
+        }
     }
     if (cpu->hyperv_crash && has_msr_hv_crash) {
         env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE;
@@ -1185,6 +1191,9 @@ static int kvm_get_supported_msrs(KVMState *s)
                 case HV_X64_MSR_TSC_FREQUENCY:
                     has_msr_hv_frequencies = true;
                     break;
+                case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
+                    has_msr_hv_reenlightenment = true;
+                    break;
                 case MSR_IA32_SPEC_CTRL:
                     has_msr_spec_ctrl = true;
                     break;
@@ -1747,6 +1756,15 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
             if (cpu->hyperv_time) {
                 kvm_msr_entry_add(cpu, HV_X64_MSR_REFERENCE_TSC,
                                   env->msr_hv_tsc);
+
+                if (has_msr_hv_reenlightenment) {
+                    kvm_msr_entry_add(cpu, HV_X64_MSR_REENLIGHTENMENT_CONTROL,
+                                      env->msr_hv_reenlightenment_control);
+                    kvm_msr_entry_add(cpu, HV_X64_MSR_TSC_EMULATION_CONTROL,
+                                      env->msr_hv_tsc_emulation_control);
+                    kvm_msr_entry_add(cpu, HV_X64_MSR_TSC_EMULATION_STATUS,
+                                      env->msr_hv_tsc_emulation_status);
+                }
             }
         }
         if (cpu->hyperv_vapic) {
@@ -2109,6 +2127,12 @@ static int kvm_get_msrs(X86CPU *cpu)
     }
     if (cpu->hyperv_time) {
         kvm_msr_entry_add(cpu, HV_X64_MSR_REFERENCE_TSC, 0);
+
+        if (has_msr_hv_reenlightenment) {
+            kvm_msr_entry_add(cpu, HV_X64_MSR_REENLIGHTENMENT_CONTROL, 0);
+            kvm_msr_entry_add(cpu, HV_X64_MSR_TSC_EMULATION_CONTROL, 0);
+            kvm_msr_entry_add(cpu, HV_X64_MSR_TSC_EMULATION_STATUS, 0);
+        }
     }
     if (has_msr_hv_crash) {
         int j;
@@ -2367,6 +2391,15 @@ static int kvm_get_msrs(X86CPU *cpu)
             env->msr_hv_stimer_count[(index - HV_X64_MSR_STIMER0_COUNT)/2] =
                                 msrs[i].data;
             break;
+        case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
+            env->msr_hv_reenlightenment_control = msrs[i].data;
+            break;
+        case HV_X64_MSR_TSC_EMULATION_CONTROL:
+            env->msr_hv_tsc_emulation_control = msrs[i].data;
+            break;
+        case HV_X64_MSR_TSC_EMULATION_STATUS:
+            env->msr_hv_tsc_emulation_status = msrs[i].data;
+            break;
         case MSR_MTRRdefType:
             env->mtrr_deftype = msrs[i].data;
             break;
diff --git a/target/i386/machine.c b/target/i386/machine.c
index bd2d82e91b..fd99c0bbb4 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -713,6 +713,29 @@ static const VMStateDescription vmstate_msr_hyperv_stimer = {
     }
 };
 
+static bool hyperv_reenlightenment_enable_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+
+    return env->msr_hv_reenlightenment_control != 0 ||
+        env->msr_hv_tsc_emulation_control != 0 ||
+        env->msr_hv_tsc_emulation_status != 0;
+}
+
+static const VMStateDescription vmstate_msr_hyperv_reenlightenment = {
+    .name = "cpu/msr_hyperv_reenlightenment",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = hyperv_reenlightenment_enable_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(env.msr_hv_reenlightenment_control, X86CPU),
+        VMSTATE_UINT64(env.msr_hv_tsc_emulation_control, X86CPU),
+        VMSTATE_UINT64(env.msr_hv_tsc_emulation_status, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static bool avx512_needed(void *opaque)
 {
     X86CPU *cpu = opaque;
@@ -1005,6 +1028,7 @@ VMStateDescription vmstate_x86_cpu = {
         &vmstate_msr_hyperv_runtime,
         &vmstate_msr_hyperv_synic,
         &vmstate_msr_hyperv_stimer,
+        &vmstate_msr_hyperv_reenlightenment,
         &vmstate_avx512,
         &vmstate_xss,
         &vmstate_tsc_khz,
-- 
2.14.3

[Qemu-devel] [PATCH v2 2/2] i386/kvm: lower requirements for Hyper-V frequency MSRs exposure

[Vitaly Kuznetsov]

Requiring tsc_is_stable_and_known() is too restrictive: even without INVTCS
nested Hyper-V-on-KVM enables TSC pages for its guests e.g. when
Reenlightenment MSRs are present. Presence of frequency MSRs doesn't mean
these frequencies are stable, it just means they're available for reading.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index accf50eac3..a6d1210f46 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -650,7 +650,7 @@ static int hyperv_handle_properties(CPUState *cs)
         env->features[FEAT_HYPERV_EAX] |= HV_TIME_REF_COUNT_AVAILABLE;
         env->features[FEAT_HYPERV_EAX] |= HV_REFERENCE_TSC_AVAILABLE;
 
-        if (has_msr_hv_frequencies && tsc_is_stable_and_known(env)) {
+        if (has_msr_hv_frequencies && env->tsc_khz) {
             env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
             env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
         }
-- 
2.14.3

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

[Roman Kagan]

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[]?

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.
(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.)

Thanks,
Roman.

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

[Vitaly Kuznetsov]

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

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

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

[Roman Kagan]

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.

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

[Paolo Bonzini]

On 19/03/2018 18:06, Roman Kagan wrote:
>> +        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[]?
> 
> 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.
> (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.)

Yeah, so far for frequencies that was okay or at least okay-ish because
Windows didn't use it by default without invtsc (disables migration).

However, we do need a new hv_reenlight property; if it is not set, QEMU
should behave as if the reenlightenment MSRs are not provided by KVM.

Thanks,

Paolo

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

[Eduardo Habkost]

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]
> >> ---
[...]
> >> +
> >> +        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).

You will need "partial" emulation if you want to support
live-migration to/from a host where the KVM or QEMU don't support
all the features from the current host.

Is this something the current Hyper-V code already supports, or
it's something known to be broken?

-- 
Eduardo

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

[Roman Kagan]

On Tue, Mar 20, 2018 at 03:16:52PM -0300, Eduardo Habkost wrote:
> On Mon, Mar 19, 2018 at 06:29:08PM +0100, Vitaly Kuznetsov wrote:
> > Roman Kagan <rkagan@virtuozzo.com> writes:
> > > (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).
> 
> You will need "partial" emulation if you want to support
> live-migration to/from a host where the KVM or QEMU don't support
> all the features from the current host.
> 
> Is this something the current Hyper-V code already supports, or
> it's something known to be broken?

There's at least one case I mentioned above where it's broken, but, as
Paolo pointed out, the real Windows guests wouldn't notice because they
didn't use the feature due to missing INVTSC.

I myself haven't tested all possible combinations of hv_* flags and KVM
versions, but from code inspection I don't immediately see anything else
that's obviously broken in this regard.

Roman.

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

[Paolo Bonzini]

On 20/03/2018 19:16, Eduardo Habkost wrote:
>> 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).
> You will need "partial" emulation if you want to support
> live-migration to/from a host where the KVM or QEMU don't support
> all the features from the current host.
> 
> Is this something the current Hyper-V code already supports, or
> it's something known to be broken?

Yes, it works.

Essentially, multiple Hyper-V features are (just like multiple KVM
features) needed to support old kernels.  The difference is that new KVM
features are added quite rarely, and most of them are enabled by
default.  The opposite is true of Hyper-V, on both counts.

Paolo