| Message ID | 20180322131358.15096-3-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 > >
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.
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 >>
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.
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.
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; }
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(-)