Re: [PATCH v2 03/14] KVM: selftests: Add vfio_pci_irq_test

Josh Hilke
2026-04-02T00:13:45Z

On Wed, Apr 01, 2026 at 12:58:08PM -0700, Sean Christopherson wrote:
> On Tue, Mar 31, 2026, Josh Hilke wrote:
> > +int main(int argc, char **argv)
> > +{
> > + /*
> > + * Pick a random vector and a random GSI to use for device IRQ.
> > + *
> > + * Pick an IRQ vector in range [32, UINT8_MAX]. Min value is 32 because
> > + * Linux/x86 reserves vectors 0-31 for exceptions and architecture
> > + * defined NMIs and interrupts.
> > + *
> > + * Pick a GSI in range [24, KVM_MAX_IRQ_ROUTES - 1]. The min value is 24
> > + * because KVM reserves GSIs 0-15 for legacy ISA IRQs and 16-23 only go
> > + * to the IOAPIC. The max is KVM_MAX_IRQ_ROUTES - 1, because
> > + * KVM_MAX_IRQ_ROUTES is exclusive.
> > + */
> > + u32 gsi = 24 + rand() % (KVM_MAX_IRQ_ROUTES - 1 - 24);
> > + u8 vector = 32 + rand() % (UINT8_MAX - 32);
>
> Ahh, now I see why you included "Reproduce tests that rely on randomization" in
> this series.
>
> I think I'd prefer to tweak guest_random_state() to drop the "guest" in favor of
> "kvm", and then use that framework in host code as well. Then you wouldn't need
> to open code the % fun.
>

How would using the guest_random_state() library remove the need to calculate the appropriate ranges using %?
The library (tools/testing/selftests/kvm/include/test_util.h) only provides:

uint64_t guest_random_u64(struct guest_random_state *state);
uint32_t guest_random_u32(struct guest_random_state *state);

I could avoid using % by adding a new library function that picks a
random number within a range:

uint64_t kvm_random_u64_in_range(struct kvm_random_state *state, uint64_t min, uint64_t max);

> > + /* Test configuration (overridable by command line flags). */
> > + int nr_irqs = 1000;
> > + int nr_vcpus = 1;
> > +
> > + struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
> > + pthread_t vcpu_threads[KVM_MAX_VCPUS];
> > + u64 irq_count, pin_count, piw_count;
> > + struct vfio_pci_device *device;
> > + struct iommu *iommu;
> > + const char *device_bdf;
> > + int i, j, c, msi, irq;
> > + struct kvm_vm *vm;
> > +
> > + device_bdf = vfio_selftests_get_bdf(&argc, argv);
> > +
> > + while ((c = getopt(argc, argv, "h")) != -1) {
> > + switch (c) {
> > + case 'h':
> > + default:
> > + help(argv[0]);
> > + }
> > + }
> > +
> > + vm = vm_create_with_vcpus(nr_vcpus, guest_code, vcpus);
> > + vm_install_exception_handler(vm, vector, guest_irq_handler);
> > +
> > + iommu = iommu_init(default_iommu_mode);
> > + device = vfio_pci_device_init(device_bdf, iommu);
> > + msi = setup_msi(device);
> > + irq = get_irq_number(device_bdf, msi);
> > +
> > + irq_count = get_irq_count(irq);
> > + pin_count = get_irq_count_by_name("PIN:");
> > + piw_count = get_irq_count_by_name("PIW:");
>
> I'm *very* skeptical of blindling printing PIN/PIW. They're Intel specific and
> are global to the entire system. Outside of debugging specific issues, I don't
> see them providing much value, while on the other hand, they could easily confuse
> readers. And unlike the IRQ counts, the data won't be lost when the test exits.
>

I'll remove PIN/PIW in v3.

> As for the get_irq_number() and get_irq_count(), those need to be much more clearly
> scoped to /proc/interrupts. E.g. "irq" in KVM context most often refers to the
> guest IRQ vector, not the host Linux make-believe IRQ number.
>

How about I rename these to get_proc_irq_num() and get_proc_irq_count()?
And then rename the library irq_util.h/c to proc_util.h/c?

> > + printf("%s %s MSI-X[%d] (IRQ-%d) %d times\n",
> > + "Notifying the eventfd for",
>
> Where's the rest of this line? Oh, this is the first %s. LOL, that's ridiculous.
>
> printf("Notifying the eventfd for BDF %s, MSI-X[%d] (IRQ-%d) %d times\n",
> device_bdf, msi, irq, nr_irqs);
>

Will fix in v3.

> > + device_bdf, msi, irq, nr_irqs);
> > +
> > + kvm_assign_irqfd(vm, gsi, device->msi_eventfds[msi]);
> > +
> > + for (i = 0; i < nr_vcpus; i++)
> > + pthread_create(&vcpu_threads[i], NULL, vcpu_thread_main, vcpus[i]);
> > +
> > + for (i = 0; i < nr_vcpus; i++) {
> > + struct kvm_vcpu *vcpu = vcpus[i];
> > +
> > + while (!READ_FROM_GUEST(vm, guest_ready_for_irqs[vcpu->id]))
> > + continue;
> > + }
> > +
> > + /* Set a consistent seed so that test are repeatable. */
> > + srand(0);
>
> And using a pRNG instead of rand() should make this unnecessary.
>

We shouldn't even need a pRNG if we follow your suggestion above and use
the guest_random_state() framework for the host, as it would provide the
guest_random_u64() function.

> > + for (i = 0; i < nr_irqs; i++) {
> > + struct kvm_vcpu *vcpu = vcpus[i % nr_vcpus];
> > + struct timespec start;
> > +
> > + kvm_route_msi(vm, gsi, vcpu, vector);
> > +
> > + for (j = 0; j < nr_vcpus; j++) {
> > + TEST_ASSERT(
> > + !READ_FROM_GUEST(vm, guest_received_irq[vcpu->id]),
> > + "IRQ flag for vCPU %d not clear prior to test",
> > + vcpu->id);
> > + }
> > +
> > + send_msi(device, msi);
> > +
> > + clock_gettime(CLOCK_MONOTONIC, &start);
> > + for (;;) {
> > + if (READ_FROM_GUEST(vm, guest_received_irq[vcpu->id]))
> > + break;
> > +
> > + if (timespec_to_ns(timespec_elapsed(start)) > TIMEOUT_NS) {
> > + printf("Timeout waiting for interrupt!\n");
> > + printf(" vCPU: %d\n", vcpu->id);
> > +
> > + TEST_FAIL("vCPU never received IRQ!\n");
>
> Which vCPU? What BDF+MSI? Some of that is available in earlier messages, but
> failure messages should be standalone.
>

Got it, I'll add that info in v3.

> > + }
> > + }
> > +
> > + WRITE_TO_GUEST(vm, guest_received_irq[vcpu->id], false);
> > + }
> > +
> > + WRITE_TO_GUEST(vm, done, true);
> > +
> > + for (i = 0; i < nr_vcpus; i++) {
>
> Unnecessary curly braces.
>

Will remove in v3.

> > + pthread_join(vcpu_threads[i], NULL);
> > + }
> > +
> > + printf("Host interrupts handled:\n");
> > + printf(" IRQ-%d: %lu\n", irq, get_irq_count(irq) - irq_count);
> > + printf(" Posted-interrupt notification events: %lu\n",
> > + get_irq_count_by_name("PIN:") - pin_count);
> > + printf(" Posted-interrupt wakeup events: %lu\n",
> > + get_irq_count_by_name("PIW:") - piw_count);
> > +
> > + vfio_pci_device_cleanup(device);
> > + iommu_cleanup(iommu);
> > +
> > + return 0;
> > +}
> > --
> > 2.53.0.1118.gaef5881109-goog
> >