In the Intel PMU counters test, only validate the counts for architectural
events that are supported in hardware.  If an arch event isn't supported,
the event selector may enable a completely different event, and thus the
logic for the expected count is bogus.

This fixes test failures on pre-Icelake systems due to the encoding for
the architectural Top-Down Slots event corresponding to something else
(at least on the Skylake family of CPUs).

Note, validation relies on *hardware* support, not KVM support and not
guest support.  Architectural events are all about enumerating the event
selector encoding; lack of enumeration for an architectural event doesn't
mean the event itself is unsupported, i.e. the event should still count as
expected even if KVM and/or guest CPUID doesn't enumerate the event as
being "architectural".

Note #2, it's desirable to _program_ the architectural event encoding even
if hardware doesn't support the event.  The count can't be validated when
the event is fully enabled, but KVM should still let the guest program the
event selector, and the PMC shouldn't count if the event is disabled.

Fixes: 4f1bd6b16074 ("KVM: selftests: Test Intel PMU architectural events on gp counters")
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202501141009.30c629b4-lkp@intel.com
Debugged-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/x86/pmu_counters_test.c     | 25 +++++++++++++------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86/pmu_counters_test.c b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
index fe7d72fc8a75..8159615ad492 100644
--- a/tools/testing/selftests/kvm/x86/pmu_counters_test.c
+++ b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
@@ -29,6 +29,8 @@
 /* Total number of instructions retired within the measured section. */
 #define NUM_INSNS_RETIRED		(NUM_LOOPS * NUM_INSNS_PER_LOOP + NUM_EXTRA_INSNS)
 
+/* Track which architectural events are supported by hardware. */
+static uint32_t hardware_pmu_arch_events;
 
 static uint8_t kvm_pmu_version;
 static bool kvm_has_perf_caps;
