diff mbox series

[kvm-unit-tests] x86: msr: Remove the loop for testing reserved bits in MSR_IA32_FLUSH_CMD

Message ID 20240415172542.1830566-1-mizhang@google.com (mailing list archive)
State New, archived
Headers show
Series [kvm-unit-tests] x86: msr: Remove the loop for testing reserved bits in MSR_IA32_FLUSH_CMD | expand

Commit Message

Mingwei Zhang April 15, 2024, 5:25 p.m. UTC 
Avoid testing reserved bits in MSR_IA32_FLUSH_CMD. Since KVM passes through
the MSR at runtime, testing reserved bits directly touches the HW and
should generate #GP. However, some older CPU models like skylake with
certain FMS do not generate #GP.

Ideally, it could be fixed by enumerating all such CPU models. The value
added is would be low. So just remove the testing loop and allow the test
pass.

Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 x86/msr.c | 2 --
 1 file changed, 2 deletions(-)


base-commit: 7b0147ea57dc29ba844f5b60393a0639e55e88af

Comments

Sean Christopherson April 16, 2024, 4:21 p.m. UTC | #1 
On Mon, Apr 15, 2024, Mingwei Zhang wrote:
> Avoid testing reserved bits in MSR_IA32_FLUSH_CMD. Since KVM passes through
> the MSR at runtime, testing reserved bits directly touches the HW and
> should generate #GP. However, some older CPU models like skylake with
> certain FMS do not generate #GP.
> 
> Ideally, it could be fixed by enumerating all such CPU models. The value
> added is would be low. So just remove the testing loop and allow the test
> pass.
> 
> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> ---
>  x86/msr.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/x86/msr.c b/x86/msr.c
> index 3a041fab..76c80d29 100644
> --- a/x86/msr.c
> +++ b/x86/msr.c
> @@ -302,8 +302,6 @@ static void test_cmd_msrs(void)
>  		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", 0);
>  		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", L1D_FLUSH);
>  	}
> -	for (i = 1; i < 64; i++)
> -		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", BIT_ULL(i));

