All of lore.kernel.org
 help / color / mirror / Atom feed
* [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support
@ 2022-06-28  9:32 Yang Weijiang
  2022-06-28  9:32 ` [kvm-unit-tests PATCH v4 2/2] x86: Check platform vPMU capabilities before run lbr tests Yang Weijiang
  2022-07-07 20:26 ` [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support Sean Christopherson
  0 siblings, 2 replies; 6+ messages in thread
From: Yang Weijiang @ 2022-06-28  9:32 UTC (permalink / raw)
  To: pbonzini, seanjc; +Cc: kvm, Yang Weijiang

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(-)

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;
 	}
 

base-commit: ca85dda2671e88d34acfbca6de48a9ab32b1810d
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [kvm-unit-tests PATCH v4 2/2] x86: Check platform vPMU capabilities before run lbr tests
  2022-06-28  9:32 [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support Yang Weijiang
@ 2022-06-28  9:32 ` Yang Weijiang
  2022-07-07 20:28   ` Sean Christopherson
  2022-07-07 20:26 ` [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support Sean Christopherson
  1 sibling, 1 reply; 6+ messages in thread
From: Yang Weijiang @ 2022-06-28  9:32 UTC (permalink / raw)
  To: pbonzini, seanjc; +Cc: kvm, Yang Weijiang

Use new helper to check whether pmu is available and Perfmon/Debug
capbilities are supported before read MSR_IA32_PERF_CAPABILITIES to
avoid test failure. The issue can be captured when enable_pmu=0.

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

v4:
- Put the X86_FEATURE_PDCM to the right place. [Sean]
---
 lib/x86/processor.h |  1 +
 x86/pmu_lbr.c       | 32 +++++++++++++-------------------
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 7b6ee92..7a35c7f 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -146,6 +146,7 @@ static inline bool is_intel(void)
  */
 #define	X86_FEATURE_MWAIT		(CPUID(0x1, 0, ECX, 3))
 #define	X86_FEATURE_VMX			(CPUID(0x1, 0, ECX, 5))
+#define	X86_FEATURE_PDCM		(CPUID(0x1, 0, ECX, 15))
 #define	X86_FEATURE_PCID		(CPUID(0x1, 0, ECX, 17))
 #define	X86_FEATURE_MOVBE		(CPUID(0x1, 0, ECX, 22))
 #define	X86_FEATURE_TSC_DEADLINE_TIMER	(CPUID(0x1, 0, ECX, 24))
diff --git a/x86/pmu_lbr.c b/x86/pmu_lbr.c
index 688634d..497df1e 100644
--- a/x86/pmu_lbr.c
+++ b/x86/pmu_lbr.c
@@ -15,6 +15,7 @@
 #define MSR_LBR_SELECT		0x000001c8
 
 volatile int count;
+u32 lbr_from, lbr_to;
 
 static noinline int compute_flag(int i)
 {
@@ -38,18 +39,6 @@ static noinline int lbr_test(void)
 	return 0;
 }
 
-union cpuid10_eax {
-	struct {
-		unsigned int version_id:8;
-		unsigned int num_counters:8;
-		unsigned int bit_width:8;
-		unsigned int mask_length:8;
-	} split;
-	unsigned int full;
-} eax;
-
-u32 lbr_from, lbr_to;
-
 static void init_lbr(void *index)
 {
 	wrmsr(lbr_from + *(int *) index, 0);
@@ -63,7 +52,7 @@ static bool test_init_lbr_from_exception(u64 index)
 
 int main(int ac, char **av)
 {
-	struct cpuid id = cpuid(10);
+	u8 version = pmu_version();
 	u64 perf_cap;
 	int max, i;
 
@@ -74,19 +63,24 @@ int main(int ac, char **av)
 		return 0;
 	}
 
-	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
-	eax.full = id.a;
-
-	if (!eax.split.version_id) {
+	if (!version) {
 		printf("No pmu is detected!\n");
 		return report_summary();
 	}
+
+	if (!this_cpu_has(X86_FEATURE_PDCM)) {
+		printf("Perfmon/Debug Capabilities MSR isn't supported\n");
+		return report_summary();
+	}
+
+	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
+
 	if (!(perf_cap & PMU_CAP_LBR_FMT)) {
-		printf("No LBR is detected!\n");
+		printf("(Architectural) LBR is not supported.\n");
 		return report_summary();
 	}
 
-	printf("PMU version:		 %d\n", eax.split.version_id);
+	printf("PMU version:		 %d\n", version);
 	printf("LBR version:		 %ld\n", perf_cap & PMU_CAP_LBR_FMT);
 
 	/* Look for LBR from and to MSRs */
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support
  2022-06-28  9:32 [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support Yang Weijiang
  2022-06-28  9:32 ` [kvm-unit-tests PATCH v4 2/2] x86: Check platform vPMU capabilities before run lbr tests Yang Weijiang
@ 2022-07-07 20:26 ` Sean Christopherson
  2022-07-08  0:48   ` Yang, Weijiang
  1 sibling, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2022-07-07 20:26 UTC (permalink / raw)
  To: Yang Weijiang; +Cc: pbonzini, kvm

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()?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [kvm-unit-tests PATCH v4 2/2] x86: Check platform vPMU capabilities before run lbr tests
  2022-06-28  9:32 ` [kvm-unit-tests PATCH v4 2/2] x86: Check platform vPMU capabilities before run lbr tests Yang Weijiang
@ 2022-07-07 20:28   ` Sean Christopherson
  2022-07-08  0:57     ` Yang, Weijiang
  0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2022-07-07 20:28 UTC (permalink / raw)
  To: Yang Weijiang; +Cc: pbonzini, kvm

On Tue, Jun 28, 2022, Yang Weijiang wrote:
> Use new helper to check whether pmu is available and Perfmon/Debug
> capbilities are supported before read MSR_IA32_PERF_CAPABILITIES to
> avoid test failure. The issue can be captured when enable_pmu=0.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> 
> v4:
> - Put the X86_FEATURE_PDCM to the right place. [Sean]
> ---

<version info goes here>

>  lib/x86/processor.h |  1 +
>  x86/pmu_lbr.c       | 32 +++++++++++++-------------------
>  2 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 7b6ee92..7a35c7f 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -146,6 +146,7 @@ static inline bool is_intel(void)
>   */
>  #define	X86_FEATURE_MWAIT		(CPUID(0x1, 0, ECX, 3))
>  #define	X86_FEATURE_VMX			(CPUID(0x1, 0, ECX, 5))
> +#define	X86_FEATURE_PDCM		(CPUID(0x1, 0, ECX, 15))
>  #define	X86_FEATURE_PCID		(CPUID(0x1, 0, ECX, 17))
>  #define	X86_FEATURE_MOVBE		(CPUID(0x1, 0, ECX, 22))
>  #define	X86_FEATURE_TSC_DEADLINE_TIMER	(CPUID(0x1, 0, ECX, 24))
> diff --git a/x86/pmu_lbr.c b/x86/pmu_lbr.c
> index 688634d..497df1e 100644
> --- a/x86/pmu_lbr.c
> +++ b/x86/pmu_lbr.c
> @@ -15,6 +15,7 @@
>  #define MSR_LBR_SELECT		0x000001c8
>  
>  volatile int count;
> +u32 lbr_from, lbr_to;
>  
>  static noinline int compute_flag(int i)
>  {
> @@ -38,18 +39,6 @@ static noinline int lbr_test(void)
>  	return 0;
>  }
>  
> -union cpuid10_eax {
> -	struct {
> -		unsigned int version_id:8;
> -		unsigned int num_counters:8;
> -		unsigned int bit_width:8;
> -		unsigned int mask_length:8;
> -	} split;
> -	unsigned int full;
> -} eax;
> -
> -u32 lbr_from, lbr_to;
> -
>  static void init_lbr(void *index)
>  {
>  	wrmsr(lbr_from + *(int *) index, 0);
> @@ -63,7 +52,7 @@ static bool test_init_lbr_from_exception(u64 index)
>  
>  int main(int ac, char **av)
>  {
> -	struct cpuid id = cpuid(10);
> +	u8 version = pmu_version();
>  	u64 perf_cap;
>  	int max, i;
>  
> @@ -74,19 +63,24 @@ int main(int ac, char **av)
>  		return 0;
>  	}
>  
> -	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
> -	eax.full = id.a;
> -
> -	if (!eax.split.version_id) {
> +	if (!version) {

If the previous patch exposes cpu_has_pmu(), then this open coded check goes away
in favor of the more obvious:

	if (!cpu_has_pmu()) {

>  		printf("No pmu is detected!\n");
>  		return report_summary();
>  	}
> +
> +	if (!this_cpu_has(X86_FEATURE_PDCM)) {
> +		printf("Perfmon/Debug Capabilities MSR isn't supported\n");
> +		return report_summary();
> +	}
> +
> +	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
> +
>  	if (!(perf_cap & PMU_CAP_LBR_FMT)) {
> -		printf("No LBR is detected!\n");
> +		printf("(Architectural) LBR is not supported.\n");
>  		return report_summary();
>  	}
>  
> -	printf("PMU version:		 %d\n", eax.split.version_id);
> +	printf("PMU version:		 %d\n", version);

And with the open coded check gone, this can be:
	
	printf("PMU version:		 %d\n", pmu_version());

>  	printf("LBR version:		 %ld\n", perf_cap & PMU_CAP_LBR_FMT);
>  
>  	/* Look for LBR from and to MSRs */
> -- 
> 2.27.0
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support
  2022-07-07 20:26 ` [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support Sean Christopherson
@ 2022-07-08  0:48   ` Yang, Weijiang
  0 siblings, 0 replies; 6+ messages in thread
From: Yang, Weijiang @ 2022-07-08  0:48 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: pbonzini, kvm


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!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [kvm-unit-tests PATCH v4 2/2] x86: Check platform vPMU capabilities before run lbr tests
  2022-07-07 20:28   ` Sean Christopherson
@ 2022-07-08  0:57     ` Yang, Weijiang
  0 siblings, 0 replies; 6+ messages in thread
From: Yang, Weijiang @ 2022-07-08  0:57 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: pbonzini, kvm


On 7/8/2022 4:28 AM, Sean Christopherson wrote:
> On Tue, Jun 28, 2022, Yang Weijiang wrote:
>> Use new helper to check whether pmu is available and Perfmon/Debug
>> capbilities are supported before read MSR_IA32_PERF_CAPABILITIES to
>> avoid test failure. The issue can be captured when enable_pmu=0.
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>>
>> v4:
>> - Put the X86_FEATURE_PDCM to the right place. [Sean]
>> ---
> <version info goes here>
Will change it.
>
>>   lib/x86/processor.h |  1 +
>>   x86/pmu_lbr.c       | 32 +++++++++++++-------------------
>>   2 files changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
>> index 7b6ee92..7a35c7f 100644
>> --- a/lib/x86/processor.h
>> +++ b/lib/x86/processor.h
>> @@ -146,6 +146,7 @@ static inline bool is_intel(void)
>>    */
>>   #define	X86_FEATURE_MWAIT		(CPUID(0x1, 0, ECX, 3))
>>   #define	X86_FEATURE_VMX			(CPUID(0x1, 0, ECX, 5))
>> +#define	X86_FEATURE_PDCM		(CPUID(0x1, 0, ECX, 15))
>>   #define	X86_FEATURE_PCID		(CPUID(0x1, 0, ECX, 17))
>>   #define	X86_FEATURE_MOVBE		(CPUID(0x1, 0, ECX, 22))
>>   #define	X86_FEATURE_TSC_DEADLINE_TIMER	(CPUID(0x1, 0, ECX, 24))
>> diff --git a/x86/pmu_lbr.c b/x86/pmu_lbr.c
>> index 688634d..497df1e 100644
>> --- a/x86/pmu_lbr.c
>> +++ b/x86/pmu_lbr.c
>> @@ -15,6 +15,7 @@
>>   #define MSR_LBR_SELECT		0x000001c8
>>   
>>   volatile int count;
>> +u32 lbr_from, lbr_to;
>>   
>>   static noinline int compute_flag(int i)
>>   {
>> @@ -38,18 +39,6 @@ static noinline int lbr_test(void)
>>   	return 0;
>>   }
>>   
>> -union cpuid10_eax {
>> -	struct {
>> -		unsigned int version_id:8;
>> -		unsigned int num_counters:8;
>> -		unsigned int bit_width:8;
>> -		unsigned int mask_length:8;
>> -	} split;
>> -	unsigned int full;
>> -} eax;
>> -
>> -u32 lbr_from, lbr_to;
>> -
>>   static void init_lbr(void *index)
>>   {
>>   	wrmsr(lbr_from + *(int *) index, 0);
>> @@ -63,7 +52,7 @@ static bool test_init_lbr_from_exception(u64 index)
>>   
>>   int main(int ac, char **av)
>>   {
>> -	struct cpuid id = cpuid(10);
>> +	u8 version = pmu_version();
>>   	u64 perf_cap;
>>   	int max, i;
>>   
>> @@ -74,19 +63,24 @@ int main(int ac, char **av)
>>   		return 0;
>>   	}
>>   
>> -	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
>> -	eax.full = id.a;
>> -
>> -	if (!eax.split.version_id) {
>> +	if (!version) {
> If the previous patch exposes cpu_has_pmu(), then this open coded check goes away
> in favor of the more obvious:
>
> 	if (!cpu_has_pmu()) {
Yep.
>>   		printf("No pmu is detected!\n");
>>   		return report_summary();
>>   	}
>> +
>> +	if (!this_cpu_has(X86_FEATURE_PDCM)) {
>> +		printf("Perfmon/Debug Capabilities MSR isn't supported\n");
>> +		return report_summary();
>> +	}
>> +
>> +	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
>> +
>>   	if (!(perf_cap & PMU_CAP_LBR_FMT)) {
>> -		printf("No LBR is detected!\n");
>> +		printf("(Architectural) LBR is not supported.\n");
>>   		return report_summary();
>>   	}
>>   
>> -	printf("PMU version:		 %d\n", eax.split.version_id);
>> +	printf("PMU version:		 %d\n", version);
> And with the open coded check gone, this can be:
> 	
> 	printf("PMU version:		 %d\n", pmu_version());
Will change it. Thank you!
>
>>   	printf("LBR version:		 %ld\n", perf_cap & PMU_CAP_LBR_FMT);
>>   
>>   	/* Look for LBR from and to MSRs */
>> -- 
>> 2.27.0
>>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-07-08  0:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-28  9:32 [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support Yang Weijiang
2022-06-28  9:32 ` [kvm-unit-tests PATCH v4 2/2] x86: Check platform vPMU capabilities before run lbr tests Yang Weijiang
2022-07-07 20:28   ` Sean Christopherson
2022-07-08  0:57     ` Yang, Weijiang
2022-07-07 20:26 ` [kvm-unit-tests PATCH v4 1/2] x86: Skip perf related tests when platform cannot support Sean Christopherson
2022-07-08  0:48   ` Yang, Weijiang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.