Re: [PATCH 2/2] KVM: selftests: Create one VM with many vCPUs for each major PMU counters test

On Tue, Aug 04, 2026, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] In `test_gp_counters`, the refactoring incorrectly configures every vCPU with the maximum number of GP counters, losing test coverage for intermediate values.
> - [Low] The `vcpus` pointer array is allocated using the size of `struct kvm_vcpu` rather than the size of a pointer.
> --
> 
> --- Patch [2]: [PATCH 2/2] KVM: selftests: Create one VM with many vCPUs for each major PMU counters test ---
> commit f846c14eae6ee2689b438ef9a6cdf2439244aee5
> Author: Sean Christopherson <[email protected]>
> 
> KVM: selftests: Create one VM with many vCPUs for each major PMU counters test
> 
> This commit refactors the PMU counters test to create a single VM per 
> test category (arch events, GP PMCs, fixed PMCs) and provisions multiple
> vCPUs within that VM. This drastically reduces the test runtime by
> avoiding repeated VM creation overhead.
> 
> > diff --git a/tools/testing/selftests/kvm/x86/pmu_counters_test.c b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> > --- a/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> > +++ b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> [ ... ]
> > -static struct kvm_vm *pmu_vm_create_with_one_vcpu(struct kvm_vcpu **vcpu,
> > -						  void *guest_code,
> > -						  u8 pmu_version,
> > -						  u64 perf_capabilities)
> > +static struct kvm_vm *pmu_vm_create_with_vcpus(u32 nr_vcpus, void *guest_code,
> > +					       u8 pmu_version,
> > +					       u64 perf_capabilities,
> > +					       struct kvm_vcpu **__vcpus[])
> >  {
> > +	struct kvm_vcpu **vcpus = calloc(nr_vcpus, sizeof(struct kvm_vcpu));
> 
> [Severity: Low]
> Is this allocating the correct size?

Gotta love Sashiko's "questions" :-)

> It looks like vcpus is an array of pointers, so this might need to be
> sizeof(struct kvm_vcpu *) instead of sizeof(struct kvm_vcpu). While it
> just results in a minor memory over-allocation here, it could be slightly
> confusing.

Ya, I'll change it to:

	struct kvm_vcpu **vcpus = calloc(nr_vcpus, sizeof(*vcpus));

> [ ... ]
> > -static void test_gp_counters(u8 pmu_version, u64 perf_capabilities,
> > -			     u8 nr_gp_counters)
> > +static void test_gp_counters(u8 pmu_version, u64 perf_capabilities)
> >  {
> > -	struct kvm_vcpu *vcpu;
> > +	u8 nr_gp_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
> > +	struct kvm_vcpu **vcpus;
> >  	struct kvm_vm *vm;
> > +	u8 j;
> >  
> > -	vm = pmu_vm_create_with_one_vcpu(&vcpu, guest_test_gp_counters,
> > -					 pmu_version, perf_capabilities);
> > +	pr_info("Testing %u GP counters, PMU version %u, perf_caps = %lx\n",
> > +		nr_gp_counters, pmu_version, perf_capabilities);
> >  
> > -	vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_NR_GP_COUNTERS,
> > -				nr_gp_counters);
> > +	vm = pmu_vm_create_with_vcpus(nr_gp_counters + 1, guest_test_gp_counters,
> > +				      pmu_version, perf_capabilities, &vcpus);
> >  
> > -	run_vcpu(vcpu);
> > +	for (j = 0; j <= nr_gp_counters; j++) {
> > +		vcpu_set_cpuid_property(vcpus[j], X86_PROPERTY_PMU_NR_GP_COUNTERS,
> > +					nr_gp_counters);
> 
> [Severity: Medium]
> Does this inadvertently lose test coverage for the intermediate counter values?
> 
> By setting X86_PROPERTY_PMU_NR_GP_COUNTERS to the loop upper bound 
> (nr_gp_counters) instead of the loop variable (j), we end up configuring
> every vCPU with the maximum number of GP counters. This seems to bypass
> testing edge cases like 0 GP counters entirely. Should this be passing j
> instead of nr_gp_counters?

Doh, yes.  I copied the code out of the helper and missed that it led to a subtle
variable "collision".