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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The test now demands up to 144 vCPUs per VM and will assert/crash on hosts with lower KVM max vCPU limits.
- [Low] The `perf_capabilities` parameter is unused in `__test_arch_events`.
- [Low] Confusing and unusual pointer syntax `struct kvm_vcpu **__vcpus[]` is used for an out parameter.
--

--- Patch [2]: [PATCH v3 2/2] KVM: selftests: Create one VM with many vCPUs for each major PMU counters test ---
commit 135d3508be788c9140c4a2eec25c4bc440f48012
Author: Sean Christopherson <seanjc@google.com>

KVM: selftests: Create one VM with many vCPUs for each major PMU counters test

This commit updates the KVM PMU counters test to batch test cases into fewer
VMs by using multiple vCPUs per VM. This significantly reduces the total
runtime spent creating and destroying 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..08bfa82d7ab84 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[])

[Severity: Low]
This isn't a bug, but is the struct kvm_vcpu **__vcpus[] signature
intentional? Mixing double pointers with array brackets in a function
signature can be a bit confusing to read compared to ***__vcpus.

[ ... ]

> @@ -326,30 +339,72 @@ static void guest_test_arch_events(void)
>  	GUEST_DONE();
>  }
>  
> -static void test_arch_events(u8 pmu_version, u64 perf_capabilities,
> -			     u8 length, u32 unavailable_mask)
> +static void __test_arch_events(struct kvm_vcpu *vcpu, u64 perf_capabilities,
> +			       u8 length, u32 unavailable_mask)
>  {

[Severity: Low]
Is the perf_capabilities parameter still needed here? It looks like it is
no longer referenced in the function body since the MSR setup was moved into
pmu_vm_create_with_vcpus().

> -	struct kvm_vcpu *vcpu;
> -	struct kvm_vm *vm;
> -
> -	/* Testing arch events requires a vPMU (there are no negative tests). */
> -	if (!pmu_version)
> -		return;
> -
>  	unavailable_mask &= GENMASK(X86_PROPERTY_PMU_EVENTS_MASK.hi_bit,
>  				    X86_PROPERTY_PMU_EVENTS_MASK.lo_bit);
>  

[ ... ]

> +static void test_arch_events(u8 pmu_version, u64 perf_capabilities)
> +{
> +	struct kvm_vcpu **vcpus;
> +	struct kvm_vm *vm;
> +	int i = 0;
> +	u32 k;
> +	u8 j;
> +

[ ... ]

> +	pr_info("Testing arch events, PMU version %u, perf_caps = %lx\n",
> +		pmu_version, perf_capabilities);
> +
> +	/* Testing arch events requires a vPMU (there are no negative tests). */
> +	if (!pmu_version)
> +		return;
> +
> +	vm = pmu_vm_create_with_vcpus((NR_INTEL_ARCH_EVENTS + 2) * (ARRAY_SIZE(unavailable_masks) - 1),
> +				      guest_test_arch_events, pmu_version,
> +				      perf_capabilities, &vcpus);

[Severity: Medium]
Could this cause the test to assert on hosts with lower vCPU limits?

This calculation requests up to 144 vCPUs per VM ((14 + 2) * (10 - 1)). If the 
host has a KVM_CAP_MAX_VCPUS limit lower than 144 (which is common in nested
virtualization or lightweight VMs), vm_create_with_vcpus() might fail with a
hard assertion.

Should this check against the host limit and either skip via TEST_REQUIRE()
or batch the creations into smaller groups of 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.
> +	 */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804210046.3413149-1-seanjc@google.com?part=2