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

[Josh Hilke <jrhilke@google.com>]

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