[kvm-unit-tests PATCH v3 0/3] Fix up test failures due to recent KVM changes

[kvm-unit-tests PATCH v3 0/3] Fix up test failures due to recent KVM changes

[Yang Weijiang]

Recently a few pmu KVM patches have been queued, and resulted into some test
failures based on the queue branch. Fix them in this series.

Patches were tested with below config:

kernel:
kvm/queue, commit 4284f0063c48

qemu:
master, commit 3a821c52e1a3

platform:
Skylake/Sapphire Rapids

v3:
1. Replaced msr test fixup with Paolo's patch at below link per
   Sean's comments, change RO bits to bit 11 and 12 due to Sean's
   commit 9fc222967a39 ("KVM: x86: Give host userspace full control of MSR_IA32_MISC_ENABLES")
   https://lore.kernel.org/all/20220520183207.7952-1-pbonzini@redhat.com/

2. Added helpers to check pmu verison and perf_global_ctrl MSR. [Sean]
3. Refactored pmu_lbr code with new helper.

Paolo Bonzini (1):
  x86: Don't overwrite bits 11 and 12 of MSR_IA32_MISC_ENABLE

Yang Weijiang (2):
  x86: Skip perf related tests when platform cannot support
  x86: Check platform vPMU capabilities before run lbr tests

 lib/x86/processor.h | 12 +++++++++++-
 x86/msr.c           |  8 +++++++-
 x86/pmu_lbr.c       | 32 +++++++++++++-------------------
 x86/vmx_tests.c     | 18 ++++++++++++++++++
 4 files changed, 49 insertions(+), 21 deletions(-)


base-commit: 610c15284a537484682adfb4b6d6313991ab954f
-- 
2.27.0

[PATCH v3 1/3] x86: Don't overwrite bits 11 and 12 of MSR_IA32_MISC_ENABLE

[Yang Weijiang]

From: Paolo Bonzini <pbonzini@redhat.com>

Bits 11 and 12 of MSR_IA32_MISC_ENABLE represent the configuration
of the vPMU, and latest KVM does not allow the guest to modify them.
Adjust kvm-unit-tests to avoid failures.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 x86/msr.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/x86/msr.c b/x86/msr.c
index 44fbb3b..8bf38ef 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -19,6 +19,7 @@ struct msr_info {
 	bool is_64bit_only;
 	const char *name;
 	unsigned long long value;
+	unsigned long long keep;
 };
 
 
@@ -27,6 +28,8 @@ struct msr_info {
 
 #define MSR_TEST(msr, val, only64)	\
 	{ .index = msr, .name = #msr, .value = val, .is_64bit_only = only64 }
+#define MSR_TEST_RO_BITS(msr, val, only64, ro)	\
+	{ .index = msr, .name = #msr, .value = val, .is_64bit_only = only64, .keep = ro }
 
 struct msr_info msr_info[] =
 {
@@ -34,7 +37,8 @@ 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),
+	// read-only: 11, 12
+	MSR_TEST_RO_BITS(MSR_IA32_MISC_ENABLE, 0x400c50009, false, 0x1800),
 	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 +63,8 @@ static void test_msr_rw(struct msr_info *msr, unsigned long long val)
 	 */
 	if (msr->index == MSR_EFER)
 		val |= orig;
+	else
+		val = (val & ~msr->keep) | (orig & msr->keep);
 	wrmsr(msr->index, val);
 	r = rdmsr(msr->index);
 	wrmsr(msr->index, orig);
-- 
2.27.0

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

[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.

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)
+{
+	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;
+			}
+		}
+
 		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");
+		return;
+	}
+
 	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
 		printf("\"load IA32_PERF_GLOBAL_CTRL\" exit control not supported\n");
 		return;
@@ -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

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

[Sean Christopherson]

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
> 

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

[Yang, Weijiang]

On 6/25/2022 6:08 AM, Sean Christopherson wrote:
> 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().
OK, will change it. Thanks.
>
>> +{
>> +	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().
Good, I'll add a 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.
Will do it.
>
>> +		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()?
Will do it.
>
>> @@ -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
>>

[PATCH v3 3/3] x86: Check platform vPMU capabilities before run lbr tests

[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.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 lib/x86/processor.h |  2 +-
 x86/pmu_lbr.c       | 32 +++++++++++++-------------------
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 70b9193..bb917b0 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -193,7 +193,7 @@ static inline bool is_intel(void)
 #define X86_FEATURE_PAUSEFILTER     (CPUID(0x8000000A, 0, EDX, 10))
 #define X86_FEATURE_PFTHRESHOLD     (CPUID(0x8000000A, 0, EDX, 12))
 #define	X86_FEATURE_VGIF		(CPUID(0x8000000A, 0, EDX, 16))
-
+#define	X86_FEATURE_PDCM		(CPUID(0x1, 0, ECX, 15))
 
 static inline bool this_cpu_has(u64 feature)
 {
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

Re: [PATCH v3 3/3] x86: Check platform vPMU capabilities before run lbr tests

[Sean Christopherson]

On Fri, Jun 24, 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.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  lib/x86/processor.h |  2 +-
>  x86/pmu_lbr.c       | 32 +++++++++++++-------------------
>  2 files changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 70b9193..bb917b0 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -193,7 +193,7 @@ static inline bool is_intel(void)
>  #define X86_FEATURE_PAUSEFILTER     (CPUID(0x8000000A, 0, EDX, 10))
>  #define X86_FEATURE_PFTHRESHOLD     (CPUID(0x8000000A, 0, EDX, 12))
>  #define	X86_FEATURE_VGIF		(CPUID(0x8000000A, 0, EDX, 16))
> -
> +#define	X86_FEATURE_PDCM		(CPUID(0x1, 0, ECX, 15))

Please try to think critically about the code you're writing.  All of the existing
X86_FEATURE_* definitions are organized by leaf, sub-leaf, register _and_ bit
position.  And now there's X86_FEATURE_PDCM...

Re: [PATCH v3 3/3] x86: Check platform vPMU capabilities before run lbr tests

[Yang, Weijiang]

On 6/25/2022 6:23 AM, Sean Christopherson wrote:
> On Fri, Jun 24, 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.
>>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>>   lib/x86/processor.h |  2 +-
>>   x86/pmu_lbr.c       | 32 +++++++++++++-------------------
>>   2 files changed, 14 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
>> index 70b9193..bb917b0 100644
>> --- a/lib/x86/processor.h
>> +++ b/lib/x86/processor.h
>> @@ -193,7 +193,7 @@ static inline bool is_intel(void)
>>   #define X86_FEATURE_PAUSEFILTER     (CPUID(0x8000000A, 0, EDX, 10))
>>   #define X86_FEATURE_PFTHRESHOLD     (CPUID(0x8000000A, 0, EDX, 12))
>>   #define	X86_FEATURE_VGIF		(CPUID(0x8000000A, 0, EDX, 16))
>> -
>> +#define	X86_FEATURE_PDCM		(CPUID(0x1, 0, ECX, 15))
> Please try to think critically about the code you're writing.  All of the existing
> X86_FEATURE_* definitions are organized by leaf, sub-leaf, register _and_ bit
> position.  And now there's X86_FEATURE_PDCM...
My fault, will put it at the right place. thanks!