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);
+
+ if (nr_counters != 0)
+ return nr_counters;
+
+ if (core_ext)
+ return AMD_NR_CORE_EXT_COUNTERS;
+
+ return AMD_NR_CORE_COUNTERS;
+
+}
+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;
+ uint32_t cnt_msr_base = core_ext ? MSR_F15H_PERF_CTR : MSR_K7_PERFCTR0;
+ 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;
+ bool expect_gp = !(i < nr_counters);
+ uint8_t vector;
+ uint64_t val;
+
+ /* Test event selection register. */
+ 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. */
+ 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-googOn 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
>
Sean Christopherson <seanjc@google.com> writes:
> 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.
Sure but since I might need it and don't see why the performance cost
matters for a test that is only calling it a few times, I thought the
code looked nicer to just read it up front when I declare the variable.
I can change it.
>> +
>> + 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);
> }
Point taken. I'll go with the macro.
>> +
> Unnecessary newline.
Will delete
>> +}
>> +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'll avoid that in the future
> I would also prefer to have consistent naming between the Intel and AMD
> tests
> (the Intel test uses base_<name>_msr).
Done
> 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.
Done
>> + 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;
Yes they are logically equivalent. I thought it was clearer by
emphasizing it was the negation of "i is a valid counter" (i <
nr_counters)
But I'll change it
>> + uint8_t vector;
>> + uint64_t val;
>> +
>> + /* Test event selection register. */
> This is pretty obvious if the MSR is named eventsel_msr.
Will delete
>> + 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.
Will delete
>> + 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
© 2016 - 2026 Red Hat, Inc.