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

[Roman Kagan]

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 <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 &&
> > +            (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.