Rather than remove this entirely, what forcing emulation?  E.g. (compile tested
only, and haven't verified all macros)

---
 lib/x86/desc.h      | 30 ++++++++++++++++++++++++------
 lib/x86/processor.h | 18 ++++++++++++++----
 x86/msr.c           | 16 +++++++++++++++-
 3 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 7778a0f8..92c45a48 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -272,9 +272,9 @@ extern gdt_entry_t *get_tss_descr(void);
 extern unsigned long get_gdt_entry_base(gdt_entry_t *entry);
 extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
 
-#define asm_safe(insn, inputs...)					\
+#define __asm_safe(fep, insn, inputs...)				\
 ({									\
-	asm volatile(ASM_TRY("1f")					\
+	asm volatile(__ASM_TRY(fep, "1f")				\
 		     insn "\n\t"					\
 		     "1:\n\t"						\
 		     :							\
@@ -283,9 +283,15 @@ extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
 	exception_vector();						\
 })
 
-#define asm_safe_out1(insn, output, inputs...)				\
+#define asm_safe(insn, inputs...)					\
+	__asm_safe("", insn, inputs)
+
+#define asm_fep_safe(insn, output, inputs...)				\
+	__asm_safe_out1(KVM_FEP, insn, output, inputs)
+
+#define __asm_safe_out1(fep, insn, output, inputs...)			\
 ({									\
-	asm volatile(ASM_TRY("1f")					\
+	asm volatile(__ASM_TRY(fep, "1f")				\
 		     insn "\n\t"					\
 		     "1:\n\t"						\
 		     : output						\
@@ -294,9 +300,15 @@ extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
 	exception_vector();						\
 })
 
-#define asm_safe_out2(insn, output1, output2, inputs...)		\
+#define asm_safe_out1(insn, output, inputs...)				\
+	__asm_safe_out1("", insn, output, inputs)
+
+#define asm_fep_safe_out1(insn, output, inputs...)			\
+	__asm_safe_out1(KVM_FEP, insn, output, inputs)
+
+#define __asm_safe_out2(fep, insn, output1, output2, inputs...)		\
 ({									\
-	asm volatile(ASM_TRY("1f")					\
+	asm volatile(__ASM_TRY(fep, "1f")				\
 		     insn "\n\t"					\
 		     "1:\n\t"						\
 		     : output1, output2					\
@@ -305,6 +317,12 @@ extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
 	exception_vector();						\
 })
 
+#define asm_safe_out2(fep, insn, output1, output2, inputs...)		\
+	__asm_safe_out2("", insn, output1, output2, inputs)
+
+#define asm_fep_safe_out2(insn, output1, output2, inputs...)		\
+	__asm_safe_out2(KVM_FEP, insn, output1, output2, inputs)
+
 #define __asm_safe_report(want, insn, inputs...)			\
 do {									\
 	int vector = asm_safe(insn, inputs);				\
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 44f4fd1e..d20496c0 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -430,12 +430,12 @@ static inline void wrmsr(u32 index, u64 val)
 	asm volatile ("wrmsr" : : "a"(a), "d"(d), "c"(index) : "memory");
 }
 
-#define rdreg64_safe(insn, index, val)					\
+#define __rdreg64_safe(fep, insn, index, val)				\
 ({									\
 	uint32_t a, d;							\
 	int vector;							\
 									\
-	vector = asm_safe_out2(insn, "=a"(a), "=d"(d), "c"(index));	\
+	vector = __asm_safe_out2(fep, insn, "=a"(a), "=d"(d), "c"(index));\
 									\
 	if (vector)							\
 		*(val) = 0;						\
@@ -444,13 +444,18 @@ static inline void wrmsr(u32 index, u64 val)
 	vector;								\
 })
 
-#define wrreg64_safe(insn, index, val)					\
+#define rdreg64_safe(insn, index, val)					\
+	__rdreg64_safe("", insn, index, val)
+
+#define __wrreg64_safe(fep, insn, index, val)				\
 ({									\
 	uint32_t eax = (val), edx = (val) >> 32;			\
 									\
-	asm_safe(insn, "a" (eax), "d" (edx), "c" (index));		\
+	__asm_safe(fep, insn, "a" (eax), "d" (edx), "c" (index));	\
 })
 
+#define wrreg64_safe(insn, index, val)					\
+	__wrreg64_safe("", insn, index, val)
 
 static inline int rdmsr_safe(u32 index, uint64_t *val)
 {
@@ -462,6 +467,11 @@ static inline int wrmsr_safe(u32 index, u64 val)
 	return wrreg64_safe("wrmsr", index, val);
 }
 
+static inline int wrmsr_fep_safe(u32 index, u64 val)
+{
+	return __wrreg64_safe(KVM_FEP, "wrmsr", index, val);
+}
+
 static inline int rdpmc_safe(u32 index, uint64_t *val)
 {
 	return rdreg64_safe("rdpmc", index, val);
diff --git a/x86/msr.c b/x86/msr.c
index 3a041fab..2830530b 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -112,6 +112,16 @@ static void test_rdmsr_fault(u32 msr, const char *name)
 	       "Expected #GP on RDSMR(%s), got vector %d", name, vector);
 }
 
+static void test_wrmsr_fep_fault(u32 msr, const char *name,
+				 unsigned long long val)
+{
+	unsigned char vector = wrmsr_fep_safe(msr, val);
+
+	report(vector == GP_VECTOR,
+	       "Expected #GP on emulated WRSMR(%s, 0x%llx), got vector %d",
+	       name, val, vector);
+}
+
 static void test_msr(struct msr_info *msr, bool is_64bit_host)
 {
 	if (is_64bit_host || !msr->is_64bit_only) {
@@ -302,8 +312,12 @@ static void test_cmd_msrs(void)
 		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", 0);
 		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", L1D_FLUSH);
 	}
+
+	if (!is_fep_available())
+		return;
+
 	for (i = 1; i < 64; i++)
-		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", BIT_ULL(i));
+		test_wrmsr_fep_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", BIT_ULL(i));
 }
 
 int main(int ac, char **av)

base-commit: 38135e08a580b9f3696f9b4ae5ca228dc71a1a56
--
Mingwei Zhang April 17, 2024, 11:11 p.m. UTC | #2 
On Tue, Apr 16, 2024, Sean Christopherson wrote:
> On Mon, Apr 15, 2024, Mingwei Zhang wrote:
> > Avoid testing reserved bits in MSR_IA32_FLUSH_CMD. Since KVM passes through
> > the MSR at runtime, testing reserved bits directly touches the HW and
> > should generate #GP. However, some older CPU models like skylake with
> > certain FMS do not generate #GP.
> > 
> > Ideally, it could be fixed by enumerating all such CPU models. The value
> > added is would be low. So just remove the testing loop and allow the test
> > pass.
> > 
> > Signed-off-by: Mingwei Zhang <mizhang@google.com>
> > ---
> >  x86/msr.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/x86/msr.c b/x86/msr.c
> > index 3a041fab..76c80d29 100644
> > --- a/x86/msr.c
> > +++ b/x86/msr.c
> > @@ -302,8 +302,6 @@ static void test_cmd_msrs(void)
> >  		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", 0);
> >  		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", L1D_FLUSH);
> >  	}
> > -	for (i = 1; i < 64; i++)
> > -		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", BIT_ULL(i));
> 
> Rather than remove this entirely, what forcing emulation?  E.g. (compile tested
> only, and haven't verified all macros)

For sure. Nice, thank you! I think this FEP thing also benefits other
tests like PMU.

Thanks.
-Mingwei
> 
> ---
>  lib/x86/desc.h      | 30 ++++++++++++++++++++++++------
>  lib/x86/processor.h | 18 ++++++++++++++----
>  x86/msr.c           | 16 +++++++++++++++-
>  3 files changed, 53 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index 7778a0f8..92c45a48 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -272,9 +272,9 @@ extern gdt_entry_t *get_tss_descr(void);
>  extern unsigned long get_gdt_entry_base(gdt_entry_t *entry);
>  extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
>  
> -#define asm_safe(insn, inputs...)					\
> +#define __asm_safe(fep, insn, inputs...)				\
>  ({									\
> -	asm volatile(ASM_TRY("1f")					\
> +	asm volatile(__ASM_TRY(fep, "1f")				\
>  		     insn "\n\t"					\
>  		     "1:\n\t"						\
>  		     :							\
> @@ -283,9 +283,15 @@ extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
>  	exception_vector();						\
>  })
>  
> -#define asm_safe_out1(insn, output, inputs...)				\
> +#define asm_safe(insn, inputs...)					\
> +	__asm_safe("", insn, inputs)
> +
> +#define asm_fep_safe(insn, output, inputs...)				\
> +	__asm_safe_out1(KVM_FEP, insn, output, inputs)
> +
> +#define __asm_safe_out1(fep, insn, output, inputs...)			\
>  ({									\
> -	asm volatile(ASM_TRY("1f")					\
> +	asm volatile(__ASM_TRY(fep, "1f")				\
>  		     insn "\n\t"					\
>  		     "1:\n\t"						\
>  		     : output						\
> @@ -294,9 +300,15 @@ extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
>  	exception_vector();						\
>  })
>  
> -#define asm_safe_out2(insn, output1, output2, inputs...)		\
> +#define asm_safe_out1(insn, output, inputs...)				\
> +	__asm_safe_out1("", insn, output, inputs)
> +
> +#define asm_fep_safe_out1(insn, output, inputs...)			\
> +	__asm_safe_out1(KVM_FEP, insn, output, inputs)
> +
> +#define __asm_safe_out2(fep, insn, output1, output2, inputs...)		\
>  ({									\
> -	asm volatile(ASM_TRY("1f")					\
> +	asm volatile(__ASM_TRY(fep, "1f")				\
>  		     insn "\n\t"					\
>  		     "1:\n\t"						\
>  		     : output1, output2					\
> @@ -305,6 +317,12 @@ extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
>  	exception_vector();						\
>  })
>  
> +#define asm_safe_out2(fep, insn, output1, output2, inputs...)		\
> +	__asm_safe_out2("", insn, output1, output2, inputs)
> +
> +#define asm_fep_safe_out2(insn, output1, output2, inputs...)		\
> +	__asm_safe_out2(KVM_FEP, insn, output1, output2, inputs)
> +
>  #define __asm_safe_report(want, insn, inputs...)			\
>  do {									\
>  	int vector = asm_safe(insn, inputs);				\
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 44f4fd1e..d20496c0 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -430,12 +430,12 @@ static inline void wrmsr(u32 index, u64 val)
>  	asm volatile ("wrmsr" : : "a"(a), "d"(d), "c"(index) : "memory");
>  }
>  
> -#define rdreg64_safe(insn, index, val)					\
> +#define __rdreg64_safe(fep, insn, index, val)				\
>  ({									\
>  	uint32_t a, d;							\
>  	int vector;							\
>  									\
> -	vector = asm_safe_out2(insn, "=a"(a), "=d"(d), "c"(index));	\
> +	vector = __asm_safe_out2(fep, insn, "=a"(a), "=d"(d), "c"(index));\
>  									\
>  	if (vector)							\
>  		*(val) = 0;						\
> @@ -444,13 +444,18 @@ static inline void wrmsr(u32 index, u64 val)
>  	vector;								\
>  })
>  
> -#define wrreg64_safe(insn, index, val)					\
> +#define rdreg64_safe(insn, index, val)					\
> +	__rdreg64_safe("", insn, index, val)
> +
> +#define __wrreg64_safe(fep, insn, index, val)				\
>  ({									\
>  	uint32_t eax = (val), edx = (val) >> 32;			\
>  									\
> -	asm_safe(insn, "a" (eax), "d" (edx), "c" (index));		\
> +	__asm_safe(fep, insn, "a" (eax), "d" (edx), "c" (index));	\
>  })
>  
> +#define wrreg64_safe(insn, index, val)					\
> +	__wrreg64_safe("", insn, index, val)
>  
>  static inline int rdmsr_safe(u32 index, uint64_t *val)
>  {
> @@ -462,6 +467,11 @@ static inline int wrmsr_safe(u32 index, u64 val)
>  	return wrreg64_safe("wrmsr", index, val);
>  }
>  
> +static inline int wrmsr_fep_safe(u32 index, u64 val)
> +{
> +	return __wrreg64_safe(KVM_FEP, "wrmsr", index, val);
> +}
> +
>  static inline int rdpmc_safe(u32 index, uint64_t *val)
>  {
>  	return rdreg64_safe("rdpmc", index, val);
> diff --git a/x86/msr.c b/x86/msr.c
> index 3a041fab..2830530b 100644
> --- a/x86/msr.c
> +++ b/x86/msr.c
> @@ -112,6 +112,16 @@ static void test_rdmsr_fault(u32 msr, const char *name)
>  	       "Expected #GP on RDSMR(%s), got vector %d", name, vector);
>  }
>  
> +static void test_wrmsr_fep_fault(u32 msr, const char *name,
> +				 unsigned long long val)
> +{
> +	unsigned char vector = wrmsr_fep_safe(msr, val);
> +
> +	report(vector == GP_VECTOR,
> +	       "Expected #GP on emulated WRSMR(%s, 0x%llx), got vector %d",
> +	       name, val, vector);
> +}
> +
>  static void test_msr(struct msr_info *msr, bool is_64bit_host)
>  {
>  	if (is_64bit_host || !msr->is_64bit_only) {
> @@ -302,8 +312,12 @@ static void test_cmd_msrs(void)
>  		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", 0);
>  		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", L1D_FLUSH);
>  	}
> +
> +	if (!is_fep_available())
> +		return;
> +
>  	for (i = 1; i < 64; i++)
> -		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", BIT_ULL(i));
> +		test_wrmsr_fep_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", BIT_ULL(i));
>  }
>  
>  int main(int ac, char **av)
> 
> base-commit: 38135e08a580b9f3696f9b4ae5ca228dc71a1a56
> -- 
>
diff mbox series

Patch

diff --git a/x86/msr.c b/x86/msr.c
index 3a041fab..76c80d29 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -302,8 +302,6 @@  static void test_cmd_msrs(void)
 		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", 0);
 		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", L1D_FLUSH);
 	}
-	for (i = 1; i < 64; i++)
-		test_wrmsr_fault(MSR_IA32_FLUSH_CMD, "FLUSH_CMD", BIT_ULL(i));
 }
 
 int main(int ac, char **av)