Re: [PATCH 2/2] KVM: selftests: Test the PMU event "Instructions retired"

[Sean Christopherson <seanjc@google.com>]

On Fri, Dec 09, 2022, Aaron Lewis wrote:
> Add testing for the event "Instructions retired" (0xc0) in the PMU
> event filter on both Intel and AMD to ensure that the event doesn't
> count when it is disallowed.  Unlike most of the other events, the
> event "Instructions retired", will be incremented by KVM when an
> instruction is emulated.  Test that this case is being properly handled
> and that KVM doesn't increment the counter when that event is
> disallowed.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  .../kvm/x86_64/pmu_event_filter_test.c        | 157 ++++++++++++------
>  1 file changed, 110 insertions(+), 47 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 2de98fce7edd..81311af9522a 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
> @@ -54,6 +54,21 @@
>  
>  #define AMD_ZEN_BR_RETIRED EVENT(0xc2, 0)
>  
> +
> +/*
> + * "Retired instructions", from Processor Programming Reference
> + * (PPR) for AMD Family 17h Model 01h, Revision B1 Processors,
> + * Preliminary Processor Programming Reference (PPR) for AMD Family
> + * 17h Model 31h, Revision B0 Processors, and Preliminary Processor
> + * Programming Reference (PPR) for AMD Family 19h Model 01h, Revision
> + * B1 Processors Volume 1 of 2.
> + *    			--- and ---
> + * "Instructions retired", from the Intel SDM, volume 3,
> + * "Pre-defined Architectural Performance Events."
> + */
> +
> +#define INST_RETIRED EVENT(0xc0, 0)
> +
>  /*
>   * This event list comprises Intel's eight architectural events plus
>   * AMD's "retired branch instructions" for Zen[123] (and possibly
> @@ -61,7 +76,7 @@
>   */
>  static const uint64_t event_list[] = {
>  	EVENT(0x3c, 0),
> -	EVENT(0xc0, 0),
> +	INST_RETIRED,

There are multiple refactorings thrown into this single patch.  Please break them
out to their own prep patches, bundling everything together makes it way too hard
to identify the actual functional change.

>  	EVENT(0x3c, 1),
>  	EVENT(0x2e, 0x4f),
>  	EVENT(0x2e, 0x41),

...

> @@ -240,14 +285,39 @@ static struct kvm_pmu_event_filter *remove_event(struct kvm_pmu_event_filter *f,
>  	return f;
>  }
>  
> +#define expect_success(r) __expect_success(r, __func__)

I'm all for macros, but in this case I think it's better to just have the callers
pass in __func__ themselves.  There's going to be copy+paste anyways, the few
extra characters is a non-issue.

Alternatively, make the inner helpers macros, though that'll be annoying to read
and maintain.

And somewhat of a nit, instead of "success" vs. "failure", what about "counting"
vs. "not_counting"?  And s/expect/assert?  Without looking at the low level code,
it wasn't clear to me what "failure" meant.  E.g.

	assert_pmc_counting(r, __func__);

	assert_pmc_not_counting(r, __func__);

> +
> +static void __expect_success(struct perf_results r, const char *func) {

Curly brace on its own line for functions.

> +	if (r.br_count != NUM_BRANCHES)
> +		pr_info("%s: Branch instructions retired = %u (expected %u)\n",
> +			func, r.br_count, NUM_BRANCHES);
> +
> +	TEST_ASSERT(r.br_count,
> +		    "Allowed event, branch instructions retired, is not counting.");
> +	TEST_ASSERT(r.ir_count,
> +		    "Allowed event, instructions retired, is not counting.");	
> +} 
> +
> +#define expect_failure(r) __expect_failure(r, __func__)
> +
> +static void __expect_failure(struct perf_results r, const char *func) {
> +	if (r.br_count)
> +		pr_info("%s: Branch instructions retired = %u (expected 0)\n",
> +			func, r.br_count);

This pr_info() seems silly.  If br_count is non-zero, the assert below will fire, no?

> +
> +	TEST_ASSERT(!r.br_count,
> +		    "Disallowed PMU event, branch instructions retired, is counting");

Either make these inner helpers macros so that the assert is guaranteed unique,
or include the function name in the assert mesage.  If __expect_{failure,success}()
is NOT inlined, but the caller is, then it will be mildly annoying to determine
exactly what test failed.

> +	TEST_ASSERT(!r.ir_count,
> +		    "Disallowed PMU event, instructions retired, is counting");
> +}
> +