Re: [PATCH] selftests: kvm/x86: test if it checks all the bits in the LBR_FMT bit-field

[Like Xu <like.xu.linux@gmail.com>]

On 5/8/2022 7:26 am, Sean Christopherson wrote:
> On Thu, Aug 04, 2022, Like Xu wrote:
>> From: Like Xu <likexu@tencent.com>
>>
>> User space only enable guest LBR feature when the exactly supported
>> LBR format value is initialized to the MSR_IA32_PERF_CAPABILITIES.
>> The input is also invalid if only partially supported bits are set.
>>
>> Note for PEBS feature, the PEBS_FORMAT bit field is the primary concern,
>> thus if the PEBS_FORMAT input is empty, the other bits check about PEBS
>> (like PEBS_TRAP or ARCH_REG) will be ignored.
>>
>> Signed-off-by: Like Xu <likexu@tencent.com>
>> ---
>>   tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
>> index 6ec901dab61e..98483947f921 100644
>> --- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
>> +++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
>> @@ -13,6 +13,7 @@
>>   
>>   #define _GNU_SOURCE /* for program_invocation_short_name */
>>   #include <sys/ioctl.h>
>> +#include <linux/bitmap.h>
>>   
>>   #include "kvm_util.h"
>>   #include "vmx.h"
>> @@ -56,7 +57,7 @@ int main(int argc, char *argv[])
>>   	const struct kvm_cpuid_entry2 *entry_a_0;
>>   	struct kvm_vm *vm;
>>   	struct kvm_vcpu *vcpu;
>> -	int ret;
>> +	int ret, bit;
>>   	union cpuid10_eax eax;
>>   	union perf_capabilities host_cap;
>>   
>> @@ -97,6 +98,12 @@ int main(int argc, char *argv[])
>>   	ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, 0x30);
>>   	TEST_ASSERT(ret == 0, "Bad PERF_CAPABILITIES didn't fail.");
>>   
>> +	/* testcase 4, reject LBR_FMT if only partially supported bits are set */
>> +	for_each_set_bit(bit, (unsigned long *)&host_cap.capabilities, 6) {
>> +		ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, BIT_ULL(bit));
>> +		TEST_ASSERT(ret == 0, "Bad PERF_CAPABILITIES didn't fail.");
>> +	}
> 
> Rather than add another testcase and target only a subset of possible values, what
> about replacing (fixing IMO) testcase #3 with fully exhaustive approach?

Yeah, much better.

> 
> ---
> From: Sean Christopherson <seanjc@google.com>
> Date: Thu, 4 Aug 2022 12:18:15 -0700
> Subject: [PATCH] KVM: selftests: Test all possible "invalid"
>   PERF_CAPABILITIES.LBR_FMT vals
> 
> Test all possible input values to verify that KVM rejects all values
> except the exact host value.  Due to the LBR format affecting the core
> functionality of LBRs, KVM can't emulate "other" formats, so even though
> there are a variety of legal values, KVM should reject anything but an
> exact host match.
> 
> Suggested-by: Like Xu <like.xu.linux@gmail.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   .../selftests/kvm/x86_64/vmx_pmu_caps_test.c    | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
> index 6ec901dab61e..069589c52f41 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
> @@ -59,6 +59,7 @@ int main(int argc, char *argv[])
>   	int ret;
>   	union cpuid10_eax eax;
>   	union perf_capabilities host_cap;
> +	uint64_t val;
> 
>   	host_cap.capabilities = kvm_get_feature_msr(MSR_IA32_PERF_CAPABILITIES);
>   	host_cap.capabilities &= (PMU_CAP_FW_WRITES | PMU_CAP_LBR_FMT);
> @@ -91,11 +92,17 @@ int main(int argc, char *argv[])
>   	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.lbr_format);
>   	ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), (u64)host_cap.lbr_format);
> 
> -	/* testcase 3, check invalid LBR format is rejected */
> -	/* Note, on Arch LBR capable platforms, LBR_FMT in perf capability msr is 0x3f,
> -	 * to avoid the failure, use a true invalid format 0x30 for the test. */
> -	ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, 0x30);
> -	TEST_ASSERT(ret == 0, "Bad PERF_CAPABILITIES didn't fail.");
> +	/*
> +	 * Testcase 3, check that an "invalid" LBR format is rejected.  Only an
> +	 * exact match of the host's format (and 0/disabled) is allowed.
> +	 */
> +	for (val = 1; val <= PMU_CAP_LBR_FMT; val++) {
> +		if (val == (host_cap.capabilities & PMU_CAP_LBR_FMT))

Emm,

		if (val == host_cap.lbr_format)

to save a minor cost of logical operation.

> +			continue;
> +
> +		ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val);
> +		TEST_ASSERT(!ret, "Bad LBR FMT = 0x%lx didn't fail", val);
> +	}
> 
>   	printf("Completed perf capability tests.\n");
>   	kvm_vm_free(vm);
> 
> base-commit: 93472b79715378a2386598d6632c654a2223267b
> --
>