| Message ID | 20220920174603.302510-2-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce and test masked events | expand |
On Tue, Sep 20, 2022, Aaron Lewis wrote: > When checking if a pmu event the guest is attempting to program should > be filtered, only consider the event select + unit mask in that > decision. Use an architecture specific mask to mask out all other bits, > including bits 35:32 on Intel. Those bits are not part of the event > select and should not be considered in that decision. > > Fixes: 66bb8a065f5a ("KVM: x86: PMU Event Filter") > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > Reviewed-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/include/asm/kvm-x86-pmu-ops.h | 1 + > arch/x86/kvm/pmu.c | 15 ++++++++++++++- > arch/x86/kvm/pmu.h | 1 + > arch/x86/kvm/svm/pmu.c | 6 ++++++ > arch/x86/kvm/vmx/pmu_intel.c | 6 ++++++ > 5 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/kvm-x86-pmu-ops.h b/arch/x86/include/asm/kvm-x86-pmu-ops.h > index c17e3e96fc1d..e0280cc3e6e4 100644 > --- a/arch/x86/include/asm/kvm-x86-pmu-ops.h > +++ b/arch/x86/include/asm/kvm-x86-pmu-ops.h > @@ -24,6 +24,7 @@ KVM_X86_PMU_OP(set_msr) > KVM_X86_PMU_OP(refresh) > KVM_X86_PMU_OP(init) > KVM_X86_PMU_OP(reset) > +KVM_X86_PMU_OP(get_eventsel_event_mask) > KVM_X86_PMU_OP_OPTIONAL(deliver_pmi) > KVM_X86_PMU_OP_OPTIONAL(cleanup) > > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c > index 02f9e4f245bd..98f383789579 100644 > --- a/arch/x86/kvm/pmu.c > +++ b/arch/x86/kvm/pmu.c > @@ -247,6 +247,19 @@ static int cmp_u64(const void *pa, const void *pb) > return (a > b) - (a < b); > } > > +static inline u64 get_event_select(u64 eventsel) > +{ > + return eventsel & static_call(kvm_x86_pmu_get_eventsel_event_mask)(); There's no need to use a callback, both values are constant. And with a constant, this line becomes much shorter and this helper goes away. More below. > +} > + > +static inline u64 get_raw_event(u64 eventsel) As a general rule, don't tag local static functions as "inline". Unless something _needs_ to be inline, the compiler is almost always going to be smarter, and if something absolutely must be inlined, then __always_inline is requires as "inline" is purely a hint. > +{ > + u64 event_select = get_event_select(eventsel); > + u64 unit_mask = eventsel & ARCH_PERFMON_EVENTSEL_UMASK; And looking forward, I think it makes sense for kvm_pmu_ops to hold the entire mask. This code from patch 3 is unnecessarily complex, and bleeds vendor details where they don't belong. I'll follow up more in patches 3 and 4. static inline u16 get_event_select(u64 eventsel) { u64 e = eventsel & static_call(kvm_x86_pmu_get_eventsel_event_mask)(); return (e & ARCH_PERFMON_EVENTSEL_EVENT) | ((e >> 24) & 0xF00ULL); } > + > + return event_select | unit_mask; > +} > + > static bool check_pmu_event_filter(struct kvm_pmc *pmc) > { > struct kvm_pmu_event_filter *filter; > @@ -263,7 +276,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 = get_raw_event(pmc->eventsel); With the above suggestion, this is simply: key = pmc->eventsel & kvm_pmu_.ops.EVENTSEL_MASK; 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; }; 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 b68956299fa8..6ef7d1fcdbc2 100644 --- a/arch/x86/kvm/svm/pmu.c +++ b/arch/x86/kvm/svm/pmu.c @@ -228,4 +228,6 @@ struct kvm_pmu_ops amd_pmu_ops __initdata = { .refresh = amd_pmu_refresh, .init = amd_pmu_init, .reset = amd_pmu_reset, + .EVENTSEL_MASK = AMD64_EVENTSEL_EVENT | + ARCH_PERFMON_EVENTSEL_UMASK, }; diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c index 25b70a85bef5..0a1c95b64ef1 100644 --- a/arch/x86/kvm/vmx/pmu_intel.c +++ b/arch/x86/kvm/vmx/pmu_intel.c @@ -811,4 +811,6 @@ struct kvm_pmu_ops intel_pmu_ops __initdata = { .reset = intel_pmu_reset, .deliver_pmi = intel_pmu_deliver_pmi, .cleanup = intel_pmu_cleanup, + .EVENTSEL_MASK = ARCH_PERFMON_EVENTSEL_EVENT | + ARCH_PERFMON_EVENTSEL_UMASK, }; base-commit: e18d6152ff0f41b7f01f9817372022df04e0d354 --
> 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. > }; > > 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 b68956299fa8..6ef7d1fcdbc2 100644 > --- a/arch/x86/kvm/svm/pmu.c > +++ b/arch/x86/kvm/svm/pmu.c > @@ -228,4 +228,6 @@ struct kvm_pmu_ops amd_pmu_ops __initdata = { > .refresh = amd_pmu_refresh, > .init = amd_pmu_init, > .reset = amd_pmu_reset, > + .EVENTSEL_MASK = AMD64_EVENTSEL_EVENT | > + ARCH_PERFMON_EVENTSEL_UMASK, > }; > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c > index 25b70a85bef5..0a1c95b64ef1 100644 > --- a/arch/x86/kvm/vmx/pmu_intel.c > +++ b/arch/x86/kvm/vmx/pmu_intel.c > @@ -811,4 +811,6 @@ struct kvm_pmu_ops intel_pmu_ops __initdata = { > .reset = intel_pmu_reset, > .deliver_pmi = intel_pmu_deliver_pmi, > .cleanup = intel_pmu_cleanup, > + .EVENTSEL_MASK = ARCH_PERFMON_EVENTSEL_EVENT | > + ARCH_PERFMON_EVENTSEL_UMASK, > }; > > base-commit: e18d6152ff0f41b7f01f9817372022df04e0d354 > -- >
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.
On Mon, Oct 17, 2022 at 6:20 PM Sean Christopherson <seanjc@google.com> 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.
diff --git a/arch/x86/include/asm/kvm-x86-pmu-ops.h b/arch/x86/include/asm/kvm-x86-pmu-ops.h index c17e3e96fc1d..e0280cc3e6e4 100644 --- a/arch/x86/include/asm/kvm-x86-pmu-ops.h +++ b/arch/x86/include/asm/kvm-x86-pmu-ops.h @@ -24,6 +24,7 @@ KVM_X86_PMU_OP(set_msr) KVM_X86_PMU_OP(refresh) KVM_X86_PMU_OP(init) KVM_X86_PMU_OP(reset) +KVM_X86_PMU_OP(get_eventsel_event_mask) KVM_X86_PMU_OP_OPTIONAL(deliver_pmi) KVM_X86_PMU_OP_OPTIONAL(cleanup) diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c index 02f9e4f245bd..98f383789579 100644 --- a/arch/x86/kvm/pmu.c +++ b/arch/x86/kvm/pmu.c @@ -247,6 +247,19 @@ static int cmp_u64(const void *pa, const void *pb) return (a > b) - (a < b); } +static inline u64 get_event_select(u64 eventsel) +{ + return eventsel & static_call(kvm_x86_pmu_get_eventsel_event_mask)(); +} + +static inline u64 get_raw_event(u64 eventsel) +{ + u64 event_select = get_event_select(eventsel); + u64 unit_mask = eventsel & ARCH_PERFMON_EVENTSEL_UMASK; + + return event_select | unit_mask; +} + static bool check_pmu_event_filter(struct kvm_pmc *pmc) { struct kvm_pmu_event_filter *filter; @@ -263,7 +276,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 = get_raw_event(pmc->eventsel); 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..4e22f9f55400 100644 --- a/arch/x86/kvm/pmu.h +++ b/arch/x86/kvm/pmu.h @@ -40,6 +40,7 @@ struct kvm_pmu_ops { void (*reset)(struct kvm_vcpu *vcpu); void (*deliver_pmi)(struct kvm_vcpu *vcpu); void (*cleanup)(struct kvm_vcpu *vcpu); + u64 (*get_eventsel_event_mask)(void); }; 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 f24613a108c5..0b35eb04aa60 100644 --- a/arch/x86/kvm/svm/pmu.c +++ b/arch/x86/kvm/svm/pmu.c @@ -294,6 +294,11 @@ static void amd_pmu_reset(struct kvm_vcpu *vcpu) } } +static u64 amd_pmu_get_eventsel_event_mask(void) +{ + return AMD64_EVENTSEL_EVENT; +} + struct kvm_pmu_ops amd_pmu_ops __initdata = { .hw_event_available = amd_hw_event_available, .pmc_is_enabled = amd_pmc_is_enabled, @@ -307,4 +312,5 @@ struct kvm_pmu_ops amd_pmu_ops __initdata = { .refresh = amd_pmu_refresh, .init = amd_pmu_init, .reset = amd_pmu_reset, + .get_eventsel_event_mask = amd_pmu_get_eventsel_event_mask, }; diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c index c399637a3a79..0aec7576af0c 100644 --- a/arch/x86/kvm/vmx/pmu_intel.c +++ b/arch/x86/kvm/vmx/pmu_intel.c @@ -793,6 +793,11 @@ void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu) } } +static u64 intel_pmu_get_eventsel_event_mask(void) +{ + return ARCH_PERFMON_EVENTSEL_EVENT; +} + struct kvm_pmu_ops intel_pmu_ops __initdata = { .hw_event_available = intel_hw_event_available, .pmc_is_enabled = intel_pmc_is_enabled, @@ -808,4 +813,5 @@ struct kvm_pmu_ops intel_pmu_ops __initdata = { .reset = intel_pmu_reset, .deliver_pmi = intel_pmu_deliver_pmi, .cleanup = intel_pmu_cleanup, + .get_eventsel_event_mask = intel_pmu_get_eventsel_event_mask, };
When checking if a pmu event the guest is attempting to program should be filtered, only consider the event select + unit mask in that decision. Use an architecture specific mask to mask out all other bits, including bits 35:32 on Intel. Those bits are not part of the event select and should not be considered in that decision. Fixes: 66bb8a065f5a ("KVM: x86: PMU Event Filter") Signed-off-by: Aaron Lewis <aaronlewis@google.com> Reviewed-by: Jim Mattson <jmattson@google.com> --- arch/x86/include/asm/kvm-x86-pmu-ops.h | 1 + arch/x86/kvm/pmu.c | 15 ++++++++++++++- arch/x86/kvm/pmu.h | 1 + arch/x86/kvm/svm/pmu.c | 6 ++++++ arch/x86/kvm/vmx/pmu_intel.c | 6 ++++++ 5 files changed, 28 insertions(+), 1 deletion(-)