[kvm-unit-tests PATCH 0/2] x86: PMU: rdpmc fixes

[kvm-unit-tests PATCH 0/2] x86: PMU: rdpmc fixes

[Nadav Amit]

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(-)

-- 
2.17.1

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

[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).

Fix the tests accordingly.

Signed-off-by: Nadav Amit <nadav.amit@gmail.com>

---

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(-)

diff --git a/x86/pmu.c b/x86/pmu.c
index 6658fe9..afb387b 100644
--- a/x86/pmu.c
+++ b/x86/pmu.c
@@ -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);
 	}
-- 
2.17.1

[kvm-unit-tests PATCH 2/2] x86: PMU: Make fast counters test optional

[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 <nadav.amit@gmail.com>
---
 x86/pmu.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/x86/pmu.c b/x86/pmu.c
index afb387b..cb8c9e3 100644
--- a/x86/pmu.c
+++ b/x86/pmu.c
@@ -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();
-- 
2.17.1

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

[Paolo Bonzini]

On 05/05/19 00:31, Nadav Amit wrote:
> 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 <nadav.amit@gmail.com>
> 
> ---
> 
> 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

> ---
>  x86/pmu.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/x86/pmu.c b/x86/pmu.c
> index 6658fe9..afb387b 100644
> --- a/x86/pmu.c
> +++ b/x86/pmu.c
> @@ -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);
>  	}
>