Re: [PATCH 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24]

[Sean Christopherson <seanjc@google.com>]

On Fri, Sep 22, 2023, Jim Mattson wrote:
> On Fri, Sep 22, 2023 at 11:15 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Sep 22, 2023, Jim Mattson wrote:
> > > On Fri, Sep 22, 2023 at 10:21 AM Sean Christopherson <seanjc@google.com> wrote:
> > > > IMO, we should delete the offending kernel code.  I don't see how it provides any
> > > > value these days.
> > >
> > > Sure, but that doesn't help legacy guests.
> >
> > Heh, IMO they don't need help, their owners just need to be placated ;-)
> >
> > > > And *if* we want to change something in KVM so that we stop getting coustomer
> > > > complaints about a useless bit, just let userspace stuff the bit.
> > >
> > > We want to make customers happy. That should not even be a question.
> >
> > Can we really not tell them "this is a benign guest bug, ignore it"?
> 
> What is the mechanism for doing that?

Not my problem?  :-D

> > Because that's architectural behavior, not dependent on FMS, and KVM needs to
> > write EFLAGS to have any hope of being useful, i.e. giving ownership of EFLAGS
> > to userspace is not a realistic option.
> 
> Remind me what "MSR" stands for. :)

Heh, that acronym has long, long since lost all meaning.

Joking aside, I think KVM needs to set a very, very high bare for emulating any
part of any MSR that is truly model specific.  IMO, it's far too likely that KVM
will be the one left holding the bag in such situations.

> > As proposed, if userspace sets CPUID to a magic FMS, and then changes the FMS to
> > something else, kvm_vcpu_after_set_cpuid() will not clear the bit and KVM will
> > end up wrongly enumerating the bit.  I doubt userspace would ever do that, but
> > it's at least possible.
> >
> > That could be fixed by actively clearing vcpu->arch.msr_hwcr for other FMS values,
> > but then KVM would have to be 100% precise on the FMS matching, which would be a
> > maintenance nightmare.
> 
> What if I did something crude like we do for MSR_IA32_MISC_ENABLE, and
> just set the bit at reset regardless of FMS:

I'd prefer that over playing games with FMS.  Though my first chioce would still
be to punt the decision to userspace.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cb02a7c2938b..4d7d0de42a9d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12086,6 +12086,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu,
> bool init_event)
>                 vcpu->arch.msr_misc_features_enables = 0;
>                 vcpu->arch.ia32_misc_enable_msr =
> MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL |
> 
> MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
> +               vcpu_arch.msr_hwcr = BIT_ULL(24);
> 
>                 __kvm_set_xcr(vcpu, 0, XFEATURE_MASK_FP);
>                 __kvm_set_msr(vcpu, MSR_IA32_XSS, 0, true);
> 
> > In other words, userspace owns the vCPU model, and for good reasons.  KVM needs
> > to allow userspace to define a sane model, but with *very* few exceptions, KVM
> > should not try to "help" userspace by stuffing bits.
> 
> Okay. What about the IA32_MISC_ENABLE bits above?

One of the exceptions where I don't see a better option, and hopefully something
that Intel won't repeat in the future.  Though I'm not exactly brimming with
confidence that Intel won't retroactively add more "gotcha! unsupported!" bits
in the future when they realize they forgot add a useful CPUID feature bit.

> > Pretty much everytime KVM tries to help, it causes problems.  E.g. initializing
> > perf_capabilities to kvm_caps.supported_perf_cap seems like a good thing, except
> > it presents a bogus model if userspace decides to not enumerate a vPMU to the
> > guest (Aaron was allegedly going to send a patch for this...).
> 
> KVM is nothing if not inconsistent.

Yeah, the existing inconsistencies are painful, but that's not a good reason to
continue what I see as bad behavior.