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

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

[Vitaly Kuznetsov]

Changes since v3:
- check cpu->hyperv_reenlightenment instead of has_msr_reenlightenment
  [Eduardo Habkost, Roman Kagan]
- expose frequency MSRs only when either INVTSC or Reenlightenment is
  provided [Paolo Bonzini] (I'm not doing 'hv_frequency' property for now
  as the discussion around it seems to be inconclusive.)

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 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: expose Hyper-V frequency MSRs with reenlightenment

 target/i386/cpu.c          |  4 +++-
 target/i386/cpu.h          |  4 ++++
 target/i386/hyperv-proto.h |  9 ++++++++-
 target/i386/kvm.c          | 41 +++++++++++++++++++++++++++++++++++++++--
 target/i386/machine.c      | 24 ++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 4 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v4 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 v3:
- check cpu->hyperv_reenlightenment instead of has_msr_reenlightenment
  [Eduardo Habkost, Roman Kagan]
---
 target/i386/cpu.c          |  4 +++-
 target/i386/cpu.h          |  4 ++++
 target/i386/hyperv-proto.h |  9 ++++++++-
 target/i386/kvm.c          | 38 +++++++++++++++++++++++++++++++++++++-
 target/i386/machine.c      | 24 ++++++++++++++++++++++++
 5 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6bb4ce8719..02579f8234 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -407,7 +407,8 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL /* hv_vpindex_access */, NULL /* hv_msr_reset_access */,
             NULL /* hv_msr_stats_access */, NULL /* hv_reftsc_access */,
             NULL /* hv_msr_idle_access */, NULL /* hv_msr_frequency_access */,
-            NULL, NULL, NULL, NULL,
+            NULL /* hv_msr_debug_access */, NULL /* hv_msr_reenlightenment_access */,
+            NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
@@ -4764,6 +4765,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false),
     DEFINE_PROP_BOOL("hv-synic", X86CPU, hyperv_synic, false),
     DEFINE_PROP_BOOL("hv-stimer", X86CPU, hyperv_stimer, false),
+    DEFINE_PROP_BOOL("hv-reenlightenment", X86CPU, hyperv_reenlightenment, false),
     DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 2e2bab5ff3..98eed72937 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;
@@ -1296,6 +1299,7 @@ struct X86CPU {
     bool hyperv_runtime;
     bool hyperv_synic;
     bool hyperv_stimer;
+    bool hyperv_reenlightenment;
     bool check_cpuid;
     bool enforce_cpuid;
     bool expose_kvm;
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..75f4e1d69e 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;
@@ -583,7 +584,8 @@ static bool hyperv_enabled(X86CPU *cpu)
             cpu->hyperv_vpindex ||
             cpu->hyperv_runtime ||
             cpu->hyperv_synic ||
-            cpu->hyperv_stimer);
+            cpu->hyperv_stimer ||
+            cpu->hyperv_reenlightenment);
 }
 
 static int kvm_arch_set_tsc_khz(CPUState *cs)
@@ -654,6 +656,14 @@ static int hyperv_handle_properties(CPUState *cs)
             env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
         }
     }
+    if (cpu->hyperv_reenlightenment) {
+        if (!has_msr_hv_reenlightenment) {
+            fprintf(stderr,
+                    "Hyper-V Reenlightenment is not supported by kernel\n");
+            return -ENOSYS;
+        }
+        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 +1195,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;
@@ -1748,6 +1761,14 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
                 kvm_msr_entry_add(cpu, HV_X64_MSR_REFERENCE_TSC,
                                   env->msr_hv_tsc);
             }
+            if (cpu->hyperv_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) {
             kvm_msr_entry_add(cpu, HV_X64_MSR_APIC_ASSIST_PAGE,
@@ -2109,6 +2130,12 @@ static int kvm_get_msrs(X86CPU *cpu)
     }
     if (cpu->hyperv_time) {
         kvm_msr_entry_add(cpu, HV_X64_MSR_REFERENCE_TSC, 0);
+
+    }
+    if (cpu->hyperv_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 +2394,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 v4 2/2] i386/kvm: expose Hyper-V frequency MSRs with reenlightenment

[Vitaly Kuznetsov]

We can also expose Hyper-V frequency MSRs when reenlightenment feature is
enabled and TSC frequency is known, Hyper-V on KVM will provide stable TSC
page clocksources to its guests.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
- Expose frequency MSRs only when either INVTSC or Reenlightenment is
  provided [Paolo Bonzini]
---
 target/i386/kvm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 75f4e1d69e..2c3c19d690 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -651,7 +651,8 @@ 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 &&
+            (tsc_is_stable_and_known(env) || cpu->hyperv_reenlightenment)) {
             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 v4 2/2] i386/kvm: expose Hyper-V frequency MSRs with reenlightenment

[Eduardo Habkost]

