[kvm-unit-tests PATCH v2 0/3] Fix up failures induced by !enable_pmu

[kvm-unit-tests PATCH v2 0/3] Fix up failures induced by !enable_pmu

[Yang Weijiang]

Recently Paolo and Like submitted fixup patches for !enable_pmu case,
this induces some test failures in kvm unit tests, fix them in this
series.

Patches were tested with below config:

kernel:
kvm/queue, commit 8baacf67c76c

qemu:
master, commit 9b1f58854959

platform:
Skylake/Sapphire Rapids

v2:
1. Check PDCM bit of CPUID(1).ecx before read PERF_CAPABILIIES msr.
2. Rebased to newer kvm unit tests master branch.

Yang Weijiang (3):
  x86: Remove perf enable bit from default config
  x86: Skip running test when pmu is disabled
  x86: Skip perf related tests when pmu is disabled

 x86/msr.c       |  4 +++-
 x86/pmu_lbr.c   | 12 +++++++++++-
 x86/vmx_tests.c | 24 ++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)


base-commit: 610c15284a537484682adfb4b6d6313991ab954f
-- 
2.31.1

[kvm-unit-tests PATCH v2 1/3] x86: Remove perf enable bit from default config

[Yang Weijiang]

When pmu is disabled in KVM by enable_pmu=0, bit 7 of guest
MSR_IA32_MISC_ENABLE is cleared, but the default value of
the MSR assumes pmu is always available, this leads to test
failure. Change the logic to make it aligned with KVM config.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 x86/msr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/x86/msr.c b/x86/msr.c
index 44fbb3b..fc05d6c 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -34,7 +34,7 @@ struct msr_info msr_info[] =
 	MSR_TEST(MSR_IA32_SYSENTER_ESP, addr_ul, false),
 	MSR_TEST(MSR_IA32_SYSENTER_EIP, addr_ul, false),
 	// reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
-	MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51889, false),
+	MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51809, false),
 	MSR_TEST(MSR_IA32_CR_PAT, 0x07070707, false),
 	MSR_TEST(MSR_FS_BASE, addr_64, true),
 	MSR_TEST(MSR_GS_BASE, addr_64, true),
@@ -59,6 +59,8 @@ static void test_msr_rw(struct msr_info *msr, unsigned long long val)
 	 */
 	if (msr->index == MSR_EFER)
 		val |= orig;
+	if (msr->index == MSR_IA32_MISC_ENABLE)
+		val |= MSR_IA32_MISC_ENABLE_EMON & orig;
 	wrmsr(msr->index, val);
 	r = rdmsr(msr->index);
 	wrmsr(msr->index, orig);
-- 
2.31.1

Re: [kvm-unit-tests PATCH v2 1/3] x86: Remove perf enable bit from default config

[Sean Christopherson]

On Wed, Jun 15, 2022, Yang Weijiang wrote:
> When pmu is disabled in KVM by enable_pmu=0, bit 7 of guest
> MSR_IA32_MISC_ENABLE is cleared, but the default value of
> the MSR assumes pmu is always available, this leads to test
> failure. Change the logic to make it aligned with KVM config.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>

Paolo's more generic approach is preferable, though even that can be more generic.

https://lore.kernel.org/all/20220520183207.7952-1-pbonzini@redhat.com

> ---
>  x86/msr.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/x86/msr.c b/x86/msr.c
> index 44fbb3b..fc05d6c 100644
> --- a/x86/msr.c
> +++ b/x86/msr.c
> @@ -34,7 +34,7 @@ struct msr_info msr_info[] =
>  	MSR_TEST(MSR_IA32_SYSENTER_ESP, addr_ul, false),
>  	MSR_TEST(MSR_IA32_SYSENTER_EIP, addr_ul, false),
>  	// reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
> -	MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51889, false),
> +	MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51809, false),
>  	MSR_TEST(MSR_IA32_CR_PAT, 0x07070707, false),
>  	MSR_TEST(MSR_FS_BASE, addr_64, true),
>  	MSR_TEST(MSR_GS_BASE, addr_64, true),
> @@ -59,6 +59,8 @@ static void test_msr_rw(struct msr_info *msr, unsigned long long val)
>  	 */
>  	if (msr->index == MSR_EFER)
>  		val |= orig;
> +	if (msr->index == MSR_IA32_MISC_ENABLE)
> +		val |= MSR_IA32_MISC_ENABLE_EMON & orig;
>  	wrmsr(msr->index, val);
>  	r = rdmsr(msr->index);
>  	wrmsr(msr->index, orig);
> -- 
> 2.31.1
> 

Re: [kvm-unit-tests PATCH v2 1/3] x86: Remove perf enable bit from default config

[Yang, Weijiang]

