Message-ID: <aNvzy5-lj3TBLT3I@google.com>
Date: Tue, 30 Sep 2025 08:14:19 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: Marc Zyngier <maz@...nel.org>, Sebastian Ott <sebott@...hat.com>, 
	Paolo Bonzini <pbonzini@...hat.com>, Shuah Khan <shuah@...nel.org>, kvm@...r.kernel.org, 
	kvmarm@...ts.linux.dev, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Naresh Kamboju <naresh.kamboju@...aro.org>
Subject: Re: [PATCH] KVM: selftests: fix irqfd_test on arm64

+Naresh

On Tue, Aug 26, 2025, Sean Christopherson wrote:
> On Tue, Aug 26, 2025, Oliver Upton wrote:
> > On Tue, Aug 26, 2025 at 11:51:18AM -0700, Sean Christopherson wrote:
> > > On Mon, Aug 25, 2025, Oliver Upton wrote:
> > > > The majority of selftests don't even need an irqchip anyway.
> > > 
> > > But it's really, really nice for developers if they can assume a certain level of
> > > configuration is done by the infrastructure, i.e. don't have worry about doing
> > > what is effectively "basic" VM setup.
> > 
> > The more we pile behind what a "basic" VM configuration is the less
> > expressive the tests become. Being able to immediately grok the *intent*
> > of a test from reading it first pass is a very good thing. Otherwise I
> > need to go figure out what the definition of "basic" means when I need
> > to write a test and decide if that is compatible with what I'm trying to
> > do.
> 
> Eh, I don't buy that argument, not as a blanket statement.
> 
> The existence of code doesn't always communicate intent, e.g. the _only_ instance
> I can think of where doing more setup by default caused problems was a few crusty
> x86 tests that relied on an int3 to cause SHUTDOWN due to lack of an IDT.  OMG was
> I increduluous when I figured out what those tests were doing.
> 
> And in that case, _not_ doing the "basic" setup hid the intent of the test.  Aside
> from the fact that deliberately triggering SHUTDOWN was completely unnecessary in
> those tests, IMO forcing such a test to use vm_create_barebones() would better
> capture that the test is doing something odd, i.e. has unusual intent.
> 
> And explicitly doing something doesn't necessarily communicate the intent of the
> test.  E.g. the intent of the irqfd_test is to verify that KVM_IRQFD assign and
> deassign behaves as expected.  The test never generates IRQs, i.e. doesn't actually
> need an IRQCHIP beyond satisfying KVM's requirements for KVM_IRQFD.
> 
> There are undoubtedly other tests that have similar "intent".  E.g. the in-progress
> mediated PMU support for x86 requires an in-kernel local APIC, and so tests like
> pmu_counters_test.c, pmu_event_filter_test.c, and vmx_pmu_caps_test.c will need
> to instantiate an IRQCHIP.  None of those tests actually touch the local APIC in
> any way, e.g. don't generate PMU interrupts, so creating an IRQCHIP is once again
> nothing more than a means to an end, and not indicative of the test's main intent.
> 
> I think the use of vgic_v3_setup() in dirty_log_perf_test.c is also a case where
> the existence of code fails to communicate intent.  Without the comment in
> arch_setup_vm() to explain that having GICv3 somehow reduces the number of exits,
> I would be very confused as to why the test cares about GICv3.
> 
> I agree there's a balance to be had in terms of doing too much.  Unfortunately in
> this case, it sounds like the fundamental problem is that the balance is simply
> different for x86 versus arm64.  Having an in-kernel local APIC is tables stakes
> for x86, to the point where I'm looking for any excuse to have KVM create a local
> APIC by default.  But for arm64, there's tremendous value in having tests do the
> lifting.
> 
> > vm_create_with_irqchip() is delightfully unambiguous.
> >
> > > E.g. x86 selftests creates an IRQCHIP, sets up descriptor tables and exception
> > > handlers, and a handful of other "basic" things, and that has eliminated soooo
> > > much boilerplate code and the associated friction with having to know/discover
> > > that e.g. sending IRQs in a test requires additional setup beyond the obvious
> > > steps like wiring up a handler.
> > 
> > That simply isn't going to happen on arm64. On top of the fact that the
> > irqchip configuration depends on the intent of the test (e.g. wired IRQs
> > v. MSIs), there's a bunch of guest-side initialization that needs to
> > happen too.
> > 
> > We can add an extremely barebones GIC when asked for (although guest
> > init isn't addressed) but batteries are not included on this architecture
> > and I'd rather not attempt to abstract that.
> 
> What about providing an API to do exactly that, instantiate and initialize a
> barebones GIC?  E.g.
> 
> 	void kvm_arch_init_barebones_irqchip(struct kvm_vm *vm)
> 
> Hmm, then we'd also need
> 
> 	void kvm_arch_vm_free(struct kvm_vm *vm)
> 
> to gracefully free the GIC, as done by dirty_log_perf_test.c.  Blech.  Though
> maybe we'll end up with that hook sooner or later?
> 
> All in all, I have no strong preference at this point.

Oliver, any thoughts?  This is causing noise in people's CIs, i.e. we should land
a fix sooner than later, even if it's not the "final" form. 

Powered by blists - more mailing lists