@@ -89,6 +91,7 @@ static struct kvm_vm *pmu_vm_create_with_one_vcpu(struct kvm_vcpu **vcpu,
 
 	vm = vm_create_with_one_vcpu(vcpu, guest_code);
 	sync_global_to_guest(vm, kvm_pmu_version);
+	sync_global_to_guest(vm, hardware_pmu_arch_events);
 
 	/*
 	 * Set PERF_CAPABILITIES before PMU version as KVM disallows enabling
@@ -152,7 +155,7 @@ static void guest_assert_event_count(uint8_t idx,
 	uint64_t count;
 
 	count = _rdpmc(pmc);
-	if (!this_pmu_has(event))
+	if (!(hardware_pmu_arch_events & BIT(idx)))
 		goto sanity_checks;
 
 	switch (idx) {
@@ -560,7 +563,7 @@ static void test_fixed_counters(uint8_t pmu_version, uint64_t perf_capabilities,
 
 static void test_intel_counters(void)
 {
-	uint8_t nr_arch_events = kvm_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
+	uint8_t nr_arch_events = this_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
 	uint8_t nr_fixed_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
 	uint8_t nr_gp_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
 	uint8_t pmu_version = kvm_cpu_property(X86_PROPERTY_PMU_VERSION);
@@ -582,18 +585,26 @@ static void test_intel_counters(void)
 
 	/*
 	 * Detect the existence of events that aren't supported by selftests.
-	 * This will (obviously) fail any time the kernel adds support for a
-	 * new event, but it's worth paying that price to keep the test fresh.
+	 * This will (obviously) fail any time hardware adds support for a new
+	 * event, but it's worth paying that price to keep the test fresh.
 	 */
 	TEST_ASSERT(nr_arch_events <= NR_INTEL_ARCH_EVENTS,
 		    "New architectural event(s) detected; please update this test (length = %u, mask = %x)",
-		    nr_arch_events, kvm_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
+		    nr_arch_events, this_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
 
 	/*
-	 * Force iterating over known arch events regardless of whether or not
-	 * KVM/hardware supports a given event.
+	 * Iterate over known arch events irrespective of KVM/hardware support
+	 * to verify that KVM doesn't reject programming of events just because
+	 * the *architectural* encoding is unsupported.  Track which events are
+	 * supported in hardware; the guest side will validate supported events
+	 * count correctly, even if *enumeration* of the event is unsupported
+	 * by KVM and/or isn't exposed to the guest.
 	 */
 	nr_arch_events = max_t(typeof(nr_arch_events), nr_arch_events, NR_INTEL_ARCH_EVENTS);
+	for (i = 0; i < nr_arch_events; i++) {
+		if (this_pmu_has(intel_event_to_feature(i).gp_event))
+			hardware_pmu_arch_events |= BIT(i);
+	}
 
 	for (v = 0; v <= max_pmu_version; v++) {
 		for (i = 0; i < ARRAY_SIZE(perf_caps); i++) {
-- 
2.48.0.rc2.279.g1de40edade-goog

On Fri, Jan 17, 2025, Sean Christopherson wrote:
> In the Intel PMU counters test, only validate the counts for architectural
> events that are supported in hardware.  If an arch event isn't supported,
> the event selector may enable a completely different event, and thus the
> logic for the expected count is bogus.
> 
> This fixes test failures on pre-Icelake systems due to the encoding for
> the architectural Top-Down Slots event corresponding to something else
> (at least on the Skylake family of CPUs).
> 
> Note, validation relies on *hardware* support, not KVM support and not
> guest support.  Architectural events are all about enumerating the event
> selector encoding; lack of enumeration for an architectural event doesn't
> mean the event itself is unsupported, i.e. the event should still count as
> expected even if KVM and/or guest CPUID doesn't enumerate the event as
> being "architectural".
> 
> Note #2, it's desirable to _program_ the architectural event encoding even
> if hardware doesn't support the event.  The count can't be validated when
> the event is fully enabled, but KVM should still let the guest program the
> event selector, and the PMC shouldn't count if the event is disabled.
> 
> Fixes: 4f1bd6b16074 ("KVM: selftests: Test Intel PMU architectural events on gp counters")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202501141009.30c629b4-lkp@intel.com
> Debugged-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  .../selftests/kvm/x86/pmu_counters_test.c     | 25 +++++++++++++------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86/pmu_counters_test.c b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> index fe7d72fc8a75..8159615ad492 100644
> --- a/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> +++ b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> @@ -29,6 +29,8 @@
>  /* Total number of instructions retired within the measured section. */
>  #define NUM_INSNS_RETIRED		(NUM_LOOPS * NUM_INSNS_PER_LOOP + NUM_EXTRA_INSNS)
>  
> +/* Track which architectural events are supported by hardware. */
> +static uint32_t hardware_pmu_arch_events;
>  
>  static uint8_t kvm_pmu_version;
>  static bool kvm_has_perf_caps;
> @@ -89,6 +91,7 @@ static struct kvm_vm *pmu_vm_create_with_one_vcpu(struct kvm_vcpu **vcpu,
>  
>  	vm = vm_create_with_one_vcpu(vcpu, guest_code);
>  	sync_global_to_guest(vm, kvm_pmu_version);
> +	sync_global_to_guest(vm, hardware_pmu_arch_events);
>  
>  	/*
>  	 * Set PERF_CAPABILITIES before PMU version as KVM disallows enabling
> @@ -152,7 +155,7 @@ static void guest_assert_event_count(uint8_t idx,
>  	uint64_t count;
>  
>  	count = _rdpmc(pmc);
> -	if (!this_pmu_has(event))
> +	if (!(hardware_pmu_arch_events & BIT(idx)))
>  		goto sanity_checks;
>  
>  	switch (idx) {
> @@ -560,7 +563,7 @@ static void test_fixed_counters(uint8_t pmu_version, uint64_t perf_capabilities,
>  
>  static void test_intel_counters(void)
>  {
> -	uint8_t nr_arch_events = kvm_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
> +	uint8_t nr_arch_events = this_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
>  	uint8_t nr_fixed_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
>  	uint8_t nr_gp_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
>  	uint8_t pmu_version = kvm_cpu_property(X86_PROPERTY_PMU_VERSION);
> @@ -582,18 +585,26 @@ static void test_intel_counters(void)
>  
>  	/*
>  	 * Detect the existence of events that aren't supported by selftests.
> -	 * This will (obviously) fail any time the kernel adds support for a
> -	 * new event, but it's worth paying that price to keep the test fresh.
> +	 * This will (obviously) fail any time hardware adds support for a new
> +	 * event, but it's worth paying that price to keep the test fresh.
>  	 */
>  	TEST_ASSERT(nr_arch_events <= NR_INTEL_ARCH_EVENTS,
>  		    "New architectural event(s) detected; please update this test (length = %u, mask = %x)",
> -		    nr_arch_events, kvm_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
> +		    nr_arch_events, this_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));

This is where it would make troubles for us (all companies that might be
using the selftest in upstream kernel and having a new hardware). In
this case when we get new hardware, the test will fail in the downstream
kernel. We will have to wait until the fix is ready, and backport it
downstream, re-test it.... It takes lots of extra work.

Perhaps we can just putting nr_arch_events = NR_INTEL_ARCH_EVENTS
if the former is larger than or equal to the latter? So that the "test"
only test what it knows. It does not test what it does not know, i.e.,
it does not "assume" it knows everything. We can always a warning or
info log at the moment. Then expanding the capability of the test should
be added smoothly later by either maintainers of SWEs from CPU vendors
without causing failures.

Thanks.
-Mingwei
>  
>  	/*
> -	 * Force iterating over known arch events regardless of whether or not
> -	 * KVM/hardware supports a given event.
> +	 * Iterate over known arch events irrespective of KVM/hardware support
> +	 * to verify that KVM doesn't reject programming of events just because
> +	 * the *architectural* encoding is unsupported.  Track which events are
> +	 * supported in hardware; the guest side will validate supported events
> +	 * count correctly, even if *enumeration* of the event is unsupported
> +	 * by KVM and/or isn't exposed to the guest.
>  	 */
>  	nr_arch_events = max_t(typeof(nr_arch_events), nr_arch_events, NR_INTEL_ARCH_EVENTS);
> +	for (i = 0; i < nr_arch_events; i++) {
> +		if (this_pmu_has(intel_event_to_feature(i).gp_event))
> +			hardware_pmu_arch_events |= BIT(i);
> +	}
>  
>  	for (v = 0; v <= max_pmu_version; v++) {
>  		for (i = 0; i < ARRAY_SIZE(perf_caps); i++) {
> -- 
> 2.48.0.rc2.279.g1de40edade-goog
>

On Sat, Jan 18, 2025, Mingwei Zhang wrote:
> On Fri, Jan 17, 2025, Sean Christopherson wrote:
> > @@ -582,18 +585,26 @@ static void test_intel_counters(void)
> >  
> >  	/*
> >  	 * Detect the existence of events that aren't supported by selftests.
> > -	 * This will (obviously) fail any time the kernel adds support for a
> > -	 * new event, but it's worth paying that price to keep the test fresh.
> > +	 * This will (obviously) fail any time hardware adds support for a new
> > +	 * event, but it's worth paying that price to keep the test fresh.
> >  	 */
> >  	TEST_ASSERT(nr_arch_events <= NR_INTEL_ARCH_EVENTS,
> >  		    "New architectural event(s) detected; please update this test (length = %u, mask = %x)",
> > -		    nr_arch_events, kvm_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
> > +		    nr_arch_events, this_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
> 
> This is where it would make troubles for us (all companies that might be
> using the selftest in upstream kernel and having a new hardware). In
> this case when we get new hardware, the test will fail in the downstream
> kernel. We will have to wait until the fix is ready, and backport it
> downstream, re-test it.... It takes lots of extra work.

If Intel can't upstream what should be a *very* simple patch to enumerate the
new encoding and its expected count in advance of hardware being shipped to
partners, then we have bigger problems.  I don't know what level of pre-silicon
and FPGA-based emulation testing Intel does these days, but I wouldn't be at all
surprised if KVM tests are being run well before silicon arrives.

I am not at all convinced that this will ever affect anyone besides the Intel
engineers doing early enablement, and I am definitely not convinced it will ever
take significant effort above beyond what would be required irrespective of what
approach we take.  E.g. figuring out what the expected count is might be time
consuming, but I don't expect updating the test to be difficult.

> Perhaps we can just putting nr_arch_events = NR_INTEL_ARCH_EVENTS
> if the former is larger than or equal to the latter? So that the "test"
> only test what it knows. It does not test what it does not know, i.e.,
> it does not "assume" it knows everything. We can always a warning or
> info log at the moment. Then expanding the capability of the test should
> be added smoothly later by either maintainers of SWEs from CPU vendors
> without causing failures.

If we just "warn", we're effectively relying on a future Intel engineer to run
the test *and* check the logs *and* actually act on the warning.  Given that tests
are rarely run manually with a human pouring over the output, I highly doubt that
will pan out.

The more likely scenario is that the warn go unnoticed until some random person
sees the warn and files a bug somewhere.  At that point, odds are good that someone
who knows nothing about Intel's new hardware/event will get saddled with hunting
down the appropriate specs, digging into the details of the event, submitting a
patch upstream, etc.

And if multiple someones detect the warn, e.g. at different companines, then we've
collectively wasted even more time.  Which is a pretty likely scenario, because I
know with 100% certainly that people carry out-of-tree fixes :-)

By failing the test, pretty much the only assumption we're making is that Intel
cares about upstream KVM.  All evidence suggests that's a very safe assumption.
And as shown by my fumbling with Top-Down Slots, Intel engineers are absolutely
the right people to tackle this sort of thing, as they have accesses to resources
about uarch/CPU behavior that others don't.

If this approach turns out to be a huge pain, then we can certainly revisit things.
But I truly expect this will be less work for everyone, Intel included.

On Fri, Jan 17, 2025, Sean Christopherson wrote:
> On Sat, Jan 18, 2025, Mingwei Zhang wrote:
> > On Fri, Jan 17, 2025, Sean Christopherson wrote:
> > > @@ -582,18 +585,26 @@ static void test_intel_counters(void)
> > >  
> > >  	/*
> > >  	 * Detect the existence of events that aren't supported by selftests.
> > > -	 * This will (obviously) fail any time the kernel adds support for a
> > > -	 * new event, but it's worth paying that price to keep the test fresh.
> > > +	 * This will (obviously) fail any time hardware adds support for a new
> > > +	 * event, but it's worth paying that price to keep the test fresh.
> > >  	 */
> > >  	TEST_ASSERT(nr_arch_events <= NR_INTEL_ARCH_EVENTS,
> > >  		    "New architectural event(s) detected; please update this test (length = %u, mask = %x)",
> > > -		    nr_arch_events, kvm_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
> > > +		    nr_arch_events, this_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
> > 
> > This is where it would make troubles for us (all companies that might be
> > using the selftest in upstream kernel and having a new hardware). In
> > this case when we get new hardware, the test will fail in the downstream
> > kernel. We will have to wait until the fix is ready, and backport it
> > downstream, re-test it.... It takes lots of extra work.
> 
> If Intel can't upstream what should be a *very* simple patch to enumerate the
> new encoding and its expected count in advance of hardware being shipped to
> partners, then we have bigger problems.  I don't know what level of pre-silicon
> and FPGA-based emulation testing Intel does these days, but I wouldn't be at all
> surprised if KVM tests are being run well before silicon arrives.

Right, Intel folks will be the 1st one that is impacted. But it should
be easy to fix on their end. But upstreaming the change may come very
late.

> 
> I am not at all convinced that this will ever affect anyone besides the Intel
> engineers doing early enablement, and I am definitely not convinced it will ever
> take significant effort above beyond what would be required irrespective of what
> approach we take.  E.g. figuring out what the expected count is might be time
> consuming, but I don't expect updating the test to be difficult.

It will affect the downstream kernels, I think? Like Paolo mentioned,
old distro kernel may run on new hardware. In usual cases, Intel HW has
already come out for a while, and the upstream software update is still
there under review. Fixing the problem is never difficult. But we need a
minor fix for each new generation of HW that adds a new architecture
event. I can imagine fixing that needs a simple patch, but each of them
has to cc stable tree and with "Fixes" tag.

> 
> > Perhaps we can just putting nr_arch_events = NR_INTEL_ARCH_EVENTS
> > if the former is larger than or equal to the latter? So that the "test"
> > only test what it knows. It does not test what it does not know, i.e.,
> > it does not "assume" it knows everything. We can always a warning or
> > info log at the moment. Then expanding the capability of the test should
> > be added smoothly later by either maintainers of SWEs from CPU vendors
> > without causing failures.
> 
> If we just "warn", we're effectively relying on a future Intel engineer to run
> the test *and* check the logs *and* actually act on the warning.  Given that tests
> are rarely run manually with a human pouring over the output, I highly doubt that
> will pan out.

In reality, we may not even need to warn. If the test only covers what
it knows, then there is no alarm to report. On the other hand, CPU
vendor who is doing the development will have selftest series to
update it. It will not just increase the array size, but also add more
meaningful testcases to the new events. The assumption is CPU vendors
care about upstream, which I think is true.
> 
> The more likely scenario is that the warn go unnoticed until some random person
> sees the warn and files a bug somewhere.  At that point, odds are good that someone
> who knows nothing about Intel's new hardware/event will get saddled with hunting
> down the appropriate specs, digging into the details of the event, submitting a
> patch upstream, etc.
> 
> And if multiple someones detect the warn, e.g. at different companines, then we've
> collectively wasted even more time.  Which is a pretty likely scenario, because I
> know with 100% certainly that people carry out-of-tree fixes :-)

Right, so a warning is not necessary here.
> 
> By failing the test, pretty much the only assumption we're making is that Intel
> cares about upstream KVM.  All evidence suggests that's a very safe assumption.
> And as shown by my fumbling with Top-Down Slots, Intel engineers are absolutely
> the right people to tackle this sort of thing, as they have accesses to resources
> about uarch/CPU behavior that others don't.

Well, yes. I believe Intel cares about it. I see Dapeng's effort, which
is really great. But often the effort is not just to fix that simple
bug, there are other parts that need to be done to test Top-Down slots
and other fixes. For instance, Dapeng's series for kvm-unit-tests/pmu is
still under review after 12+ months?

https://lore.kernel.org/all/20240914101728.33148-8-dapeng1.mi@linux.intel.com/

I think this is a minor pain for us, since we are strictly following the
upstream version on kut. Maybe that is something we need to change?

Thanks.
-Mingwei
>
> If this approach turns out to be a huge pain, then we can certainly revisit things.
> But I truly expect this will be less work for everyone, Intel included.

On Wed, Jan 22, 2025, Mingwei Zhang wrote:
> On Fri, Jan 17, 2025, Sean Christopherson wrote:
> > On Sat, Jan 18, 2025, Mingwei Zhang wrote:
> > > On Fri, Jan 17, 2025, Sean Christopherson wrote:
> > > > @@ -582,18 +585,26 @@ static void test_intel_counters(void)
> > > >  
> > > >  	/*
> > > >  	 * Detect the existence of events that aren't supported by selftests.
> > > > -	 * This will (obviously) fail any time the kernel adds support for a
> > > > -	 * new event, but it's worth paying that price to keep the test fresh.
> > > > +	 * This will (obviously) fail any time hardware adds support for a new
> > > > +	 * event, but it's worth paying that price to keep the test fresh.
> > > >  	 */
> > > >  	TEST_ASSERT(nr_arch_events <= NR_INTEL_ARCH_EVENTS,
> > > >  		    "New architectural event(s) detected; please update this test (length = %u, mask = %x)",
> > > > -		    nr_arch_events, kvm_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
> > > > +		    nr_arch_events, this_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
> > > 
> > > This is where it would make troubles for us (all companies that might be
> > > using the selftest in upstream kernel and having a new hardware). In
> > > this case when we get new hardware, the test will fail in the downstream
> > > kernel. We will have to wait until the fix is ready, and backport it
> > > downstream, re-test it.... It takes lots of extra work.
> > 
> > If Intel can't upstream what should be a *very* simple patch to enumerate the
> > new encoding and its expected count in advance of hardware being shipped to
> > partners, then we have bigger problems.  I don't know what level of pre-silicon
> > and FPGA-based emulation testing Intel does these days, but I wouldn't be at all
> > surprised if KVM tests are being run well before silicon arrives.
> 
> Right, Intel folks will be the 1st one that is impacted. But it should
> be easy to fix on their end. But upstreaming the change may come very
> late.

And I'm saying we do what we can to motivate upstreaming the "fix" sooner than
later.

> > I am not at all convinced that this will ever affect anyone besides the Intel
> > engineers doing early enablement, and I am definitely not convinced it will ever
> > take significant effort above beyond what would be required irrespective of what
> > approach we take.  E.g. figuring out what the expected count is might be time
> > consuming, but I don't expect updating the test to be difficult.
> 
> It will affect the downstream kernels, I think? Like Paolo mentioned,
> old distro kernel may run on new hardware. In usual cases, Intel HW has
> already come out for a while, and the upstream software update is still
> there under review.

This isn't a usual case.  Or at least, it shouldn't be.  The effort required
should be more on par with adding Family/Model/Stepping information, and Intel
is quite capable of landing those types of changes well in advance of general
availability.

E.g. commit 7beade0dd41d ("x86/cpu: Add several Intel server CPU model numbers")
added Sierra Forest and Granite Rapids two years before they launched.  I don't
know when third parties first got silicion, but I would be surprised if it was
much, if at all, before that commit.

> Fixing the problem is never difficult. But we need a minor fix for each new
> generation of HW that adds a new architecture event. I can imagine fixing
> that needs a simple patch, but each of them has to cc stable tree

Yes, but sending patches to LTS kernels isn't inherently bad.  If the changes end
up conflicting regularly, then we can certainly reconsider the cost vs. benefit
of the assert.

> and with "Fixes" tag.

Nit, it doesn't need a Fixes, just Cc: stable@.

> > > Perhaps we can just putting nr_arch_events = NR_INTEL_ARCH_EVENTS
> > > if the former is larger than or equal to the latter? So that the "test"
> > > only test what it knows. It does not test what it does not know, i.e.,
> > > it does not "assume" it knows everything. We can always a warning or
> > > info log at the moment. Then expanding the capability of the test should
> > > be added smoothly later by either maintainers of SWEs from CPU vendors
> > > without causing failures.
> > 
> > If we just "warn", we're effectively relying on a future Intel engineer to run
> > the test *and* check the logs *and* actually act on the warning.  Given that tests
> > are rarely run manually with a human pouring over the output, I highly doubt that
> > will pan out.
> 
> In reality, we may not even need to warn. If the test only covers what
> it knows, then there is no alarm to report.

The assertion/warn isn't about test correctness, it's about ensuring test coverage
and distributing maintenance burden.  I don't want to have to chase down someone
at Intel that can provide the gory details on whatever architectural event comes
along next.

On 1/18/25 01:39, Sean Christopherson wrote:
> On Sat, Jan 18, 2025, Mingwei Zhang wrote:
>> On Fri, Jan 17, 2025, Sean Christopherson wrote:
>>> @@ -582,18 +585,26 @@ static void test_intel_counters(void)
>>>   
>>>   	/*
>>>   	 * Detect the existence of events that aren't supported by selftests.
>>> -	 * This will (obviously) fail any time the kernel adds support for a
>>> -	 * new event, but it's worth paying that price to keep the test fresh.
>>> +	 * This will (obviously) fail any time hardware adds support for a new
>>> +	 * event, but it's worth paying that price to keep the test fresh.
>>>   	 */
>>>   	TEST_ASSERT(nr_arch_events <= NR_INTEL_ARCH_EVENTS,
>>>   		    "New architectural event(s) detected; please update this test (length = %u, mask = %x)",
>>> -		    nr_arch_events, kvm_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
>>> +		    nr_arch_events, this_cpu_property(X86_PROPERTY_PMU_EVENTS_MASK));
>>
>> This is where it would make troubles for us (all companies that might be
>> using the selftest in upstream kernel and having a new hardware). In
>> this case when we get new hardware, the test will fail in the downstream
>> kernel. We will have to wait until the fix is ready, and backport it
>> downstream, re-test it.... It takes lots of extra work.
> 
> If Intel can't upstream what should be a *very* simple patch to enumerate the
> new encoding and its expected count in advance of hardware being shipped to
> partners, then we have bigger problems.

Conceptually I have bigger problems with people running stable kernels 
than people running on really really new hardware.

However the intersection of running a pretty old kernel on a very new 
bare metal x86 system is relatively small but nonzero (those pesky 
Debian users); it may happen with cloud instances but then the 
intersection of running old selftests in a nested virt environment is 
probably even smaller.

I am not too happy about the assertion, but it does seem like the lesser 
evil.

Paolo