Re: [PATCH v2 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 3 potential issue(s) to consider:
- [Medium] `test_gp_counters()` sets the GP counters CPUID property to the maximum value in every iteration, ignoring the loop variable `j`.
- [Low] `pmu_vm_create_with_vcpus()` allocates the `vcpus` array using `sizeof(struct kvm_vcpu)` instead of `sizeof(struct kvm_vcpu *)`.
- [Low] `test_arch_events()` allocates and initializes more vCPUs than it actually consumes in its test loops.
--

--- Patch [2]: [PATCH v2 2/2] KVM: selftests: Create one VM with many vCPUs for each major PMU counters test ---
commit a4cedd8a736c776f6a5266a65eb438757b2a50d6
Author: Sean Christopherson <[email protected]>
Subject: KVM: selftests: Create one VM with many vCPUs for each major PMU counters test

To reduce the runtime of the PMU counters test by an order of magnitude,
create one VM per category of test (arch events, GP PMCs, fixed PMCs),
with N vCPUs per VM, where 'N' is the number of testscases to run per
category. The vast majority of the test's runtime is spent creating VMs.

> diff --git a/tools/testing/selftests/kvm/x86/pmu_counters_test.c b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> index 8abf17cc9469d..c7c5662203815 100644
> --- a/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> +++ b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> @@ -87,14 +87,18 @@ static struct kvm_intel_pmu_event intel_event_to_feature(u8 idx)
>  	return __intel_event_to_feature[idx];
>  }
>  
> -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]
Does this calloc() call allocate too much memory?

The vcpus array contains pointers, so shouldn't the element size be
sizeof(struct kvm_vcpu *) instead of sizeof(struct kvm_vcpu)?

[ ... ]
> @@ -326,30 +339,72 @@ static void guest_test_arch_events(void)
[ ... ]
> +static void test_arch_events(u8 pmu_version, u64 perf_capabilities)
> +{
[ ... ]
> +	vm = pmu_vm_create_with_vcpus((NR_INTEL_ARCH_EVENTS + 2) * ARRAY_SIZE(unavailable_masks),
> +				      guest_test_arch_events, pmu_version,
> +				      perf_capabilities, &vcpus);
> +
> +	/*
> +	 * Test single bits for all PMU version and lengths up the number of
> +	 * events +1 (to verify KVM doesn't do weird things if the guest length
> +	 * is greater than the host length).  Explicitly test a mask of '0' and
> +	 * all ones i.e. all events being available and unavailable.
> +	 */
> +	for (j = 0; j <= NR_INTEL_ARCH_EVENTS + 1; j++) {
> +		for (k = 1; k < ARRAY_SIZE(unavailable_masks); k++)
> +			__test_arch_events(vcpus[i++], perf_capabilities, j,
> +					   unavailable_masks[k]);
> +	}

[Severity: Low]
Are we over-allocating vCPUs for this VM?

The VM is created with a vCPU count scaling by ARRAY_SIZE(unavailable_masks),
but the inner loop below it starts at k = 1.

This means the inner loop only executes ARRAY_SIZE - 1 times, leaving some
allocated and initialized vCPUs unused.

[ ... ]
> @@ -493,21 +548,27 @@ static void guest_test_gp_counters(void)
[ ... ]
> +static void test_gp_counters(u8 pmu_version, u64 perf_capabilities)
>  {
> +	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]
Should the third parameter to vcpu_set_cpuid_property() be j instead of
nr_gp_counters here?

Using nr_gp_counters sets the maximum value in every iteration, missing test
coverage for guests configured with fewer than the maximum number of GP
counters.

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