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

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?

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.

[ ... ]
> -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?

>  
> -	kvm_vm_free(vm);
> +		run_vcpu(vcpus[j]);
> +	}
> +
> +	pmu_vm_free(vm, vcpus);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2