On 6/17/2022 2:30 AM, Sean Christopherson wrote:
> On Wed, Jun 15, 2022, Yang Weijiang wrote:
>> When pmu is disabled in KVM by enable_pmu=0, bit 7 of guest
>> MSR_IA32_MISC_ENABLE is cleared, but the default value of
>> the MSR assumes pmu is always available, this leads to test
>> failure. Change the logic to make it aligned with KVM config.
>>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> Paolo's more generic approach is preferable, though even that can be more generic.
>
> https://lore.kernel.org/all/20220520183207.7952-1-pbonzini@redhat.com
Saw it , thanks! Maybe I just need to help Paolo resend the patch in my 
series.
>
>> ---
>>   x86/msr.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/x86/msr.c b/x86/msr.c
>> index 44fbb3b..fc05d6c 100644
>> --- a/x86/msr.c
>> +++ b/x86/msr.c
>> @@ -34,7 +34,7 @@ struct msr_info msr_info[] =
>>   	MSR_TEST(MSR_IA32_SYSENTER_ESP, addr_ul, false),
>>   	MSR_TEST(MSR_IA32_SYSENTER_EIP, addr_ul, false),
>>   	// reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
>> -	MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51889, false),
>> +	MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51809, false),
>>   	MSR_TEST(MSR_IA32_CR_PAT, 0x07070707, false),
>>   	MSR_TEST(MSR_FS_BASE, addr_64, true),
>>   	MSR_TEST(MSR_GS_BASE, addr_64, true),
>> @@ -59,6 +59,8 @@ static void test_msr_rw(struct msr_info *msr, unsigned long long val)
>>   	 */
>>   	if (msr->index == MSR_EFER)
>>   		val |= orig;
>> +	if (msr->index == MSR_IA32_MISC_ENABLE)
>> +		val |= MSR_IA32_MISC_ENABLE_EMON & orig;
>>   	wrmsr(msr->index, val);
>>   	r = rdmsr(msr->index);
>>   	wrmsr(msr->index, orig);
>> -- 
>> 2.31.1
>>

[kvm-unit-tests PATCH v2 2/3] x86: Skip running test when pmu is disabled

[Yang Weijiang]

Read MSR_IA32_PERF_CAPABILITIES triggers #GP when pmu is disabled
by enable_pmu=0 in KVM. Let's check whether pmu is available before
issue msr reading to avoid the #GP. Also check PDCM bit before read
the MSR.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 x86/pmu_lbr.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/x86/pmu_lbr.c b/x86/pmu_lbr.c
index 688634d..62614a0 100644
--- a/x86/pmu_lbr.c
+++ b/x86/pmu_lbr.c
@@ -5,6 +5,7 @@
 #define N 1000000
 #define MAX_NUM_LBR_ENTRY	  32
 #define DEBUGCTLMSR_LBR	  (1UL <<  0)
+#define PDCM_ENABLED	  (1UL << 15)
 #define PMU_CAP_LBR_FMT	  0x3f
 
 #define MSR_LBR_NHM_FROM	0x00000680
@@ -74,13 +75,22 @@ int main(int ac, char **av)
 		return 0;
 	}
 
-	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
 	eax.full = id.a;
 
 	if (!eax.split.version_id) {
 		printf("No pmu is detected!\n");
 		return report_summary();
 	}
