diff mbox series

[kvm-unit-tests,v4,1/2] x86: Skip perf related tests when platform cannot support

Message ID 20220628093203.73160-1-weijiang.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series [kvm-unit-tests,v4,1/2] x86: Skip perf related tests when platform cannot support | expand

Commit Message

Yang, Weijiang June 28, 2022, 9:32 a.m. UTC 
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.

Opportunistically replace some "printf" with "report_skip" to make
the output log clean.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>

v4:
- Use supported_fn() to make the code nicer. [Sean]
- Replace some of the printf with report_skip to make the results clean. [Sean]
---
 lib/x86/processor.h | 10 ++++++++++
 x86/vmx_tests.c     | 40 +++++++++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 13 deletions(-)


base-commit: ca85dda2671e88d34acfbca6de48a9ab32b1810d

Comments

Sean Christopherson July 7, 2022, 8:26 p.m. UTC | #1 
On Tue, Jun 28, 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.
> 
> Opportunistically replace some "printf" with "report_skip" to make
> the output log clean.

Ooof, these end up dominating the patch.  Can you split them to a separate prep
patch?  Thanks!

> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> 
> v4:
> - Use supported_fn() to make the code nicer. [Sean]
> - Replace some of the printf with report_skip to make the results clean. [Sean]

Put the versioning info below the three dashes so that it doesn't show up in the
final changelog.

> ---

<version info goes here>

>  lib/x86/processor.h | 10 ++++++++++
>  x86/vmx_tests.c     | 40 +++++++++++++++++++++++++++-------------
>  2 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 9a0dad6..7b6ee92 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 cpu_has_perf_global_ctrl(void)
> +{
> +	return pmu_version() > 1;
> +}
> +
>  #endif
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 4d581e7..3a14cb2 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -852,6 +852,10 @@ static bool monitor_supported(void)
>  	return this_cpu_has(X86_FEATURE_MWAIT);
>  }
>  
> +static inline bool pmu_supported(void) {

Curly brace goes on a new line.

> +	return !!pmu_version();
> +}

Why not put this in processor.h?  And maybe call it cpu_has_pmu()?
Yang, Weijiang July 8, 2022, 12:48 a.m. UTC | #2 
On 7/8/2022 4:26 AM, Sean Christopherson wrote:
> On Tue, Jun 28, 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.
>>
>> Opportunistically replace some "printf" with "report_skip" to make
>> the output log clean.
> Ooof, these end up dominating the patch.  Can you split them to a separate prep
> patch?  Thanks!

Welcome back!

Will do it, thanks!

>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>>
>> v4:
>> - Use supported_fn() to make the code nicer. [Sean]
>> - Replace some of the printf with report_skip to make the results clean. [Sean]
> Put the versioning info below the three dashes so that it doesn't show up in the
> final changelog.
>
>> ---
> <version info goes here>
OK.
>
>>   lib/x86/processor.h | 10 ++++++++++
>>   x86/vmx_tests.c     | 40 +++++++++++++++++++++++++++-------------
>>   2 files changed, 37 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
>> index 9a0dad6..7b6ee92 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 cpu_has_perf_global_ctrl(void)
>> +{
>> +	return pmu_version() > 1;
>> +}
>> +
>>   #endif
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index 4d581e7..3a14cb2 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -852,6 +852,10 @@ static bool monitor_supported(void)
>>   	return this_cpu_has(X86_FEATURE_MWAIT);
>>   }
>>   
>> +static inline bool pmu_supported(void) {
> Curly brace goes on a new line.
Sorry for the typo.
>
>> +	return !!pmu_version();
>> +}
> Why not put this in processor.h?  And maybe call it cpu_has_pmu()?
Good suggestion, then it can serve other apps. Thank you!
diff mbox series

Patch

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 9a0dad6..7b6ee92 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 cpu_has_perf_global_ctrl(void)
+{
+	return pmu_version() > 1;
+}
+
 #endif
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 4d581e7..3a14cb2 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -852,6 +852,10 @@  static bool monitor_supported(void)
 	return this_cpu_has(X86_FEATURE_MWAIT);
 }
 
+static inline bool pmu_supported(void) {
+	return !!pmu_version();
+}
+
 struct insn_table {
 	const char *name;
 	u32 flag;
@@ -881,7 +885,7 @@  static struct insn_table insn_table[] = {
 	{"INVLPG", CPU_INVLPG, insn_invlpg, INSN_CPU0, 14,
 		0x12345678, 0, FIELD_EXIT_QUAL},
 	{"MWAIT", CPU_MWAIT, insn_mwait, INSN_CPU0, 36, 0, 0, 0, &monitor_supported},
-	{"RDPMC", CPU_RDPMC, insn_rdpmc, INSN_CPU0, 15, 0, 0, 0},
+	{"RDPMC", CPU_RDPMC, insn_rdpmc, INSN_CPU0, 15, 0, 0, 0, &pmu_supported},
 	{"RDTSC", CPU_RDTSC, insn_rdtsc, INSN_CPU0, 16, 0, 0, 0},
 	{"CR3 load", CPU_CR3_LOAD, insn_cr3_load, INSN_CPU0, 28, 0x3, 0,
 		FIELD_EXIT_QUAL},
@@ -4107,7 +4111,7 @@  static void test_vpid(void)
 	int i;
 
 	if (!is_vpid_supported()) {
-		printf("Secondary controls and/or VPID not supported\n");
+		report_skip("Secondary controls and/or VPID not supported\n");
 		return;
 	}
 
@@ -4614,7 +4618,7 @@  static void test_nmi_ctrls(void)
 
 	if ((ctrl_pin_rev.clr & (PIN_NMI | PIN_VIRT_NMI)) !=
 	    (PIN_NMI | PIN_VIRT_NMI)) {
-		printf("NMI exiting and Virtual NMIs are not supported !\n");
+		report_skip("NMI exiting and Virtual NMIs are not supported !\n");
 		return;
 	}
 
@@ -4724,7 +4728,7 @@  static void test_ept_eptp(void)
 
 	if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) &&
 	    (ctrl_cpu_rev[1].clr & CPU_EPT))) {
-		printf("\"CPU secondary\" and/or \"enable EPT\" execution controls are not supported !\n");
+		report_skip("\"CPU secondary\" and/or \"enable EPT\" execution controls are not supported !\n");
 		return;
 	}
 
