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

From: Aaron Lewis <aaronlewis@google.com>
To: Like Xu <like.xu.linux@gmail.com>
Cc: pbonzini@redhat.com, jmattson@google.com, seanjc@google.com,
	kvm list <kvm@vger.kernel.org>
Subject: Re: [PATCH v8 1/7] kvm: x86/pmu: Correct the mask used in a pmu event filter lookup
Date: Wed, 28 Dec 2022 20:00:39 +0000	[thread overview]
Message-ID: <CAAAPnDE2HjrDbgzJgj7G6=5NQ11BJ=x4wmEStJkoPPCyWQmK_w@mail.gmail.com> (raw)
In-Reply-To: <37064a64-47cb-aaad-4b8e-6ce2bdf68e56@gmail.com>

> > diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> > index 85ff3c0588ba..5b070c563a97 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_EVENT;
>
> Isn't it weird when the new thing added here is
> not of the same type as the existing members ?
>
> Doesn't "pmu->raw_event_mask" help here ?
>

In earlier revisions I had this as a callback, but it was a little
awkward being that I really just wanted it to be a const that differed
depending on platform.  Making it a const fit more naturally when
using it than the callback approach, so despite it being different
from the others here I think it works better overall.

> >   };
> >
> >   void kvm_pmu_ops_update(const struct kvm_pmu_ops *pmu_ops);
> > diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> > index 0e313fbae055..d3ae261d56a6 100644
> > --- a/arch/x86/kvm/svm/pmu.c
> > +++ b/arch/x86/kvm/svm/pmu.c
> > @@ -229,4 +229,5 @@ struct kvm_pmu_ops amd_pmu_ops __initdata = {
> >       .refresh = amd_pmu_refresh,
> >       .init = amd_pmu_init,
> >       .reset = amd_pmu_reset,
> > +     .EVENTSEL_EVENT = AMD64_EVENTSEL_EVENT,
> >   };
> > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> > index e5cec07ca8d9..edf23115f2ef 100644
> > --- a/arch/x86/kvm/vmx/pmu_intel.c
> > +++ b/arch/x86/kvm/vmx/pmu_intel.c
> > @@ -810,4 +810,5 @@ struct kvm_pmu_ops intel_pmu_ops __initdata = {
> >       .reset = intel_pmu_reset,
> >       .deliver_pmi = intel_pmu_deliver_pmi,
> >       .cleanup = intel_pmu_cleanup,
> > +     .EVENTSEL_EVENT = ARCH_PERFMON_EVENTSEL_EVENT,
> >   };