+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.
