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

[Like Xu]

On 5/8/2022 7:26 am, Sean Christopherson wrote:
> On Thu, Aug 04, 2022, Like Xu wrote:
> > From: Like Xu <likexu@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>   .../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
> --