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

[Sean Christopherson <seanjc@google.com>]

On Fri, Sep 29, 2023, Jim Mattson wrote:
> On Thu, Sep 28, 2023 at 5:56 PM Sean Christopherson <seanjc@google.com> wrote:
> > ---
> >  arch/x86/kvm/x86.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 791a644dd481..4dd64d359142 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3700,11 +3700,19 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >                 data &= ~(u64)0x100;    /* ignore ignne emulation enable */
> >                 data &= ~(u64)0x8;      /* ignore TLB cache disable */
> >
> > -               /* Handle McStatusWrEn */
> > -               if (data & ~BIT_ULL(18)) {
> > +               /*
> > +                * Allow McStatusWrEn and TscFreqSel, some guests whine if they
> > +                * aren't set.
> > +                */
> 
> The whining is only about TscFreqSel. KVM actually supports the
> functionality of McStatusWrEn (i.e. allows the guest to write to the
> MCi_STATUS MSRs).

Ugh, I missed the usage can_set_mci_status().  Stupid open coded bit numbers.

> How about...
> 
> /*
> * Allow McStatusWrEn and TscFreqSel. (Linux guests from v3.2 through
> * at least v6.6 whine if TscFreqSel is clear, depending on F/M/S.
> */
> 
> > +               if (data & ~(BIT_ULL(18) | BIT_ULL(24))) {
> >                         kvm_pr_unimpl_wrmsr(vcpu, msr, data);
> >                         return 1;
> >                 }
> > +
> > +               /* TscFreqSel is architecturally read-only, writes are ignored */
> 
> This isn't true. TscFreqSel is not architectural at all.

Doh, right, the whole source of this mess is the uarch specific nature of the MSR.

> On Family
> 10h, per https://www.amd.com/content/dam/amd/en/documents/archived-tech-docs/programmer-references/31116.pdf,
> it was R/W and powered on as 0. In Family 15h, one of the "changes
> relative to Family 10H Revision D processors," per
> https://www.amd.com/content/dam/amd/en/documents/archived-tech-docs/programmer-references/42301_15h_Mod_00h-0Fh_BKDG.pdf,
> was:
> 
> • MSRC001_0015 [Hardware Configuration (HWCR)]:
>   • Dropped TscFreqSel; TSC can no longer be selected to run at NB P0-state.
> 
> Despite the "Dropped" above, that same document later describes
> HWCR[bit 24] as follows:
> 
> TscFreqSel: TSC frequency select. Read-only. Reset: 1. 1=The TSC
> increments at the P0 frequency
> 
> So, perhaps this block of code can just be dropped? Who really cares
> if the guest changes the value (unless it goes on to kexec a new
> kernel, which will whine about the bit being clear)?

Works for me.  If userspace wants to precisely emulate model specific behavior,
then userspace can darn well intercept writes to the MSR.