linux-kernel - Re: [PATCH 2/5] KVM: selftests: Only validate counts for hardware-supported arch events

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: <Z4r4UtpAIVe-EGeI@google.com>
Date: Fri, 17 Jan 2025 16:39:46 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Mingwei Zhang <mizhang@...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 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.