On Thu, Mar 22, 2018 at 02:13:58PM +0100, Vitaly Kuznetsov wrote:
> We can also expose Hyper-V frequency MSRs when reenlightenment feature is
> enabled and TSC frequency is known, Hyper-V on KVM will provide stable TSC
> page clocksources to its guests.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> - Expose frequency MSRs only when either INVTSC or Reenlightenment is
>   provided [Paolo Bonzini]
> ---
>  target/i386/kvm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 75f4e1d69e..2c3c19d690 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -651,7 +651,8 @@ 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 &&

Why is the check for env->tsc_khz necessary?

Are there known circumstances where HV_X64_MSR_TSC_FREQUENCY will be supported
by KVM but ioctl(KVM_GET_TSC_KHZ) will return 0, or this is just for extra
safety?

> +            (tsc_is_stable_and_known(env) || cpu->hyperv_reenlightenment)) {
>              env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
>              env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
>          }
> -- 
> 2.14.3
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/2] i386/kvm: expose Hyper-V frequency MSRs with reenlightenment

[Eduardo Habkost]

On Thu, Mar 22, 2018 at 02:13:58PM +0100, Vitaly Kuznetsov wrote:
> We can also expose Hyper-V frequency MSRs when reenlightenment feature is
> enabled and TSC frequency is known, Hyper-V on KVM will provide stable TSC
> page clocksources to its guests.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> - Expose frequency MSRs only when either INVTSC or Reenlightenment is
>   provided [Paolo Bonzini]
> ---
>  target/i386/kvm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 75f4e1d69e..2c3c19d690 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -651,7 +651,8 @@ 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 &&
> +            (tsc_is_stable_and_known(env) || cpu->hyperv_reenlightenment)) {

Continuing the discussion we had in v3 about being possible to
crash when migrating to a host running an older kernel:

This patch doesn't fix the issue, because it is still implicitly
enabling hv-frequencies.

But I don't think this patch will make the situation any worse,
because we don't have any KVM versions that support
HV_X64_MSR_REENLIGHTENMENT_CONTROL but not
HV_X64_MSR_TSC_FREQUENCY.

This means that we can safely make "hv-reenlightenment=on"
automatically set "hv-frequencies=on", when we finally introduce
a hv-frequencies property.

Roman, do you agree?

The next question is: do we need to fix this and introduce a
hv-frequencies property in 2.12, or can this wait for 2.13?


>              env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
>              env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
>          }
> -- 
> 2.14.3
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/2] i386/kvm: expose Hyper-V frequency MSRs with reenlightenment

[Roman Kagan]

On Thu, Mar 22, 2018 at 04:01:46PM -0300, Eduardo Habkost wrote:
> On Thu, Mar 22, 2018 at 02:13:58PM +0100, Vitaly Kuznetsov wrote:
> > We can also expose Hyper-V frequency MSRs when reenlightenment feature is
> > enabled and TSC frequency is known, Hyper-V on KVM will provide stable TSC
> > page clocksources to its guests.
> > 
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > ---
> > - Expose frequency MSRs only when either INVTSC or Reenlightenment is
> >   provided [Paolo Bonzini]
> > ---
> >  target/i386/kvm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > index 75f4e1d69e..2c3c19d690 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -651,7 +651,8 @@ 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 &&
> > +            (tsc_is_stable_and_known(env) || cpu->hyperv_reenlightenment)) {
> 
> Continuing the discussion we had in v3 about being possible to
> crash when migrating to a host running an older kernel:
> 
> This patch doesn't fix the issue, because it is still implicitly
> enabling hv-frequencies.
> 
> But I don't think this patch will make the situation any worse,
> because we don't have any KVM versions that support
> HV_X64_MSR_REENLIGHTENMENT_CONTROL but not
> HV_X64_MSR_TSC_FREQUENCY.
> 
> This means that we can safely make "hv-reenlightenment=on"
> automatically set "hv-frequencies=on", when we finally introduce
> a hv-frequencies property.

Do I get you right that there's no controversy about the need for
hv-frequencies?

> Roman, do you agree?

Well, this strictly depends on the answer to your next question, because
hv-reenlightenment is most certainly 2.13 material.

> The next question is: do we need to fix this and introduce a
> hv-frequencies property in 2.12, or can this wait for 2.13?

I'm tempted to stick hv-frequencies into 2.12 and retrospectively into
2.11-stable, to minimize the exposure to the problem but to keep the
effort and added complexity reasonable.  But I'm not sure this will be
ok from the compatibility policy standpoint.  So I'd appreciate an
advice on this.
[Should I better just post a patch doing that and continue this
discussion there?]

Thanks,
Roman.

Re: [Qemu-devel] [PATCH v4 2/2] i386/kvm: expose Hyper-V frequency MSRs with reenlightenment

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Mar 22, 2018 at 02:13:58PM +0100, Vitaly Kuznetsov wrote:
>> We can also expose Hyper-V frequency MSRs when reenlightenment feature is
>> enabled and TSC frequency is known, Hyper-V on KVM will provide stable TSC
>> page clocksources to its guests.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> - Expose frequency MSRs only when either INVTSC or Reenlightenment is
>>   provided [Paolo Bonzini]
>> ---
>>  target/i386/kvm.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 75f4e1d69e..2c3c19d690 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -651,7 +651,8 @@ 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 &&
>
> Why is the check for env->tsc_khz necessary?
>
> Are there known circumstances where HV_X64_MSR_TSC_FREQUENCY will be supported
> by KVM but ioctl(KVM_GET_TSC_KHZ) will return 0, or this is just for extra
> safety?
>

Yes,

I didn't experiment with passing '0' to Windows but in general it
doesn't sound like a good idea. 

>> +            (tsc_is_stable_and_known(env) || cpu->hyperv_reenlightenment)) {
>>              env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
>>              env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
>>          }
>> -- 
>> 2.14.3
>> 

