* [kvm-unit-tests PATCH] x86: msr: Remove the loop for testing reserved bits in MSR_IA32_FLUSH_CMD
@ 2024-04-15 17:25 Mingwei Zhang
2024-04-16 16:21 ` Sean Christopherson
0 siblings, 1 reply; 3+ messages in thread
From: Mingwei Zhang @ 2024-04-15 17:25 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson; +Cc: kvm, Mingwei Zhang
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));
}
int main(int ac, char **av)
base-commit: 7b0147ea57dc29ba844f5b60393a0639e55e88af
--
2.44.0.683.g7961c838ac-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: msr: Remove the loop for testing reserved bits in MSR_IA32_FLUSH_CMD
2024-04-15 17:25 [kvm-unit-tests PATCH] x86: msr: Remove the loop for testing reserved bits in MSR_IA32_FLUSH_CMD Mingwei Zhang
@ 2024-04-16 16:21 ` Sean Christopherson
2024-04-17 23:11 ` Mingwei Zhang
0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2024-04-16 16:21 UTC (permalink / raw)
To: Mingwei Zhang; +Cc: Paolo Bonzini, kvm
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
--
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: msr: Remove the loop for testing reserved bits in MSR_IA32_FLUSH_CMD
2024-04-16 16:21 ` Sean Christopherson
@ 2024-04-17 23:11 ` Mingwei Zhang
0 siblings, 0 replies; 3+ messages in thread
From: Mingwei Zhang @ 2024-04-17 23:11 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Paolo Bonzini, kvm
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
> --
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-17 23:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-15 17:25 [kvm-unit-tests PATCH] x86: msr: Remove the loop for testing reserved bits in MSR_IA32_FLUSH_CMD Mingwei Zhang
2024-04-16 16:21 ` Sean Christopherson
2024-04-17 23:11 ` Mingwei Zhang
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.