On Wed, Sep 18, 2024, Colton Lewis wrote:
Run a basic test to ensure we can write an arbitrary value to the core counters and read it back.
Signed-off-by: Colton Lewis coltonlewis@google.com
.../selftests/kvm/x86_64/pmu_counters_test.c | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+)
diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c index 5b240585edc5..79ca7d608e00 100644 --- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c +++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c @@ -641,11 +641,65 @@ static uint8_t nr_core_counters(void) return AMD_NR_CORE_EXT_COUNTERS; return AMD_NR_CORE_COUNTERS; +}
+static uint8_t guest_nr_core_counters(void) +{
- uint8_t nr_counters = this_cpu_property(X86_PROPERTY_NUM_PERF_CTR_CORE);
- bool core_ext = this_cpu_has(X86_FEATURE_PERF_CTR_EXT_CORE);
For both this and nr_core_counters(), there's no need to read PERF_CTR_EXT_CORE if nr_counters is non-zero, and then no need to capture it in a local variable.
- if (nr_counters != 0)
return nr_counters;- if (core_ext)
return AMD_NR_CORE_EXT_COUNTERS;- return AMD_NR_CORE_COUNTERS;
This is *painfully* similar to nr_core_counters(). It actually took me almost a minute of staring to see the difference. One option would be to add a helper to dedup the if-statements, but while somewhat gross, I actually think a macro is the way to go.
#define nr_core_counters(scope) \ ({ \ uint8_t nr_counters = scope##_cpu_property(X86_PROPERTY_NR_PERFCTR_CORE); \ \ if (!nr_counters) { \ if (scope##_cpu_has(X86_FEATURE_PERFCTR_CORE)) \ nr_counters = AMD_NR_CORE_EXT_COUNTERS; \ else \ nr_counters = AMD_NR_CORE_COUNTERS; \ } \ nr_counters; \ })
static uint8_t kvm_nr_core_counters(void) { return nr_core_counters(kvm); }
static uint8_t guest_nr_core_counters(void) { return nr_core_counters(this);
}
Unnecessary newline.
+} +static void guest_test_rdwr_core_counters(void) +{
- bool core_ext = this_cpu_has(X86_FEATURE_PERF_CTR_EXT_CORE);
- uint8_t nr_counters = guest_nr_core_counters();
- uint8_t i;
- uint32_t esel_msr_base = core_ext ? MSR_F15H_PERF_CTL : MSR_K7_EVNTSEL0;
Please don't concoct new abbreviations. "esel" isn't used anywhere in KVM, and AFAICT it's not used in perf either.
I would also prefer to have consistent naming between the Intel and AMD tests (the Intel test uses base_<name>_msr).
base_eventsel_msr is all of four characters more.
- uint32_t cnt_msr_base = core_ext ? MSR_F15H_PERF_CTR : MSR_K7_PERFCTR0;
For better or worse, the Intel version uses "base_pmc_msr". I see no reason to diverage from that.
- uint32_t msr_step = core_ext ? 2 : 1;
- for (i = 0; i < AMD_NR_CORE_EXT_COUNTERS; i++) {
uint64_t test_val = 0xffff;uint32_t esel_msr = esel_msr_base + msr_step * i;uint32_t cnt_msr = cnt_msr_base + msr_step * i;
And then uint32_t eventsel_msr = ...; uint32_t pmc_msr = ...;
bool expect_gp = !(i < nr_counters);
Uh, isn't that just a weird way of writing:
bool expect_gp = i >= nr_counters;
uint8_t vector;uint64_t val;/* Test event selection register. */
This is pretty obvious if the MSR is named eventsel_msr.
vector = wrmsr_safe(esel_msr, test_val);GUEST_ASSERT_PMC_MSR_ACCESS(WRMSR, esel_msr, expect_gp, vector);vector = rdmsr_safe(esel_msr, &val);GUEST_ASSERT_PMC_MSR_ACCESS(RDMSR, esel_msr, expect_gp, vector);if (!expect_gp)GUEST_ASSERT_PMC_VALUE(RDMSR, esel_msr, val, test_val);/* Test counter register. */
Same thing here. If there is novel information/behavior, then by all means add a comment.
vector = wrmsr_safe(cnt_msr, test_val);GUEST_ASSERT_PMC_MSR_ACCESS(WRMSR, cnt_msr, expect_gp, vector);vector = rdmsr_safe(cnt_msr, &val);GUEST_ASSERT_PMC_MSR_ACCESS(RDMSR, cnt_msr, expect_gp, vector);if (!expect_gp)GUEST_ASSERT_PMC_VALUE(RDMSR, cnt_msr, val, test_val);- }
} static void guest_test_core_counters(void) {
- guest_test_rdwr_core_counters(); GUEST_DONE();
} -- 2.46.0.662.g92d0881bb0-goog