Skip to contentSkip to conversation

x86: PMU: rdpmc fixes

PATCH Applied Nadav Amit 2 participants 4 messages May 5, 2019 – May 20, 2019 KVM

lore
Share & tools
lore

Nadav Amit

author
#0 lore raw

cover x86: PMU: rdpmc fixes

The PMU tests have a real and latent bugs. This patch-set addresses them. Once the patches are applied, kvm-unit-tests fail on KVM, so a small fix of KVM is also needed.

(I am not going to submit a patch that fixes KVM).

Nadav Amit (2):
x86: PMU: Fix PMU counters masking
x86: PMU: Make fast counters test optional

 x86/pmu.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 9 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 1/2 x86: PMU: Fix PMU counters masking

Intel SDM says that for MSR_IA32_PERFCTR0/1 "the lower-order 32 bits of each MSR may be written with any value, and the high-order 8 bits are sign-extended according to the value of bit 31." The current PMU tests ignored the fact that the high bit is sign-extended.

At the same time, the fixed counters are not limited to 32-bit, but appear to be limited to the width of the fixed counters (I could not find clear documentation).

Fix the tests accordingly.

Signed-off-by Nadav Amit <[email protected]>

---

As a result of this fix, the fixed counters test currently fails on KVM. I am unable to provide a bug-fix although the fix is simple.

---
 x86/pmu.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)
x86/pmu.c +21 -7
index 6658fe9..afb387b 100644
@@ -316,14 +316,19 @@ static void check_counter_overflow(void)
 	for (i = 0; i < num_counters + 1; i++, cnt.ctr++) {
 		uint64_t status;
 		int idx;
-		if (i == num_counters)
+
+		cnt.count = 1 - count;
+
+		if (i == num_counters) {
 			cnt.ctr = fixed_events[0].unit_sel;
+			cnt.count &= (1ul << edx.split.bit_width_fixed) - 1;
+		}
+
 		if (i % 2)
 			cnt.config |= EVNTSEL_INT;
 		else
 			cnt.config &= ~EVNTSEL_INT;
 		idx = event_to_global_idx(&cnt);
-		cnt.count = 1 - count;
 		measure(&cnt, 1);
 		report("cntr-%d", cnt.count == 1, i);
 		status = rdmsr(MSR_CORE_PERF_GLOBAL_STATUS);

@@ -357,16 +362,25 @@ static void check_rdpmc(void)
 	report_prefix_push("rdpmc");
 
 	for (i = 0; i < num_counters; i++) {
-		uint64_t x = (val & 0xffffffff) |
-			((1ull << (eax.split.bit_width - 32)) - 1) << 32;
+		uint64_t x;
+
+		/*
+		 * Only the low 32 bits are writable, and the value is
+		 * sign-extended.
+		 */
+		x = (uint64_t)(int64_t)(int32_t)val;
+
+		/* Mask according to the number of supported bits */
+		x &= (1ull << eax.split.bit_width) - 1;
+
 		wrmsr(MSR_IA32_PERFCTR0 + i, val);
 		report("cntr-%d", rdpmc(i) == x, i);
 		report("fast-%d", rdpmc(i | (1<<31)) == (u32)val, i);
 	}
 	for (i = 0; i < edx.split.num_counters_fixed; i++) {
-		uint64_t x = (val & 0xffffffff) |
-			((1ull << (edx.split.bit_width_fixed - 32)) - 1) << 32;
-		wrmsr(MSR_CORE_PERF_FIXED_CTR0 + i, val);
+		uint64_t x = val & ((1ull << edx.split.bit_width_fixed) - 1);
+
+		wrmsr(MSR_CORE_PERF_FIXED_CTR0 + i, x);
 		report("fixed cntr-%d", rdpmc(i | (1 << 30)) == x, i);
 		report("fixed fast-%d", rdpmc(i | (3<<30)) == (u32)val, i);
 	}
Signature · 1 line
2.17.1
maintainer Applied
#2 lore raw

Re: [kvm-unit-tests PATCH 1/2] x86: PMU: Fix PMU counters masking

Quoted 9 lines · Nadav Amit

Intel SDM says that for MSR_IA32_PERFCTR0/1 "the lower-order 32 bits of each MSR may be written with any value, and the high-order 8 bits are sign-extended according to the value of bit 31." The current PMU tests ignored the fact that the high bit is sign-extended.

At the same time, the fixed counters are not limited to 32-bit, but appear to be limited to the width of the fixed counters (I could not find clear documentation).

Quoted 8 lines

Fix the tests accordingly.

Signed-off-by Nadav Amit <[email protected]>

---

As a result of this fix, the fixed counters test currently fails on KVM. I am unable to provide a bug-fix although the fix is simple.

Fair enough, I'll give it a look.

Queued both, thanks.

Paolo

Quoted 62 lines
---
 x86/pmu.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)
