Re: [PATCH v9 10/28] KVM: x86/pmu: Explicitly check for RDPMC of unsupported Intel PMC types

[Sean Christopherson]

On Mon, Dec 11, 2023, Jim Mattson wrote:
> On Mon, Dec 11, 2023 at 3:43 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > > > @@ -82,9 +85,13 @@ static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
> > > > >       /*
> > > > >        * Fixed PMCs are supported on all architectural PMUs.  Note, KVM only
> > > > >        * emulates fixed PMCs for PMU v2+, but the flag itself is still valid,
> > > > > -      * i.e. let RDPMC fail due to accessing a non-existent counter.
> > > > > +      * i.e. let RDPMC fail due to accessing a non-existent counter.  Reject
> > > > > +      * attempts to read all other types, which are unknown/unsupported.
> > > > >        */
> > > > > -     idx &= ~INTEL_RDPMC_FIXED;
> > > > > +     if (idx & INTEL_RDPMC_TYPE_MASK & ~INTEL_RDPMC_FIXED)
> > >
> > > You know how I hate to be pedantic (ROFL), but the SDM only says:
> > >
> > > If the processor does support architectural performance monitoring
> > > (CPUID.0AH:EAX[7:0] ≠ 0), ECX[31:16] specifies type of PMC while
> > > ECX[15:0] specifies the index of the PMC to be read within that type.
> > >
> > > It does not say that the types are bitwise-exclusive.
> > >
> > > Yes, the types defined thus far are bitwise-exclusive, but who knows
> > > what tomorrow may bring?
> >
> > The goal isn't to make the types exclusive, the goal is to reject types that
> > aren't supported by KVM.  The above accomplishes that, no?  I don't see how KVM
> > could get a false negative or false positive, the above allows exactly FIXED and
> > "none" types.  Or are you objecting to the comment?
> 
> You're right. The code is fine. My brain is not.
> 
> But what's wrong with something like:
> 
> type = idx & INTEL_RDPMC_TYPE_MASK;
> if (type != INTEL_RDPMC_GP && type != INTEL_RDPMC_FIXED) ...
> 
> This makes it more clear what kvm accepts and what it doesn't accept,
> regardless of the actual values of the macros.

Because when I read the SDM, my reading was heavily colored by KVM's existing
implementation.  And the SDM using 4000H and 2000H for the non-zero types doesn't
help (those scream "flags" to me).  But rereading things, the SDM clearly states
they are explicit, distinct types.  I'll massage this to have KVM treat them as
such.