Re: [PATCH v4 21/21] KVM: selftests: Support fixed counters bitmap in pmu_counters_test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] KVM: selftests: missing EDX/ECX combinations in
  test_fixed_counters()
--

commit 3d8b2ca900bb2a01f5f9cee15fc9a12bc40184ad
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

KVM: selftests: Support fixed counters bitmap in pmu_counters_test

This commit updates this_pmu_has() to check the guest PMU version before
consulting CPUID.0AH:ECX and updates test_fixed_counters() to test
supported fixed counter bitmasks alongside CPUID.0AH:EDX[4:0].

> diff --git a/tools/testing/selftests/kvm/x86/pmu_counters_test.c b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> index c3e784e16348c..61e6fe5b6ef48 100644
> --- a/tools/testing/selftests/kvm/x86/pmu_counters_test.c
> +++ b/tools/testing/selftests/kvm/x86/pmu_counters_test.c

[ ... ]

> @@ -631,25 +632,43 @@ static void __test_fixed_counters(struct kvm_vcpu *vcpu, u8 nr_fixed_counters,
>  static void test_fixed_counters(u8 pmu_version, u64 perf_capabilities)
>  {
>  	u8 nr_fixed_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
> +	unsigned long fixed_subset;
>  	struct kvm_vcpu **vcpus;
>  	struct kvm_vm *vm;
> +	u32 fixed_bitmap;
>  	int i = 0;
> -	u32 k;
>  	u8 j;
>  
>  	pr_info("Testing %u fixed counters, PMU version %u, perf_caps = %lx\n",
>  		nr_fixed_counters, pmu_version, perf_capabilities);
>  
> +	fixed_bitmap = BIT(nr_fixed_counters) - 1;
> +	if (pmu_version >= 5)
> +		fixed_bitmap |= kvm_cpu_property(X86_PROPERTY_PMU_FIXED_COUNTERS_BITMASK);
>  
> -	vm = pmu_vm_create_with_vcpus((nr_fixed_counters + 1) * BIT(nr_fixed_counters),
> +	/* One vCPU per subset of fixed_bitmap, plus one per legacy EDX test. */
> +	vm = pmu_vm_create_with_vcpus(BIT(__builtin_popcount(fixed_bitmap)) + nr_fixed_counters,
>  				      guest_test_fixed_counters,
>  				      pmu_version, perf_capabilities, &vcpus);
>  
> -	for (j = 0; j <= nr_fixed_counters; j++) {
> -		for (k = 0; k <= (BIT(nr_fixed_counters) - 1); k++)
> -			__test_fixed_counters(vcpus[i++], j, k);
> +	for (fixed_subset = 0; fixed_subset <= fixed_bitmap; fixed_subset++) {
> +		u32 nr_contiguous;
> +
> +		/*
> +		 * The loop walks all values from 0 to fixed_bitmap, so skip any
> +		 * value that is not a subset of fixed_bitmap.
> +		 */
> +		if (fixed_subset & ~fixed_bitmap)
> +			continue;
> +
> +		nr_contiguous = find_first_zero_bit(&fixed_subset,
> +						    MAX_NR_FIXED_COUNTERS);
> +		__test_fixed_counters(vcpus[i++], nr_contiguous, fixed_subset);
>  	}
>  
> +	for (j = 1; j <= nr_fixed_counters; j++)
> +		__test_fixed_counters(vcpus[i++], j, 0);
> +
>  	pmu_vm_free(vm, vcpus);
>  }

[Severity: Medium]
Does deriving nr_contiguous from fixed_subset drop test coverage for fixed
counters enabled solely via CPUID.0AH:ECX when EDX[4:0] is 0?

Whenever bit 0 of fixed_subset is set, find_first_zero_bit() returns at
least 1, and the second loop only tests supported_bitmask == 0 with j >= 1.
As a result, fixed counter 0 (and any contiguous prefix of fixed_subset
starting at bit 0) is never tested with nr_fixed_counters == 0 and ECX[i]
set, nor does a guest with pmu_version < 5 get tested with EDX[4:0] == 0
and a non-zero contiguous ECX mask to verify that KVM ignores ECX.

In addition, gating X86_PROPERTY_PMU_FIXED_COUNTERS_BITMASK on
pmu_version >= 5 skips non-contiguous host ECX bits when testing guest
pmu_version < 5, so the test never verifies that KVM ignores those bits on
older PMU versions.  Keeping the outer loop over j in 0..nr_fixed_counters
and the inner loop over subsets of an unconditionally constructed
fixed_bitmap would cover all (EDX[4:0], ECX) combinations across all PMU
versions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112203227.289379-1-zide.chen@intel.com?part=21