Re: [PATCH 2/2] KVM: selftests: Add a test for vfio-pci device IRQ delivery to vCPUs

[Sean Christopherson <seanjc@google.com>]

On Mon, Oct 27, 2025, David Matlack wrote:
> On Mon, Oct 27, 2025 at 9:52 AM Sean Christopherson <seanjc@google.com> wrote:
> 
> Thank you for the feedback!
> 
> > On Fri, Sep 12, 2025, David Matlack wrote:
> > >
> > > This test only supports x86_64 for now, but can be ported to other
> > > architectures in the future.
> >
> > Can it though?  There are bits and pieces that can be reused, but this test is
> > x86 through and through.
> 
> Delivering MSIs from a vfio-pci device into a guest is not an
> x86-specific thing. But I think you could make the argument that it
> will be simpler for each architecture to have their own version of
> this test, with some shared code, rather than the other way around.

Right, the concept is x86, but the code as written is very x86 centric.

> > > +static void guest_irq_handler(struct ex_regs *regs)
> > > +{
> > > +     WRITE_ONCE(guest_received_irq[guest_get_vcpu_id()], true);
> >
> > Hmm, using APID ID works, but I don't like the hidden dependency on the library
> > using ascending IDs starting from '0'.  This would also be a good opportunity to
> > improve the core infrastructure.
> 
> What dependency are you referring to? I think the only requirement is
> vcpu->id == APIC ID

Yep, this one.

> Are you saying tests should not make that assumption?

Ya, ideally, they would not.

> > > +     WRITE_ONCE(vcpu_tids[vcpu->id], syscall(__NR_gettid));
> >
> > Please add wrapper in tools/testing/selftests/kvm/include/kvm_syscalls.h.
> 
> Will do.
> 
> > > +static void pin_vcpu_threads(int nr_vcpus, int start_cpu, cpu_set_t *available_cpus)
> > > +{
> > > +     const size_t size = sizeof(cpu_set_t);
> > > +     int nr_cpus, cpu, vcpu_index = 0;
> > > +     cpu_set_t target_cpu;
> > > +     int r;
> > > +
> > > +     nr_cpus = get_nprocs();
> >
> > Generally speaking, KVM selftests try to avoid affining tasks to CPUs that are
> > outside of the original affinity list.  See various usage of sched_getaffinity().
> 
> available_cpus is initialized by calling sched_getaffinity().

Ah, I missed that in the for-loop.
 
> > > +static FILE *open_proc_interrupts(void)
> > > +{
> > > +     FILE *fp;
> > > +
> > > +     fp = fopen("/proc/interrupts", "r");
> > > +     TEST_ASSERT(fp, "fopen(/proc/interrupts) failed");
> >
> > open_path_or_exit()?
> 
> I guess I'll have to rework this code to use fds instead of FILE?

Hmm, not necessarily.  E.g. maybe open_file_or_exit()?  I don't have a strong
preference (and have put zero thought into what would work well).
 
> > > +int main(int argc, char **argv)
> > > +{
> > > +     /* Random non-reserved vector and GSI to use for the device IRQ */
> > > +     const u8 vector = 0xe0;
> >
> > s/random/arbitrary
> >
> > Why not make it truly random?
> 
> Only because there's already a lot going on in this test. Do you think
> it's worth randomizing these?

Probably?  But without a command line option, to keep this somewhat less crazy?

> > > +     irq_count = get_irq_count(irq);
> > > +     pin_count = __get_irq_count("PIN:");
> > > +     piw_count = __get_irq_count("PIW:");
> >
> > This is obviously very Intel specific information.  If you're going to print the
> > posted IRQ info, then the test should also print e.g. AMD GALogIntr events.
> 
> I saw PIN and PIW in /proc/interrupts when I tested on AMD hosts,

The kernel really should key off CONFIG_KVM_INTEL={m,y}, not CONFIG_KVM (though
in practice it doesn't matter all that much).

> that's why I included them both by default.

I think it makes sense to only print PIN+PIW on Intel, otherwise it's pure noise.

> I can look into adding GALogIntr if you want.

PIN is midly interesting.  PIW and GALogIntr are much more interesting from a
coverage perspective.  E.g. if the deltas for PIW and GALogIntr are '0', then
the test likely isn't exercising the blocking path.

> > > +     /* Set a consistent seed so that test are repeatable. */
> > > +     srand(0);
> >
> > We should really figure out a solution for reproducible random numbers in the
> > host.  Ah, and kvm_selftest_init()'s handling of guest random seeds is flawed,
> > because it does random() without srand() and so AFAICT, gets the same seed every
> > time.  E.g. seems like we want something like this, but with a way to override
> > "random_seed" from a test.
> >
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 5744643d9ec3..0118fd2ba56b 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -2310,6 +2310,7 @@ void __attribute((constructor)) kvm_selftest_init(void)
> >         struct sigaction sig_sa = {
> >                 .sa_handler = report_unexpected_signal,
> >         };
> > +       int random_seed;
> >
> >         /* Tell stdout not to buffer its content. */
> >         setbuf(stdout, NULL);
> > @@ -2319,8 +2320,13 @@ void __attribute((constructor)) kvm_selftest_init(void)
> >         sigaction(SIGILL, &sig_sa, NULL);
> >         sigaction(SIGFPE, &sig_sa, NULL);
> >
> > +       random_seed = time(NULL);
> > +       srand(random_seed);
> > +
> >         guest_random_seed = last_guest_seed = random();
> > -       pr_info("Random seed: 0x%x\n", guest_random_seed);
> > +
> > +       pr_info("Guest random seed: 0x%x (srand: 0x%x)\n",
> > +               guest_random_seed, random_seed);
> >
> >         kvm_selftest_arch_init();
> >  }
> 
> Just to make sure I understand: You are proposing using the current
> time as the seed

Or anything that's somewhat random.  I don't know what glibc is doing under the
hood, so it's entirely possible/likely there's a much better method.

> and printing it to the console. That way each run uses a different random
> seed and we get broader test coverage. Then if someone wants to reproduce a
> test result, there would be some way for them to override the seed via
> cmdline? That sounds reasonable to me, I can take a look at adding that in
> the next version.

Yep.  That was the intent with the guest_random_seed, I just didn't implement it
very well :-)

Regarding reproducibility, one idea would be to have kvm_selftest_init() pull the
seed from an environment variable, e.g. so that reproducing with a specific seed
doesn't require hacking or test support.  I'm not entirely sure I like the idea
of using environment variables though...