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 05:19:24PM -0300, Eduardo Habkost wrote:
> On Wed, Mar 21, 2018 at 07:57:29PM +0300, Roman Kagan wrote:
> > 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.
> 
> If we keep machine-type compatibility, people will need to do
> that only if they change the machine-type (or use the "pc" or
> "q35" aliases).  If they copy the old configuration, it will keep
> working.

The problem is that the feature is not fixed by the machine-type, due to
the forgotten property: it only depends on the KVM version.  So, once
(if) we add the property and make the feature deterministic, we'll lose
compatibility one way or another.

Or are you suggesting that for pre-2.12 machine types we leave the
property at "decided by your KVM" state?

> 
> machine-type compatibility also makes the following case a bit
> safer:
> 
> > 
> > 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.
> 
> If we keep machine-type compatibility, the CPUID bit won't
> disappear for the guest while the MSRs keep working.
> 
> 
> Whichever solution we choose, we can still have guests crashing
> if migrating a pc-2.11 machine from a 4.14+ host kernel to a host
> with an older kernel.  But I don't think there's a way out of
> this, except requiring an explicit "hv-frequencies" CPU option on
> newer machine-types.

What's wrong with requiring it, as we do for all other hv_* properties?

Roman.