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

[Eduardo Habkost <ehabkost@redhat.com>]

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