-- 
  Vitaly

Re: [Qemu-devel] [PATCH v4 2/2] i386/kvm: expose Hyper-V frequency MSRs with reenlightenment

[Roman Kagan]

On Fri, Mar 23, 2018 at 03:45:49PM +0100, Vitaly Kuznetsov wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Mar 22, 2018 at 02:13:58PM +0100, Vitaly Kuznetsov wrote:
> >> We can also expose Hyper-V frequency MSRs when reenlightenment feature is
> >> enabled and TSC frequency is known, Hyper-V on KVM will provide stable TSC
> >> page clocksources to its guests.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >> - Expose frequency MSRs only when either INVTSC or Reenlightenment is
> >>   provided [Paolo Bonzini]
> >> ---
> >>  target/i386/kvm.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> >> index 75f4e1d69e..2c3c19d690 100644
> >> --- a/target/i386/kvm.c
> >> +++ b/target/i386/kvm.c
> >> @@ -651,7 +651,8 @@ 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 &&
> >
> > Why is the check for env->tsc_khz necessary?
> >
> > Are there known circumstances where HV_X64_MSR_TSC_FREQUENCY will be supported
> > by KVM but ioctl(KVM_GET_TSC_KHZ) will return 0, or this is just for extra
> > safety?
> >
> 
> Yes,
> 
> I didn't experiment with passing '0' to Windows but in general it
> doesn't sound like a good idea. 

AFAICS the value of ->tsc_khz that QEMU pushes into KVM is the one that
it first obtains from KVM via ioctl(KVM_GET_TSC_KHZ), or receives in the
migration stream (obtained in a similar way on the source VM).  So for
all relevant configurations this check seems indeed redundant, and I
went ahead and dropped it in the patch I posted.  Did I miss any case
where it is not?

Thanks,
Roman.

Re: [Qemu-devel] [PATCH v4 2/2] i386/kvm: expose Hyper-V frequency MSRs with reenlightenment

[Eduardo Habkost]

On Mon, Mar 26, 2018 at 05:25:47PM +0300, Roman Kagan wrote:
> On Fri, Mar 23, 2018 at 03:45:49PM +0100, Vitaly Kuznetsov wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > 
> > > On Thu, Mar 22, 2018 at 02:13:58PM +0100, Vitaly Kuznetsov wrote:
> > >> We can also expose Hyper-V frequency MSRs when reenlightenment feature is
> > >> enabled and TSC frequency is known, Hyper-V on KVM will provide stable TSC
> > >> page clocksources to its guests.
> > >> 
> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > >> ---
> > >> - Expose frequency MSRs only when either INVTSC or Reenlightenment is
> > >>   provided [Paolo Bonzini]
> > >> ---
> > >>  target/i386/kvm.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > >> index 75f4e1d69e..2c3c19d690 100644
> > >> --- a/target/i386/kvm.c
> > >> +++ b/target/i386/kvm.c
> > >> @@ -651,7 +651,8 @@ 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 &&
> > >
> > > Why is the check for env->tsc_khz necessary?
> > >
> > > Are there known circumstances where HV_X64_MSR_TSC_FREQUENCY will be supported
> > > by KVM but ioctl(KVM_GET_TSC_KHZ) will return 0, or this is just for extra
> > > safety?
> > >
> > 
> > Yes,
> > 
> > I didn't experiment with passing '0' to Windows but in general it
> > doesn't sound like a good idea. 
> 
> AFAICS the value of ->tsc_khz that QEMU pushes into KVM is the one that
> it first obtains from KVM via ioctl(KVM_GET_TSC_KHZ), or receives in the
> migration stream (obtained in a similar way on the source VM).  So for
> all relevant configurations this check seems indeed redundant, and I
> went ahead and dropped it in the patch I posted.  Did I miss any case
> where it is not?

That's my impression as well.

-- 
Eduardo