2022-05-03 05:01:53

by Kyle Huey

[permalink] [raw]
Subject: [PATCH] KVM: x86/svm: Account for family 17h event renumberings in amd_pmc_perf_hw_id

From: Kyle Huey <[email protected]>

Zen renumbered some of the performance counters that correspond to the
well known events in perf_hw_id. This code in KVM was never updated for
that, so guest that attempt to use counters on Zen that correspond to the
pre-Zen perf_hw_id values will silently receive the wrong values.

This has been observed in the wild with rr[0] when running in Zen 3
guests. rr uses the retired conditional branch counter 00d1 which is
incorrectly recognized by KVM as PERF_COUNT_HW_STALLED_CYCLES_BACKEND.

[0] https://rr-project.org/

Signed-off-by: Kyle Huey <[email protected]>
---
arch/x86/kvm/svm/pmu.c | 28 +++++++++++++++++++++++++---
1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index b14860863c39..43d24746c0a7 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -45,6 +45,22 @@ static struct kvm_event_hw_type_mapping amd_event_mapping[] = {
[7] = { 0xd1, 0x00, PERF_COUNT_HW_STALLED_CYCLES_BACKEND },
};

+/* duplicated from amd_f17h_perfmon_event_map. */
+static struct kvm_event_hw_type_mapping amd_f17h_event_mapping[] = {
+ [0] = { 0x76, 0x00, PERF_COUNT_HW_CPU_CYCLES },
+ [1] = { 0xc0, 0x00, PERF_COUNT_HW_INSTRUCTIONS },
+ [2] = { 0x60, 0xff, PERF_COUNT_HW_CACHE_REFERENCES },
+ [3] = { 0x64, 0x09, PERF_COUNT_HW_CACHE_MISSES },
+ [4] = { 0xc2, 0x00, PERF_COUNT_HW_BRANCH_INSTRUCTIONS },
+ [5] = { 0xc3, 0x00, PERF_COUNT_HW_BRANCH_MISSES },
+ [6] = { 0x87, 0x02, PERF_COUNT_HW_STALLED_CYCLES_FRONTEND },
+ [7] = { 0x87, 0x01, PERF_COUNT_HW_STALLED_CYCLES_BACKEND },
+};
+
+/* amd_pmc_perf_hw_id depends on these being the same size */
+static_assert(ARRAY_SIZE(amd_event_mapping) ==
+ ARRAY_SIZE(amd_f17h_event_mapping));
+
static unsigned int get_msr_base(struct kvm_pmu *pmu, enum pmu_type type)
{
struct kvm_vcpu *vcpu = pmu_to_vcpu(pmu);
@@ -140,6 +156,7 @@ static inline struct kvm_pmc *get_gp_pmc_amd(struct kvm_pmu *pmu, u32 msr,

static unsigned int amd_pmc_perf_hw_id(struct kvm_pmc *pmc)
{
+ struct kvm_event_hw_type_mapping *event_mapping;
u8 event_select = pmc->eventsel & ARCH_PERFMON_EVENTSEL_EVENT;
u8 unit_mask = (pmc->eventsel & ARCH_PERFMON_EVENTSEL_UMASK) >> 8;
int i;
@@ -148,15 +165,20 @@ static unsigned int amd_pmc_perf_hw_id(struct kvm_pmc *pmc)
if (WARN_ON(pmc_is_fixed(pmc)))
return PERF_COUNT_HW_MAX;

+ if (boot_cpu_data.x86 >= 0x17)
+ event_mapping = amd_f17h_event_mapping;
+ else
+ event_mapping = amd_event_mapping;
+
for (i = 0; i < ARRAY_SIZE(amd_event_mapping); i++)
- if (amd_event_mapping[i].eventsel == event_select
- && amd_event_mapping[i].unit_mask == unit_mask)
+ if (event_mapping[i].eventsel == event_select
+ && event_mapping[i].unit_mask == unit_mask)
break;

if (i == ARRAY_SIZE(amd_event_mapping))
return PERF_COUNT_HW_MAX;

- return amd_event_mapping[i].event_type;
+ return event_mapping[i].event_type;
}

/* check if a PMC is enabled by comparing it against global_ctrl bits. Because
--
2.36.0


2022-05-03 11:37:15

by Jim Mattson

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86/svm: Account for family 17h event renumberings in amd_pmc_perf_hw_id

On Tue, May 3, 2022 at 2:46 AM Paolo Bonzini <[email protected]> wrote:
>
> Queued, thanks.

Isn't it better to just drop this entirely, as in
https://lore.kernel.org/kvm/[email protected]/?

2022-05-04 02:29:31

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86/svm: Account for family 17h event renumberings in amd_pmc_perf_hw_id

On 5/3/22 12:00, Jim Mattson wrote:
>> Queued, thanks.
> Isn't it better to just drop this entirely, as in
> https://lore.kernel.org/kvm/[email protected]/?

I plan to do that on top, this patch is good enough for stable.

Paolo

2022-05-04 12:44:34

by Kyle Huey

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86/svm: Account for family 17h event renumberings in amd_pmc_perf_hw_id

On Tue, May 3, 2022 at 4:50 AM Paolo Bonzini <[email protected]> wrote:
>
> On 5/3/22 12:00, Jim Mattson wrote:
> >> Queued, thanks.
> > Isn't it better to just drop this entirely, as in
> > https://lore.kernel.org/kvm/[email protected]/?
>
> I plan to do that on top, this patch is good enough for stable.
>
> Paolo

Yeah, rr will want this on stable, though this patch won't apply cleanly.

- Kyle