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:26:56 -0700
In-Reply-To: <20230607123700.40229-3-cloudliang@tencent.com>
Mime-Version: 1.0
References: <20230607123700.40229-1-cloudliang@tencent.com> <20230607123700.40229-3-cloudliang@tencent.com>
Message-ID: <ZJyloBGvASSXldCy@google.com>
Subject: Re: [PATCH v3 2/4] KVM: selftests: Test unavailable event filters are rejected
From:   Sean Christopherson <seanjc@google.com>
To:     Jinrong Liang <ljr.kernel@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>,
        Shuah Khan <shuah@kernel.org>,
        Aaron Lewis <aaronlewis@google.com>,
        David Matlack <dmatlack@google.com>,
        Vishal Annapurve <vannapurve@google.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 Wed, Jun 07, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> This commit adds test cases for unsupported input values in the

Avoid "this commit" and "this patch", simply state what the patch does as a
command, e.g. "Add test cases for ...".

> PMU event filter. The tests cover unsupported "action" values,
> unsupported "flags" values, and unsupported "nevents" values.
> All these cases should return an error, as they are currently
> not supported by the filter. Additionally, the patch tests setting
> non-exist fixed counters in the fixed bitmap doesn't fail.
> 
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../kvm/x86_64/pmu_event_filter_test.c        | 48 +++++++++++++++++--
>  1 file changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> index 26f674c32cde..7555e0f4290c 100644
> --- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> @@ -11,9 +11,7 @@
>   */
>  
>  #define _GNU_SOURCE /* for program_invocation_short_name */
> -#include "test_util.h"
> -#include "kvm_util.h"
> -#include "processor.h"
> +#include "pmu.h"
>  
>  /*
>   * In lieu of copying perf_event.h into tools...
> @@ -32,6 +30,10 @@
>  #define MAX_FILTER_EVENTS	300
>  #define MAX_TEST_EVENTS	10
>  
> +#define PMU_EVENT_FILTER_INVALID_ACTION		(KVM_PMU_EVENT_DENY + 1)
> +#define PMU_EVENT_FILTER_INVALID_FLAGS			(KVM_PMU_EVENT_FLAG_MASKED_EVENTS + 1)
> +#define PMU_EVENT_FILTER_INVALID_NEVENTS		(MAX_FILTER_EVENTS + 1)
> +
>  /*
>   * This is how the event selector and unit mask are stored in an AMD
>   * core performance event-select register. Intel's format is similar,
> @@ -762,6 +764,7 @@ static void test_filter_ioctl(struct kvm_vcpu *vcpu)
>  {
>  	uint64_t e = ~0ul;
>  	int r;
> +	struct __kvm_pmu_event_filter f;

Reverse xmas tree.

>  
>  	/*
>  	 * Unfortunately having invalid bits set in event data is expected to
> @@ -780,6 +783,45 @@ static void test_filter_ioctl(struct kvm_vcpu *vcpu)
>  					KVM_PMU_EVENT_FLAG_MASKED_EVENTS,
>  					KVM_PMU_EVENT_ALLOW);
>  	TEST_ASSERT(r == 0, "Valid PMU Event Filter is failing");
> +
> +	/*
> +	 * Testing unsupported "action" input values should return an error.

Omit the "Testing", KVM's behavior isn't specific to "testing", any unsupported
action should fail.

	/* Unsupported actions should be rejected by KVM. */

Though honestly, I would forego the comments entirely, the macro name plus the
assert make it quite clear what's being tested.

> +	 * Currently, only values 0 or 1 are supported.

Drop this part of the comment, it will become stale if PMU_EVENT_FILTER_INVALID_ACTION
is modified, and readers can look at the definition of PMU_EVENT_FILTER_INVALID_ACTION
if they really care about the actual value.

> +	 */
> +	f = base_event_filter;
> +	f.action = PMU_EVENT_FILTER_INVALID_ACTION;
> +	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
> +	TEST_ASSERT(r != 0, "Set invalid action is expected to fail.");

Ignore the bad precedent set by this test, the preferred way to check for 0 and
!0 is TEST_ASSERT(r, ...) and TEST_ASSERT(!r, ...);

And no punctuation in the assert, i.e. drop the period.

> +
> +	/*
> +	 * Testing unsupported "flags" input values should return an error.
> +	 * Currently, only values 0 or 1 are supported.
> +	 */

Same here.

> +	f = base_event_filter;
> +	f.flags = PMU_EVENT_FILTER_INVALID_FLAGS;
> +	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
> +	TEST_ASSERT(r != 0, "Set invalid flags is expected to fail.");
> +
> +	/*
> +	 * Testing unsupported "nevents" input values should return an error.
> +	 * Currently, only values less than or equal to
> +	 * MAX_FILTER_EVENTS are supported.

And here.

> +	 */
> +	f = base_event_filter;
> +	f.nevents = PMU_EVENT_FILTER_INVALID_NEVENTS;
> +	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
> +	TEST_ASSERT(r != 0,
> +		    "Setting PMU event filters that exceeds the maximum supported value should fail");

To avoid splitting lines,

	TEST_ASSERT(r, "Exceeding the max number of filter events should fail");
> +
> +	/*
> +	 * In this case, setting non-exist fixed counters in the fixed bitmap
> +	 * doesn't fail.
> +	 */

And here.

> +	f = base_event_filter;
> +	f.fixed_counter_bitmap = ~GENMASK_ULL(X86_INTEL_MAX_FIXED_CTR_NUM, 0);
> +	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
> +	TEST_ASSERT(r == 0,
> +		    "Setting invalid or non-exist fixed cunters in the fixed bitmap fail.");

Something like so to avoid multiple lines.

	TEST_ASSERT(!r, "Masking non-existent fixed counters should be allowed");