Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Jun 2023 14:55:30 -0700
In-Reply-To: <20230608113420.14695-3-cloudliang@tencent.com>
Mime-Version: 1.0
References: <20230608113420.14695-1-cloudliang@tencent.com> <20230608113420.14695-3-cloudliang@tencent.com>
Message-ID: <ZJysUp5Ndnecok4S@google.com>
Subject: Re: [PATCH 2/2] KVM: selftests: Add PEBS test for MSR_IA32_PERF_CAPABILITIES
From:   Sean Christopherson <seanjc@google.com>
To:     Jinrong Liang <ljr.kernel@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Like Xu <like.xu.linux@gmail.com>,
        Jinrong Liang <cloudliang@tencent.com>,
        linux-kselftest@vger.kernel.org, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Thu, Jun 08, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> This commit adds a PEBS test that verifies all possible combinations
> of PEBS-related bits in MSR_IA32_PERF_CAPABILITIES. This comprehensive
> test ensures the accuracy of the PEBS feature.
> 
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../selftests/kvm/x86_64/vmx_pmu_caps_test.c  | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> 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 02903084598f..c1b1ba44bc26 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
> @@ -21,6 +21,12 @@
>  
>  #define MAX_LINEAR_ADDR_MASK		GENMASK_ULL(15, 8)
>  #define ADDR_OFS_BIT			8
> +#define PMU_CAP_LBR_FMT		0x3f
> +#define PMU_CAP_SMM_FREEZE		BIT_ULL(12)
> +#define PMU_CAP_FW_WRITES		BIT_ULL(13)
> +#define PMU_CAP_PERF_METRICS_AVAILABLE	BIT_ULL(PERF_CAP_METRICS_IDX)
> +#define PMU_CAP_PEBS_OUTPUT_PT_AVAIL	BIT_ULL(PERF_CAP_PT_IDX)
> +#define PMU_CAP_PEBS_ALL		(PERF_CAP_PEBS_MASK | PMU_CAP_PEBS_OUTPUT_PT_AVAIL)
>  
>  union perf_capabilities {
>  	struct {
> @@ -331,6 +337,70 @@ static void test_ds_area_noncanonical_address(union perf_capabilities host_cap)
>  	kvm_vm_free(vm);
>  }
>  
> +static void test_pebs_bit_combinations(union perf_capabilities host_cap)
> +{
> +	int ret;

Reverse xmas tree.

> +	uint64_t pebs_val, val;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm = vm_create_with_one_vcpu(&vcpu, NULL);

It's kinda silly, but I think it makes sense to wait until after all of the
TEST_REQUIRE()s to create the VM+vCPU.

> +
> +	TEST_REQUIRE(kvm_cpu_property(X86_PROPERTY_PMU_VERSION) > 1);
> +	TEST_REQUIRE(host_cap.capabilities & PERF_CAP_PEBS_FORMAT);
> +	TEST_REQUIRE(vcpu_get_msr(vcpu, MSR_IA32_MISC_ENABLE) &
> +		     MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL);
> +
> +	/*
> +	 * Test if PEBS_REC_FMT is set and the value is the same as host,
> +	 * the other PEBS bits are allowed to be set only if they are the
> +	 * same as host.
> +	 */
> +	pebs_val = host_cap.capabilities & PMU_CAP_PEBS_ALL;
> +
> +	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, pebs_val);
> +	ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES),
> +		  (u64)pebs_val);

This cast shouldn't be necessary.  And if you're going to split lines...

	ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES),
		  host_cap.capabilities & PMU_CAP_PEBS_ALL);

Though isn't that flawed?  E.g. will fail if MSR_IA32_PERF_CAPABILITIES has
non-PEBS bits set.  I think what you want is something like:

	guest_perf_caps = vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES);

	ASSERT_EQ(guest_perf_caps & PMU_CAP_PEBS_ALL,
		  host_cap.capabilities & PMU_CAP_PEBS_ALL);

> +
> +	/* Test all PEBS bit combinations. */
> +	for (val = 0x0; val <= (~0ul & PMU_CAP_PEBS_ALL); val++) {
> +		/* Skips values that are not related to PEBS. */
> +		if (val & (PMU_CAP_LBR_FMT | PMU_CAP_SMM_FREEZE |
> +		    PMU_CAP_FW_WRITES | PMU_CAP_PERF_METRICS_AVAILABLE))

Align things by their scope, i.e.

		if (val & (PMU_CAP_LBR_FMT | PMU_CAP_SMM_FREEZE
			   PMU_CAP_FW_WRITES | PMU_CAP_PERF_METRICS_AVAILABLE))

But even better would be to look for !PEBS, not some other values where it's not
clear they exhaustively cover all !PEBS value.  E.g. can't this be?

		if (val & ~PMU_CAP_PEBS_ALL)
			continue;

> +			continue;
> +
> +		/*
> +		 * Test that value of PEBS is rejected when the KVM doesn't

Just "KVM", not "the KVM".

> +		 * supports Intel PT.
> +		 */
> +		if ((val & PMU_CAP_PEBS_OUTPUT_PT_AVAIL) &&
> +		    (!(host_cap.capabilities & PMU_CAP_PEBS_OUTPUT_PT_AVAIL))) {
> +			ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val);
> +			TEST_ASSERT(!ret, "Bad PEBS auxiliary bits = 0x%lx didn't fail", val);
> +
> +			continue;
> +		}
> +
> +		/*
> +		 * Test that value of PEBS is rejected when carrying

I don't quite follow what you mean by "carrying".  Do you mean a non-zero value?

> +		 * PEBS_REC_FMT if the value of PEBS is not equal to host.
> +		 */
> +		if ((val & PERF_CAP_PEBS_FORMAT) && val != pebs_val) {
> +			ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val);
> +			TEST_ASSERT(!ret, "Bad PEBS auxiliary bits = 0x%lx didn't fail", val);
> +
> +			continue;
> +		}
> +
> +		/*
> +		 * Test that PEBS bits can be written simultaneously or
> +		 * independently if PEBS_REC_FMT is not carried.
> +		 */
> +		vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val);
> +		ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), val);
> +	}
> +
> +	kvm_vm_free(vm);
> +}