Re: [PATCH v5 1/7] kvm: x86/pmu: Correct the mask used in a pmu event filter lookup

[Sean Christopherson <seanjc@google.com>]

On Sat, Oct 15, 2022, Aaron Lewis wrote:
> > And the total patch is:
> >
> > ---
> >  arch/x86/kvm/pmu.c           | 2 +-
> >  arch/x86/kvm/pmu.h           | 2 ++
> >  arch/x86/kvm/svm/pmu.c       | 2 ++
> >  arch/x86/kvm/vmx/pmu_intel.c | 2 ++
> >  4 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > index d9b9a0f0db17..d0e2c7eda65b 100644
> > --- a/arch/x86/kvm/pmu.c
> > +++ b/arch/x86/kvm/pmu.c
> > @@ -273,7 +273,7 @@ static bool check_pmu_event_filter(struct kvm_pmc *pmc)
> >                 goto out;
> >
> >         if (pmc_is_gp(pmc)) {
> > -               key = pmc->eventsel & AMD64_RAW_EVENT_MASK_NB;
> > +               key = pmc->eventsel & kvm_pmu_ops.EVENTSEL_MASK;
> >                 if (bsearch(&key, filter->events, filter->nevents,
> >                             sizeof(__u64), cmp_u64))
> >                         allow_event = filter->action == KVM_PMU_EVENT_ALLOW;
> > diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> > index 5cc5721f260b..45a7dd24125d 100644
> > --- a/arch/x86/kvm/pmu.h
> > +++ b/arch/x86/kvm/pmu.h
> > @@ -40,6 +40,8 @@ struct kvm_pmu_ops {
> >         void (*reset)(struct kvm_vcpu *vcpu);
> >         void (*deliver_pmi)(struct kvm_vcpu *vcpu);
> >         void (*cleanup)(struct kvm_vcpu *vcpu);
> > +
> > +       const u64 EVENTSEL_MASK;
> 
> Agreed, a constant is better.  Had I realized I could do that, that
> would have been my first choice.
> 
> What about calling it EVENTSEL_RAW_MASK to make it more descriptive
> though?  It's a little too generic given there is EVENTSEL_UMASK and
> EVENTSEL_CMASK, also there is precedent for using the term 'raw event'
> for (eventsel+umask), i.e.
> https://man7.org/linux/man-pages/man1/perf-record.1.html.

Hmm.  I'd prefer to avoid "raw" because that implies there's a non-raw version
that can be translated into the "raw" version.  This is kinda the opposite, where
the above field is the composite type and the invidiual fields are the "raw"
components.

Refresh me, as I've gotten twisted about: this mask needs to be EVENTSEL_EVENT_MASK
+ EVENTSEL_UMASK, correct?  Or phrased differently, it'll hold more than just
EVENTSEL_EVENT_MASK?

What about something completely different, e.g. FILTER_MASK?  It'll require a
comment to document, but that seems inevitable, and FILTER_MASK should be really
hard to confuse with the myriad EVENTSEL_*MASK fields.