Re: [PATCH v3 2/3] x86: Skip perf related tests when platform cannot support

[Sean Christopherson <seanjc@google.com>]

On Fri, Jun 24, 2022, Yang Weijiang wrote:
> Add helpers to check whether MSR_CORE_PERF_GLOBAL_CTRL and rdpmc
> are supported in KVM. When pmu is disabled with enable_pmu=0,
> reading MSR_CORE_PERF_GLOBAL_CTRL or executing rdpmc leads to #GP,
> so skip related tests in this case to avoid test failure.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  lib/x86/processor.h | 10 ++++++++++
>  x86/vmx_tests.c     | 18 ++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 9a0dad6..70b9193 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -690,4 +690,14 @@ static inline bool cpuid_osxsave(void)
>  	return cpuid(1).c & (1 << (X86_FEATURE_OSXSAVE % 32));
>  }
>  
> +static inline u8 pmu_version(void)
> +{
> +	return cpuid(10).a & 0xff;
> +}
> +
> +static inline bool has_perf_global_ctrl(void)

Slight preference for this_cpu_has_perf_global_ctrl() or cpu_has_perf_global_ctrl().

> +{
> +	return pmu_version() > 1;
> +}
> +
>  #endif
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 4d581e7..3cf0776 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -944,6 +944,14 @@ static void insn_intercept_main(void)
>  			continue;
>  		}
>  
> +		if (insn_table[cur_insn].flag == CPU_RDPMC) {
> +			if (!!pmu_version()) {
> +				printf("\tFeature required for %s is not supported.\n",
> +				       insn_table[cur_insn].name);
> +				continue;
> +			}
> +		}

There's no need to copy+paste a bunch of code plus a one-off check, just add
another helper that plays nice with supported_fn().

static inline bool this_cpu_has_pmu(void)
{
	return !!pmu_version();
}

> +
>  		if (insn_table[cur_insn].disabled) {
>  			printf("\tFeature required for %s is not supported.\n",
>  			       insn_table[cur_insn].name);
> @@ -7490,6 +7498,11 @@ static void test_perf_global_ctrl(u32 nr, const char *name, u32 ctrl_nr,
>  
>  static void test_load_host_perf_global_ctrl(void)
>  {
> +	if (!has_perf_global_ctrl()) {
> +		report_skip("test_load_host_perf_global_ctrl");

If you're going to print just the function name, then

		report_skip(__func__);

will suffice.  I'd still prefer a more helpful message, especially since there's
another "skip" in this function.

> +		return;
> +	}
> +
>  	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
>  		printf("\"load IA32_PERF_GLOBAL_CTRL\" exit control not supported\n");
>  		return;

Speaking of said skip, can you clean up the existing code to use report_skip()?

> @@ -7502,6 +7515,11 @@ static void test_load_host_perf_global_ctrl(void)
>  
>  static void test_load_guest_perf_global_ctrl(void)
>  {
> +	if (!has_perf_global_ctrl()) {
> +		report_skip("test_load_guest_perf_global_ctrl");
> +		return;
> +	}
> +
>  	if (!(ctrl_enter_rev.clr & ENT_LOAD_PERF)) {
>  		printf("\"load IA32_PERF_GLOBAL_CTRL\" entry control not supported\n");
>  		return;
> -- 
> 2.27.0
>