+
+	id = cpuid(1);
+
+	if (!(id.c & PDCM_ENABLED)) {
+		printf("No PDCM is detected!\n");
+		return report_summary();
+	}
+
+	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
+
 	if (!(perf_cap & PMU_CAP_LBR_FMT)) {
 		printf("No LBR is detected!\n");
 		return report_summary();
-- 
2.31.1

Re: [kvm-unit-tests PATCH v2 2/3] x86: Skip running test when pmu is disabled

[Sean Christopherson]

On Wed, Jun 15, 2022, Yang Weijiang wrote:
> Read MSR_IA32_PERF_CAPABILITIES triggers #GP when pmu is disabled
> by enable_pmu=0 in KVM. Let's check whether pmu is available before
> issue msr reading to avoid the #GP. Also check PDCM bit before read
> the MSR.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  x86/pmu_lbr.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/x86/pmu_lbr.c b/x86/pmu_lbr.c
> index 688634d..62614a0 100644
> --- a/x86/pmu_lbr.c
> +++ b/x86/pmu_lbr.c
> @@ -5,6 +5,7 @@
>  #define N 1000000
>  #define MAX_NUM_LBR_ENTRY	  32
>  #define DEBUGCTLMSR_LBR	  (1UL <<  0)
> +#define PDCM_ENABLED	  (1UL << 15)
>  #define PMU_CAP_LBR_FMT	  0x3f
>  
>  #define MSR_LBR_NHM_FROM	0x00000680
> @@ -74,13 +75,22 @@ int main(int ac, char **av)
>  		return 0;
>  	}
>  
> -	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
>  	eax.full = id.a;
>  
>  	if (!eax.split.version_id) {
>  		printf("No pmu is detected!\n");
>  		return report_summary();
>  	}
> +
> +	id = cpuid(1);
> +
> +	if (!(id.c & PDCM_ENABLED)) {

Don't open code cpuid(), add and use X86_FEATURE_PDCM:

  #define	X86_FEATURE_PDCM		(CPUID(0x1, 0, ECX, 15))

	if (!this_cpu_has(X86_FEATURE_PDCM))
		...



> +		printf("No PDCM is detected!\n");

If your going to bother printing a message, please make it useful.  Every time I
read PMU code I have to reread the kernel's cpufeatures.h to remember what PDCM
stands for.

		printf("Perf/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");

Similar complaint,

		printf("Architectural LBRs are not supported.\n");

>  		return report_summary();
> -- 
> 2.31.1
> 

Re: [kvm-unit-tests PATCH v2 2/3] x86: Skip running test when pmu is disabled

[Yang, Weijiang]

On 6/17/2022 2:34 AM, Sean Christopherson wrote:
> On Wed, Jun 15, 2022, Yang Weijiang wrote:
>> Read MSR_IA32_PERF_CAPABILITIES triggers #GP when pmu is disabled
>> by enable_pmu=0 in KVM. Let's check whether pmu is available before
>> issue msr reading to avoid the #GP. Also check PDCM bit before read
>> the MSR.
>>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>>   x86/pmu_lbr.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/x86/pmu_lbr.c b/x86/pmu_lbr.c
>> index 688634d..62614a0 100644
>> --- a/x86/pmu_lbr.c
>> +++ b/x86/pmu_lbr.c
>> @@ -5,6 +5,7 @@
>>   #define N 1000000
>>   #define MAX_NUM_LBR_ENTRY	  32
>>   #define DEBUGCTLMSR_LBR	  (1UL <<  0)
>> +#define PDCM_ENABLED	  (1UL << 15)
>>   #define PMU_CAP_LBR_FMT	  0x3f
>>   
>>   #define MSR_LBR_NHM_FROM	0x00000680
>> @@ -74,13 +75,22 @@ int main(int ac, char **av)
>>   		return 0;
>>   	}
>>   
>> -	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
>>   	eax.full = id.a;
>>   
>>   	if (!eax.split.version_id) {
>>   		printf("No pmu is detected!\n");
>>   		return report_summary();
>>   	}
>> +
>> +	id = cpuid(1);
>> +
>> +	if (!(id.c & PDCM_ENABLED)) {
> Don't open code cpuid(), add and use X86_FEATURE_PDCM:
>
>    #define	X86_FEATURE_PDCM		(CPUID(0x1, 0, ECX, 15))
>
> 	if (!this_cpu_has(X86_FEATURE_PDCM))
> 		...
Oops, this is more x86 style code, thank you!
>
>
>> +		printf("No PDCM is detected!\n");
> If your going to bother printing a message, please make it useful.  Every time I
> read PMU code I have to reread the kernel's cpufeatures.h to remember what PDCM
> stands for.
>
> 		printf("Perf/Debug Capabilities MSR isn't supported\n");
Exactly, I'll bear it in mind :-)
>
>> +		return report_summary();
>> +	}
>> +
>> +	perf_cap = rdmsr(MSR_IA32_PERF_CAPABILITIES);
>> +
>>   	if (!(perf_cap & PMU_CAP_LBR_FMT)) {
>>   		printf("No LBR is detected!\n");
> Similar complaint,
>
> 		printf("Architectural LBRs are not supported.\n");
Sure.
>
>>   		return report_summary();
>> -- 
>> 2.31.1
>>

[kvm-unit-tests PATCH v2 3/3] x86: Skip perf related tests when pmu is disabled

[Yang Weijiang]

When pmu is disabled in KVM, reading MSR_CORE_PERF_GLOBAL_CTRL
or executing rdpmc leads to #GP, so skip related tests in this case.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 x86/vmx_tests.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 4d581e7..dd6fc13 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -944,6 +944,16 @@ static void insn_intercept_main(void)
 			continue;
 		}
 
