Re: [Qemu-devel] [PATCH v3 2/2] i386/kvm: lower requirements for Hyper-V frequency MSRs exposure

[Roman Kagan]

On Wed, Mar 21, 2018 at 02:18:54PM +0100, Vitaly Kuznetsov wrote:
> Roman Kagan <address@hidden> writes:
> 
> > On Tue, Mar 20, 2018 at 06:35:00PM +0100, Vitaly Kuznetsov wrote:
> >> Requiring tsc_is_stable_and_known() is too restrictive: even without INVTCS
> >> nested Hyper-V-on-KVM enables TSC pages for its guests e.g. when
> >> Reenlightenment MSRs are present. Presence of frequency MSRs doesn't mean
> >> these frequencies are stable, it just means they're available for reading.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <address@hidden>
> >> ---
> >>  target/i386/kvm.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> >> index 7d9f9ca0b1..74fc3d3b2c 100644
> >> --- a/target/i386/kvm.c
> >> +++ b/target/i386/kvm.c
> >> @@ -651,7 +651,7 @@ 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) {
> >>              env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
> >>              env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
> >>          }
> >
> > I suggest that we add a corresponding cpu property here, too.  The guest
> > may legitimately rely on these msrs when it sees the support in CPUID,
> > and migrating from a kernel with the feature supported (4.14+) to an
> > older one will make it crash.
> >
> 
> This can be arranged, but what happens to people who use these features
> today? Assuming they also passed 'invtsc' they have stable TSC page
> clocksource already (when Hyper-V role is enabled) but when we start
> requesting a new 'hv_frequency' cpu property they'll suddenly lose what
> they have...

I see two cases here:

1) people start a new VM, and discover that their old configuration is
   not enough for this feature to work.

   They need to reconfigure and restart the VM.  This costs them some
   time investigating and restarting, but not data.

2) people migrate from a QEMU without ->hv_frequency, to a new one with
   ->hv_frequency=off (assuming on both ends KVM supports the frequency
   MSRs).

   With the current implementation in KVM, this will only result in the
   feature bits disappearing from the respective CPUID leaf, but the
   MSRs themselves will continue working as they used to.  So the guest
   either won't notice or will check the CPUID and adjust.


Am I missing anything?

Roman.