@@ -4884,7 +4888,7 @@  static void test_pml(void)
 
 	if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) &&
 	    (ctrl_cpu_rev[1].clr & CPU_EPT) && (ctrl_cpu_rev[1].clr & CPU_PML))) {
-		printf("\"Secondary execution\" control or \"enable EPT\" control or \"enable PML\" control is not supported !\n");
+		report_skip("\"Secondary execution\" control or \"enable EPT\" control or \"enable PML\" control is not supported !\n");
 		return;
 	}
 
@@ -4936,7 +4940,7 @@  static void test_vmx_preemption_timer(void)
 
 	if (!((ctrl_exit_rev.clr & EXI_SAVE_PREEMPT) ||
 	    (ctrl_pin_rev.clr & PIN_PREEMPT))) {
-		printf("\"Save-VMX-preemption-timer\" control and/or \"Enable-VMX-preemption-timer\" control is not supported\n");
+		report_skip("\"Save-VMX-preemption-timer\" control and/or \"Enable-VMX-preemption-timer\" control is not supported\n");
 		return;
 	}
 
@@ -7175,7 +7179,7 @@  static void test_efer(u32 fld, const char * fld_name, u32 ctrl_fld,
 		efer_reserved_bits &= ~EFER_NX;
 
 	if (!ctrl_bit1) {
-		printf("\"Load-IA32-EFER\" exit control not supported\n");
+		report_skip("\"Load-IA32-EFER\" exit control not supported\n");
 		goto test_entry_exit_mode;
 	}
 
@@ -7258,7 +7262,7 @@  static void test_host_efer(void)
 static void test_guest_efer(void)
 {
 	if (!(ctrl_enter_rev.clr & ENT_LOAD_EFER)) {
-		printf("\"Load-IA32-EFER\" entry control not supported\n");
+		report_skip("\"Load-IA32-EFER\" entry control not supported\n");
 		return;
 	}
 
@@ -7349,7 +7353,7 @@  static void test_load_host_pat(void)
 	 * "load IA32_PAT" VM-exit control
 	 */
 	if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT)) {
-		printf("\"Load-IA32-PAT\" exit control not supported\n");
+		report_skip("\"Load-IA32-PAT\" exit control not supported\n");
 		return;
 	}
 
@@ -7490,8 +7494,13 @@  static void test_perf_global_ctrl(u32 nr, const char *name, u32 ctrl_nr,
 
 static void test_load_host_perf_global_ctrl(void)
 {
+	if (!cpu_has_perf_global_ctrl()) {
+		report_skip("IA32_PERF_GLOBAL_CTRL not supported\n");
+		return;
+	}
+
 	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
-		printf("\"load IA32_PERF_GLOBAL_CTRL\" exit control not supported\n");
+		report_skip("\"load IA32_PERF_GLOBAL_CTRL\" exit control not supported\n");
 		return;
 	}
 
@@ -7502,8 +7511,13 @@  static void test_load_host_perf_global_ctrl(void)
 
 static void test_load_guest_perf_global_ctrl(void)
 {
+	if (!cpu_has_perf_global_ctrl()) {
+		report_skip("IA32_PERF_GLOBAL_CTRL not supported\n");
+		return;
+	}
+
 	if (!(ctrl_enter_rev.clr & ENT_LOAD_PERF)) {
-		printf("\"load IA32_PERF_GLOBAL_CTRL\" entry control not supported\n");
+		report_skip("\"load IA32_PERF_GLOBAL_CTRL\" entry control not supported\n");
 		return;
 	}
 
@@ -7809,7 +7823,7 @@  static void test_load_guest_pat(void)
 	 * "load IA32_PAT" VM-entry control
 	 */
 	if (!(ctrl_enter_rev.clr & ENT_LOAD_PAT)) {
-		printf("\"Load-IA32-PAT\" entry control not supported\n");
+		report_skip("\"Load-IA32-PAT\" entry control not supported\n");
 		return;
 	}
 
@@ -7833,7 +7847,7 @@  static void test_load_guest_bndcfgs(void)
 	u64 bndcfgs;
 
 	if (!(ctrl_enter_rev.clr & ENT_LOAD_BNDCFGS)) {
-		printf("\"Load-IA32-BNDCFGS\" entry control not supported\n");
+		report_skip("\"Load-IA32-BNDCFGS\" entry control not supported\n");
 		return;
 	}