+		if (insn_table[cur_insn].flag == CPU_RDPMC) {
+			struct cpuid id = cpuid(10);
+
+			if (!(id.a & 0xff)) {
+				printf("\tFeature required for %s is not supported.\n",
+				       insn_table[cur_insn].name);
+				continue;
+			}
+		}
+
 		if (insn_table[cur_insn].disabled) {
 			printf("\tFeature required for %s is not supported.\n",
 			       insn_table[cur_insn].name);
@@ -7490,6 +7500,13 @@ static void test_perf_global_ctrl(u32 nr, const char *name, u32 ctrl_nr,
 
 static void test_load_host_perf_global_ctrl(void)
 {
+	struct cpuid id = cpuid(10);
+
+	if (!(id.a & 0xff)) {
+		report_skip("test_load_host_perf_global_ctrl");
+		return;
+	}
+
 	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
 		printf("\"load IA32_PERF_GLOBAL_CTRL\" exit control not supported\n");
 		return;
@@ -7502,6 +7519,13 @@ static void test_load_host_perf_global_ctrl(void)
 
 static void test_load_guest_perf_global_ctrl(void)
 {
+	struct cpuid id = cpuid(10);
+
+	if (!(id.a & 0xff)) {
+		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.31.1

Re: [kvm-unit-tests PATCH v2 3/3] x86: Skip perf related tests when pmu is disabled

[Sean Christopherson]

On Wed, Jun 15, 2022, Yang Weijiang wrote:
> When pmu is disabled in KVM, reading MSR_CORE_PERF_GLOBAL_CTRL
> or executing rdpmc leads to #GP, so skip related tests in this case.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  x86/vmx_tests.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 4d581e7..dd6fc13 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -944,6 +944,16 @@ static void insn_intercept_main(void)
>  			continue;
>  		}
>  
> +		if (insn_table[cur_insn].flag == CPU_RDPMC) {
> +			struct cpuid id = cpuid(10);
> +
> +			if (!(id.a & 0xff)) {

Please add helpers to query (a) the PMU version and (b) whether or not PERF_GLOBAL_CTRL
is supported.

> +				printf("\tFeature required for %s is not supported.\n",
> +				       insn_table[cur_insn].name);
> +				continue;
> +			}
> +		}
> +
>  		if (insn_table[cur_insn].disabled) {
>  			printf("\tFeature required for %s is not supported.\n",
>  			       insn_table[cur_insn].name);
> @@ -7490,6 +7500,13 @@ static void test_perf_global_ctrl(u32 nr, const char *name, u32 ctrl_nr,
>  
>  static void test_load_host_perf_global_ctrl(void)
>  {
> +	struct cpuid id = cpuid(10);
> +
> +	if (!(id.a & 0xff)) {
> +		report_skip("test_load_host_perf_global_ctrl");
> +		return;
> +	}
> +
>  	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
>  		printf("\"load IA32_PERF_GLOBAL_CTRL\" exit control not supported\n");
>  		return;
> @@ -7502,6 +7519,13 @@ static void test_load_host_perf_global_ctrl(void)
>  
>  static void test_load_guest_perf_global_ctrl(void)
>  {
> +	struct cpuid id = cpuid(10);
> +
> +	if (!(id.a & 0xff)) {
> +		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.31.1
> 

Re: [kvm-unit-tests PATCH v2 3/3] x86: Skip perf related tests when pmu is disabled

[Yang, Weijiang]

On 6/17/2022 2:40 AM, Sean Christopherson wrote:
> On Wed, Jun 15, 2022, Yang Weijiang wrote:
>> When pmu is disabled in KVM, reading MSR_CORE_PERF_GLOBAL_CTRL
>> or executing rdpmc leads to #GP, so skip related tests in this case.
>>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>>   x86/vmx_tests.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index 4d581e7..dd6fc13 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -944,6 +944,16 @@ static void insn_intercept_main(void)
>>   			continue;
>>   		}
>>   
>> +		if (insn_table[cur_insn].flag == CPU_RDPMC) {
>> +			struct cpuid id = cpuid(10);
>> +
>> +			if (!(id.a & 0xff)) {
> Please add helpers to query (a) the PMU version and (b) whether or not PERF_GLOBAL_CTRL
> is supported.
Sure.
>
>> +				printf("\tFeature required for %s is not supported.\n",
>> +				       insn_table[cur_insn].name);
>> +				continue;
>> +			}
>> +		}
>> +
>>   		if (insn_table[cur_insn].disabled) {
>>   			printf("\tFeature required for %s is not supported.\n",
>>   			       insn_table[cur_insn].name);
>> @@ -7490,6 +7500,13 @@ static void test_perf_global_ctrl(u32 nr, const char *name, u32 ctrl_nr,
>>   
>>   static void test_load_host_perf_global_ctrl(void)
>>   {
>> +	struct cpuid id = cpuid(10);
>> +
>> +	if (!(id.a & 0xff)) {
>> +		report_skip("test_load_host_perf_global_ctrl");
>> +		return;
>> +	}
>> +
>>   	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
>>   		printf("\"load IA32_PERF_GLOBAL_CTRL\" exit control not supported\n");
>>   		return;
>> @@ -7502,6 +7519,13 @@ static void test_load_host_perf_global_ctrl(void)
>>   
>>   static void test_load_guest_perf_global_ctrl(void)
>>   {
>> +	struct cpuid id = cpuid(10);
>> +
>> +	if (!(id.a & 0xff)) {
>> +		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.31.1
>>