On Mon, Oct 17, 2022 at 6:20 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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?
I found it more useful to just have the event select. That allowed me
to use just it or the event select + umask as needed in patch #4.
const u64 EVENTSEL_EVENT;
>
> 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.
