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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z37J1jJCTBZk-0cs@google.com>
Date: Wed, 8 Jan 2025 10:54:14 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Colton Lewis <coltonlewis@...gle.com>
Cc: kvm@...r.kernel.org, Mingwei Zhang <mizhang@...gle.com>, 
	Jinrong Liang <ljr.kernel@...il.com>, Jim Mattson <jmattson@...gle.com>, 
	Aaron Lewis <aaronlewis@...gle.com>, Paolo Bonzini <pbonzini@...hat.com>, 
	Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/6] KVM: x86: selftests: Test read/write core counters

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@...gle.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
>