Message-ID: <Z5B5TJz9vn43AFcT@google.com>
Date: Wed, 22 Jan 2025 04:51:24 +0000
From: Mingwei Zhang <mizhang@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH 2/5] KVM: selftests: Only validate counts for
 hardware-supported arch events

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.

Powered by blists - more mailing lists