[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/2] i386/kvm: expose Hyper-V frequency MSRs
From: |
Roman Kagan |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/2] i386/kvm: expose Hyper-V frequency MSRs with reenlightenment |
Date: |
Mon, 26 Mar 2018 17:25:47 +0300 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
On Fri, Mar 23, 2018 at 03:45:49PM +0100, Vitaly Kuznetsov wrote:
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> >> ---
> >> - 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.