x86/pmu.c +21 -7
index 6658fe9..afb387b 100644
@@ -316,14 +316,19 @@ static void check_counter_overflow(void)
 	for (i = 0; i < num_counters + 1; i++, cnt.ctr++) {
 		uint64_t status;
 		int idx;
-		if (i == num_counters)
+
+		cnt.count = 1 - count;
+
+		if (i == num_counters) {
 			cnt.ctr = fixed_events[0].unit_sel;
+			cnt.count &= (1ul << edx.split.bit_width_fixed) - 1;
+		}
+
 		if (i % 2)
 			cnt.config |= EVNTSEL_INT;
 		else
 			cnt.config &= ~EVNTSEL_INT;
 		idx = event_to_global_idx(&cnt);
-		cnt.count = 1 - count;
 		measure(&cnt, 1);
 		report("cntr-%d", cnt.count == 1, i);
 		status = rdmsr(MSR_CORE_PERF_GLOBAL_STATUS);

@@ -357,16 +362,25 @@ static void check_rdpmc(void)
 	report_prefix_push("rdpmc");
 
 	for (i = 0; i < num_counters; i++) {
-		uint64_t x = (val & 0xffffffff) |
-			((1ull << (eax.split.bit_width - 32)) - 1) << 32;
+		uint64_t x;
+
+		/*
+		 * Only the low 32 bits are writable, and the value is
+		 * sign-extended.
+		 */
+		x = (uint64_t)(int64_t)(int32_t)val;
+
+		/* Mask according to the number of supported bits */
+		x &= (1ull << eax.split.bit_width) - 1;
+
 		wrmsr(MSR_IA32_PERFCTR0 + i, val);
 		report("cntr-%d", rdpmc(i) == x, i);
 		report("fast-%d", rdpmc(i | (1<<31)) == (u32)val, i);
 	}
 	for (i = 0; i < edx.split.num_counters_fixed; i++) {
-		uint64_t x = (val & 0xffffffff) |
-			((1ull << (edx.split.bit_width_fixed - 32)) - 1) << 32;
-		wrmsr(MSR_CORE_PERF_FIXED_CTR0 + i, val);
+		uint64_t x = val & ((1ull << edx.split.bit_width_fixed) - 1);
+
+		wrmsr(MSR_CORE_PERF_FIXED_CTR0 + i, x);
 		report("fixed cntr-%d", rdpmc(i | (1 << 30)) == x, i);
 		report("fixed fast-%d", rdpmc(i | (3<<30)) == (u32)val, i);
 	}
author
#3 lore raw

PATCH 2/2 x86: PMU: Make fast counters test optional

In reply to Nadav Amit

The fast counters are optional. As the Intel SDM says: "ECX[31] selects "fast" read mode if supported." Skip the test is the fast counters are not supported.

Signed-off-by Nadav Amit <[email protected]>
---
 x86/pmu.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)
x86/pmu.c +33 -2
index afb387b..cb8c9e3 100644
@@ -354,15 +354,32 @@ static void check_gp_counter_cmask(void)
 	report("cmask", cnt.count < gp_events[1].min);
 }
 
+static void do_rdpmc_fast(void *ptr)
+{
+	pmu_counter_t *cnt = ptr;
+	uint32_t idx = (uint32_t)cnt->idx | (1u << 31);
+
+	if (!is_gp(cnt))
+		idx |= 1 << 30;
+
+	cnt->count = rdpmc(idx);
+}
+
+
 static void check_rdpmc(void)
 {
 	uint64_t val = 0x1f3456789ull;
+	bool exc;
 	int i;
 
 	report_prefix_push("rdpmc");
 
 	for (i = 0; i < num_counters; i++) {
 		uint64_t x;
+		pmu_counter_t cnt = {
+			.ctr = MSR_IA32_PERFCTR0 + i,
+			.idx = i
+		};
 
 		/*
 		 * Only the low 32 bits are writable, and the value is

@@ -375,14 +392,28 @@ static void check_rdpmc(void)
 
 		wrmsr(MSR_IA32_PERFCTR0 + i, val);
 		report("cntr-%d", rdpmc(i) == x, i);
-		report("fast-%d", rdpmc(i | (1<<31)) == (u32)val, i);
+
+		exc = test_for_exception(GP_VECTOR, do_rdpmc_fast, &cnt);
+		if (exc)
+			report_skip("fast-%d", i);
+		else
+			report("fast-%d", cnt.count == (u32)val, i);
 	}
 	for (i = 0; i < edx.split.num_counters_fixed; i++) {
 		uint64_t x = val & ((1ull << edx.split.bit_width_fixed) - 1);
+		pmu_counter_t cnt = {
+			.ctr = MSR_CORE_PERF_FIXED_CTR0 + i,
+			.idx = i
+		};
 
 		wrmsr(MSR_CORE_PERF_FIXED_CTR0 + i, x);
 		report("fixed cntr-%d", rdpmc(i | (1 << 30)) == x, i);
-		report("fixed fast-%d", rdpmc(i | (3<<30)) == (u32)val, i);
+
+		exc = test_for_exception(GP_VECTOR, do_rdpmc_fast, &cnt);
+		if (exc)
+			report_skip("fixed fast-%d", i);
+		else
+			report("fixed fast-%d", cnt.count == (u32)x, i);
 	}
 
 	report_prefix_pop();
Signature · 1 line
2.17.1