| Message ID | 20190504223142.26668-2-nadav.amit@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: PMU: rdpmc fixes | expand |
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); > } >
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); }
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(-)