Re: [PATCH v2] KVM: selftest: Add a common check to identify AMD cpu in perf event filter test

[Sean Christopherson <seanjc@google.com>]

On Wed, Jun 05, 2024, Manali Shukla wrote:
> PMU event filter test fails on zen4 architecture because of the
> unavailability of family and model check for zen4 in use_amd_pmu().
> use_amd_pmu() is added to detect architectures that supports event

No, use_amd_pmu() already exists.

> select 0xc2 umask 0 as "retired branch instructions".
> 
> Model ranges in is_zen1(), is_zen2() and is_zen3() are used only for
> sever SOCs, so they might not cover all the model ranges which supports
> retired branch instructions.
> 
> X86_FEATURE_ZEN is a synthetic feature flag specifically added to
> recognize all Zen generations by commit 232afb557835d ("x86/CPU/AMD: Add
> X86_FEATURE_ZEN1"). init_amd_zen_common() uses family >= 0x17 check to
> enable X86_FEATURE_ZEN.

This is a lot of unnecessary noise.  It's mildly interesting that the kernel also
treats 17h+ as Zen, but the existence of a synthetic flag in the kernel doesn't
make this any more or less correct.

> Family 17h+ is where Zen and its successors start and that event 0xc2,0
> is supported on all currently released F17h+ processors as branch
> instruction retired and it is true going forward to maintain the
> backward compatibility for the branch instruction retired.
> 
> Since X86_FEATURE_ZEN is not recognized in selftest framework, instead
> of checking family and model value for all zen architecture, "family >=
> 0x17" check is added in use_amd_pmu().
> 
> Fixes: bef9a701f3eb ("selftests: kvm/x86: Add test for KVM_SET_PMU_EVENT_FILTER")

I don't think a Fixes tag is justified.  There was nothing wrong with the commit
when it went in, e.g. Zen4 wasn't even officialy released yet.

If we want to get test coverage on older kernels, then an explicit Cc: to stable
would be the way to go, but it's not clear to me that even that is warranted.

> Suggested-by: Sandipan Das <sandipan.das@amd.com>
> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
> ---

...

>  /*
> - * Determining AMD support for a PMU event requires consulting the AMD
> - * PPR for the CPU or reference material derived therefrom. The AMD
> - * test code herein has been verified to work on Zen1, Zen2, and Zen3.
> - *
> - * Feel free to add more AMD CPUs that are documented to support event
> - * select 0xc2 umask 0 as "retired branch instructions."
> + * Family 17h+ is where Zen and its successors start and that event
> + * 0xc2,0 is supported on all currently released F17h+ processors as
> + * branch instruction retired and it is true going forward to maintain
> + * the backward compatibility for the branch instruction retired.

For the comment, just state what the "rule" is, and leave it to the changelog to
explain why the rule is correct (i.e. that AMD pinky swears not to change it).

>   */
>  static bool use_amd_pmu(void)
>  {
>  	uint32_t family = kvm_cpu_family();
> -	uint32_t model = kvm_cpu_model();
> -
> -	return host_cpu_is_amd &&
> -		(is_zen1(family, model) ||
> -		 is_zen2(family, model) ||
> -		 is_zen3(family, model));
> +	return family >= 0x17;

This still needs to check host_cpu_is_amd.  There's also a comment elsewhere in
the file that needs to be updated.

All in all, this is what I'm applying (not yet tested).  Please holler if you
disagree with anything.

--
From: Manali Shukla <manali.shukla@amd.com>
Date: Wed, 5 Jun 2024 05:08:35 +0000
Subject: [PATCH] KVM: selftests: Treat AMD Family 17h+ as supporting branch
 insns retired

When detecting AMD PMU support for encoding "branch instructions retired"
as event 0xc2,0, simply check for Family 17h+ as all Zen CPUs support said
encoding, and AMD will maintain the encoding for backwards compatibility
on future CPUs.

Note, the kernel proper also interprets Family 17h+ as Zen (see the sole
caller of init_amd_zen_common()).

Suggested-by: Sandipan Das <sandipan.das@amd.com>
Signed-off-by: Manali Shukla <manali.shukla@amd.com>
Link: https://lore.kernel.org/r/20240605050835.30491-1-manali.shukla@amd.com
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../kvm/x86_64/pmu_event_filter_test.c        | 35 +++----------------
 1 file changed, 5 insertions(+), 30 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
index 26b3e7efe5dd..c15513cd74d1 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
@@ -32,8 +32,8 @@ struct __kvm_pmu_event_filter {
 
 /*
  * This event list comprises Intel's known architectural events, plus AMD's
- * "retired branch instructions" for Zen1-Zen3 (and* possibly other AMD CPUs).
- * Note, AMD and Intel use the same encoding for instructions retired.
+ * Branch Instructions Retired for Zen CPUs.  Note, AMD and Intel use the
+ * same encoding for Instructions Retired.
  */
 kvm_static_assert(INTEL_ARCH_INSTRUCTIONS_RETIRED == AMD_ZEN_INSTRUCTIONS_RETIRED);
 
@@ -353,38 +353,13 @@ static bool use_intel_pmu(void)
 	       kvm_pmu_has(X86_PMU_FEATURE_BRANCH_INSNS_RETIRED);
 }
 
-static bool is_zen1(uint32_t family, uint32_t model)
-{
-	return family == 0x17 && model <= 0x0f;
-}
-
-static bool is_zen2(uint32_t family, uint32_t model)
-{
-	return family == 0x17 && model >= 0x30 && model <= 0x3f;
-}
-
-static bool is_zen3(uint32_t family, uint32_t model)
-{
-	return family == 0x19 && model <= 0x0f;
-}
-
 /*
- * Determining AMD support for a PMU event requires consulting the AMD
- * PPR for the CPU or reference material derived therefrom. The AMD
- * test code herein has been verified to work on Zen1, Zen2, and Zen3.
- *
- * Feel free to add more AMD CPUs that are documented to support event
- * select 0xc2 umask 0 as "retired branch instructions."
+ * On AMD, all Family 17h+ CPUs (Zen and its successors) use event encoding
+ * 0xc2,0 for Branch Instructions Retired.
  */
 static bool use_amd_pmu(void)
 {
-	uint32_t family = kvm_cpu_family();
-	uint32_t model = kvm_cpu_model();
-
-	return host_cpu_is_amd &&
-		(is_zen1(family, model) ||
-		 is_zen2(family, model) ||
-		 is_zen3(family, model));
+	return host_cpu_is_amd && kvm_cpu_family() >= 0x17;
 }
 
 /*

base-commit: f626279dea33ba551839f2321511ad127e5a58e8
--