[Qemu-devel] [PATCH v4 2/2] i386/kvm: expose Hyper-V frequency MSRs with reenlightenment
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
---
- 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
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
> ---
> - 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
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
> ---
> - 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
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
> > ---
> > - 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
Eduardo Habkost 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
>> ---
>> - 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
On Fri, Mar 23, 2018 at 03:45:49PM +0100, Vitaly Kuznetsov wrote:
> Eduardo Habkost 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
> >> ---
> >> - 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
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 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
> > >> ---
> > >> - 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
