From: Sean Christopherson <seanjc@google.com>
To: Dapeng Mi <dapeng1.mi@linux.intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
Jim Mattson <jmattson@google.com>,
Mingwei Zhang <mizhang@google.com>,
Zide Chen <zide.chen@intel.com>,
Das Sandipan <Sandipan.Das@amd.com>,
Shukla Manali <Manali.Shukla@amd.com>,
Yi Lai <yi1.lai@intel.com>, Dapeng Mi <dapeng1.mi@intel.com>
Subject: Re: [PATCH v2 3/5] KVM: Selftests: Validate more arch-events in pmu_counters_test
Date: Wed, 10 Sep 2025 16:51:01 -0700 [thread overview]
Message-ID: <aMIO5ZLNur5JkdYl@google.com> (raw)
In-Reply-To: <20250718001905.196989-4-dapeng1.mi@linux.intel.com>
On Fri, Jul 18, 2025, Dapeng Mi wrote:
> Clearwater Forest introduces 5 new architectural events (4 topdown
> level 1 metrics events and LBR inserts event). This patch supports
> to validate these 5 newly added events. The detailed info about these
> 5 events can be found in SDM section 21.2.7 "Pre-defined Architectural
> Performance Events".
>
> It becomes unrealistic to traverse all possible combinations of
> unavailable events mask (may need dozens of minutes to finish all
> possible combination validation). So only limit unavailable events mask
> traverse to the first 8 arch-events.
Split these into separate patches. Buring a meaningful change like this in a big
patch that seemingly just adds architectural collateral is pure evil.
> @@ -612,15 +620,19 @@ static void test_intel_counters(void)
> pr_info("Testing arch events, PMU version %u, perf_caps = %lx\n",
> v, perf_caps[i]);
> /*
> - * To keep the total runtime reasonable, test every
> - * possible non-zero, non-reserved bitmap combination
> - * only with the native PMU version and the full bit
> - * vector length.
> + * To keep the total runtime reasonable, especially after
> + * the total number of arch-events increasing to 13, It's
> + * impossible to test every possible non-zero, non-reserved
> + * bitmap combination. Only test the first 8-bits combination
> + * with the native PMU version and the full bit vector length.
> */
> if (v == pmu_version) {
> - for (k = 1; k < (BIT(NR_INTEL_ARCH_EVENTS) - 1); k++)
> + int max_events = min(NR_INTEL_ARCH_EVENTS, 8);
Too arbitrary, and worse, bad coverage. And honestly, even iterating over 255
(or 512?) different values is a waste of time. Ha! And test_arch_events() is
buggy, it takes unavailable_mask as u8 instead of a u32. I'll slot in a patch
to fix that.
As for the runtime, I think it's time to throw in the towel in terms of brute
forcing the validation space, and just test a handful of hopefully-interesting
values, e.g.
---
.../selftests/kvm/x86/pmu_counters_test.c | 38 +++++++++++--------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86/pmu_counters_test.c b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
index cfeed0103341..09ad68675576 100644
--- a/tools/testing/selftests/kvm/x86/pmu_counters_test.c
+++ b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
@@ -577,6 +577,26 @@ static void test_intel_counters(void)
PMU_CAP_FW_WRITES,
};
+ /*
+ * To keep the total runtime reasonable, test only a handful of select,
+ * semi-arbitrary values for the mask of unavailable PMU events. Test
+ * 0 (all events available) and all ones (no events available) as well
+ * as alternating bit sequencues, e.g. to detect if KVM is checking the
+ * wrong bit(s).
+ */
+ const uint32_t unavailable_masks[] = {
+ 0x0,
+ 0xffffffffu,
+ 0xf0f0f0f0u,
+ 0x0f0f0f0fu,
+ 0xaaaaaaaau,
+ 0xa0a0a0a0u,
+ 0x0a0a0a0au,
+ 0x55555555u,
+ 0x50505050u,
+ 0x05050505u,
+ };
+
/*
* Test up to PMU v5, which is the current maximum version defined by
* Intel, i.e. is the last version that is guaranteed to be backwards
@@ -614,16 +634,7 @@ static void test_intel_counters(void)
pr_info("Testing arch events, PMU version %u, perf_caps = %lx\n",
v, perf_caps[i]);
- /*
- * To keep the total runtime reasonable, test every
- * possible non-zero, non-reserved bitmap combination
- * only with the native PMU version and the full bit
- * vector length.
- */
- if (v == pmu_version) {
- for (k = 1; k < (BIT(NR_INTEL_ARCH_EVENTS) - 1); k++)
- test_arch_events(v, perf_caps[i], NR_INTEL_ARCH_EVENTS, k);
- }
+
/*
* Test single bits for all PMU version and lengths up
* the number of events +1 (to verify KVM doesn't do
@@ -632,11 +643,8 @@ static void test_intel_counters(void)
* ones i.e. all events being available and unavailable.
*/
for (j = 0; j <= NR_INTEL_ARCH_EVENTS + 1; j++) {
- test_arch_events(v, perf_caps[i], j, 0);
- test_arch_events(v, perf_caps[i], j, -1u);
-
- for (k = 0; k < NR_INTEL_ARCH_EVENTS; k++)
- test_arch_events(v, perf_caps[i], j, BIT(k));
+ for (k = 1; k < ARRAY_SIZE(unavailable_masks); k++)
+ test_arch_events(v, perf_caps[i], j, unavailable_masks[k]);
}
pr_info("Testing GP counters, PMU version %u, perf_caps = %lx\n",
--
next prev parent reply other threads:[~2025-09-10 23:51 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-18 0:19 [PATCH v2 0/5] Fix PMU kselftests errors on GNR/SRF/CWF Dapeng Mi
2025-07-18 0:19 ` [PATCH v2 1/5] KVM: x86/pmu: Correct typo "_COUTNERS" to "_COUNTERS" Dapeng Mi
2025-07-18 0:19 ` [PATCH v2 2/5] KVM: selftests: Add timing_info bit support in vmx_pmu_caps_test Dapeng Mi
2025-09-10 22:03 ` Sean Christopherson
2025-09-11 1:20 ` Mi, Dapeng
2025-07-18 0:19 ` [PATCH v2 3/5] KVM: Selftests: Validate more arch-events in pmu_counters_test Dapeng Mi
2025-09-10 23:51 ` Sean Christopherson [this message]
2025-09-11 1:41 ` Mi, Dapeng
2025-07-18 0:19 ` [PATCH v2 4/5] KVM: selftests: Relax precise event count validation as overcount issue Dapeng Mi
2025-09-10 23:56 ` Sean Christopherson
2025-09-11 1:55 ` Mi, Dapeng
2025-07-18 0:19 ` [PATCH v2 5/5] KVM: selftests: Relax branches event count check for event_filter test Dapeng Mi
2025-09-10 23:52 ` Sean Christopherson
2025-09-10 23:59 ` [PATCH v2 0/5] Fix PMU kselftests errors on GNR/SRF/CWF Sean Christopherson
2025-09-11 1:59 ` Mi, Dapeng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aMIO5ZLNur5JkdYl@google.com \
--to=seanjc@google.com \
--cc=Manali.Shukla@amd.com \
--cc=Sandipan.Das@amd.com \
--cc=dapeng1.mi@intel.com \
--cc=dapeng1.mi@linux.intel.com \
--cc=jmattson@google.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mizhang@google.com \
--cc=pbonzini@redhat.com \
--cc=yi1.lai@intel.com \
--cc=zide.chen@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.