Re: [PATCH v2 4/6] KVM: x86: selftests: Test read/write core counters

[Colton Lewis]

Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >   .../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