Re: [kvm-unit-tests PATCH 2/3] x86/msr: Add a testcase to verify SPEC_CTRL exists (or not) as expected

[Chao Gao <chao.gao@intel.com>]

On Thu, Jun 05, 2025 at 12:26:42PM -0700, Sean Christopherson wrote:
>Verify that SPEC_CTRL can be read when it should exist, #GPs on all reads
>and writes does not exist, and that various bits can be set when they're
>supported.
>
>Opportunistically define more AMD mitigation features.
>
>Cc: Chao Gao <chao.gao@intel.com>
>Signed-off-by: Sean Christopherson <seanjc@google.com>
>---
> lib/x86/msr.h       |  8 ++++++--
> lib/x86/processor.h |  9 +++++++--
> x86/msr.c           | 31 +++++++++++++++++++++++++++++--
> 3 files changed, 42 insertions(+), 6 deletions(-)
>
>diff --git a/lib/x86/msr.h b/lib/x86/msr.h
>index ccfd6bdd..cc4cb855 100644
>--- a/lib/x86/msr.h
>+++ b/lib/x86/msr.h
>@@ -32,8 +32,12 @@
> #define EFER_FFXSR		(1<<_EFER_FFXSR)
> 
> /* Intel MSRs. Some also available on other CPUs */
>-#define MSR_IA32_SPEC_CTRL              0x00000048
>-#define MSR_IA32_PRED_CMD               0x00000049
>+#define MSR_IA32_SPEC_CTRL		0x00000048
>+#define SPEC_CTRL_IBRS			BIT(0)
>+#define SPEC_CTRL_STIBP			BIT(1)
>+#define SPEC_CTRL_SSBD			BIT(2)
>+
>+#define MSR_IA32_PRED_CMD		0x00000049
> #define PRED_CMD_IBPB			BIT(0)
> 
> #define MSR_IA32_FLUSH_CMD		0x0000010b
>diff --git a/lib/x86/processor.h b/lib/x86/processor.h
>index 9e3659d4..cbfaa018 100644
>--- a/lib/x86/processor.h
>+++ b/lib/x86/processor.h
>@@ -288,13 +288,13 @@ struct x86_cpu_feature {
> #define	X86_FEATURE_LA57		X86_CPU_FEATURE(0x7, 0, ECX, 16)
> #define	X86_FEATURE_RDPID		X86_CPU_FEATURE(0x7, 0, ECX, 22)
> #define	X86_FEATURE_SHSTK		X86_CPU_FEATURE(0x7, 0, ECX, 7)
>+#define	X86_FEATURE_PKS			X86_CPU_FEATURE(0x7, 0, ECX, 31)
> #define	X86_FEATURE_IBT			X86_CPU_FEATURE(0x7, 0, EDX, 20)
> #define	X86_FEATURE_SPEC_CTRL		X86_CPU_FEATURE(0x7, 0, EDX, 26)
> #define	X86_FEATURE_FLUSH_L1D		X86_CPU_FEATURE(0x7, 0, EDX, 28)
> #define	X86_FEATURE_ARCH_CAPABILITIES	X86_CPU_FEATURE(0x7, 0, EDX, 29)
>-#define	X86_FEATURE_PKS			X86_CPU_FEATURE(0x7, 0, ECX, 31)
>+#define X86_FEATURE_SSBD		X86_CPU_FEATURE(0x7, 0, EDX, 31)

nit: looks adding a tab after "#define" is the convention in this file

> #define	X86_FEATURE_LAM			X86_CPU_FEATURE(0x7, 1, EAX, 26)
>-
> /*
>  * KVM defined leafs
>  */
>@@ -312,6 +312,11 @@ struct x86_cpu_feature {
> #define	X86_FEATURE_LM			X86_CPU_FEATURE(0x80000001, 0, EDX, 29)
> #define	X86_FEATURE_RDPRU		X86_CPU_FEATURE(0x80000008, 0, EBX, 4)
> #define	X86_FEATURE_AMD_IBPB		X86_CPU_FEATURE(0x80000008, 0, EBX, 12)
>+#define	X86_FEATURE_AMD_IBRS		X86_CPU_FEATURE(0x80000008, 0, EBX, 14)
>+#define X86_FEATURE_AMD_STIBP		X86_CPU_FEATURE(0x80000008, 0, EBX, 15)
>+#define X86_FEATURE_AMD_STIBP_ALWAYS_ON	X86_CPU_FEATURE(0x80000008, 0, EBX, 17)
>+#define X86_FEATURE_AMD_IBRS_SAME_MODE	X86_CPU_FEATURE(0x80000008, 0, EBX, 19)
>+#define X86_FEATURE_AMD_SSBD		X86_CPU_FEATURE(0x80000008, 0, EBX, 24)

ditto

> #define	X86_FEATURE_NPT			X86_CPU_FEATURE(0x8000000A, 0, EDX, 0)
> #define	X86_FEATURE_LBRV		X86_CPU_FEATURE(0x8000000A, 0, EDX, 1)
> #define	X86_FEATURE_NRIPS		X86_CPU_FEATURE(0x8000000A, 0, EDX, 3)
>diff --git a/x86/msr.c b/x86/msr.c
>index ac12d127..ca265fac 100644
>--- a/x86/msr.c
>+++ b/x86/msr.c
>@@ -290,10 +290,37 @@ static void test_x2apic_msrs(void)
> 	__test_x2apic_msrs(true);
> }
> 
>-static void test_cmd_msrs(void)
>+static void test_mitigation_msrs(void)
> {
>+	u64 spec_ctrl_bits = 0, val;
> 	int i;
> 
>+	if (this_cpu_has(X86_FEATURE_SPEC_CTRL) || this_cpu_has(X86_FEATURE_AMD_IBRS))
>+		spec_ctrl_bits |= SPEC_CTRL_IBRS;
>+
>+	if (this_cpu_has(X86_FEATURE_SPEC_CTRL) || this_cpu_has(X86_FEATURE_AMD_STIBP))
>+		spec_ctrl_bits |= SPEC_CTRL_STIBP;

CPUID.(EAX=07H, ECX=0):EDX[26] enumerates IBRS and IBPB support, but it doesn't
enumerate STIBP support. EDX[27] does.

Aside from this, the patch looks good to me.

Reviewed-by: Chao Gao <chao.gao@intel.com>

>+
>+	if (this_cpu_has(X86_FEATURE_SSBD) || this_cpu_has(X86_FEATURE_AMD_SSBD))
>+		spec_ctrl_bits |= SPEC_CTRL_SSBD;
>+
>+	if (spec_ctrl_bits) {
>+		for (val = 0; val <= spec_ctrl_bits; val++) {
>+			/*
>+			 * Test only values that are guaranteed not to fault,
>+			 * virtualization of SPEC_CTRL has myriad holes that
>+			 * won't be ever closed.
>+			 */
>+			if ((val & spec_ctrl_bits) != val)
>+				continue;
>+
>+			test_msr_rw(MSR_IA32_SPEC_CTRL, "SPEC_CTRL", val);
>+		}
>+	} else {
>+		test_rdmsr_fault(MSR_IA32_SPEC_CTRL, "SPEC_CTRL");
>+		test_wrmsr_fault(MSR_IA32_SPEC_CTRL, "SPEC_CTRL", 0);
>+	}
>+
> 	test_rdmsr_fault(MSR_IA32_PRED_CMD, "PRED_CMD");
> 	if (this_cpu_has(X86_FEATURE_SPEC_CTRL) ||
> 	    this_cpu_has(X86_FEATURE_AMD_IBPB) ||
>@@ -332,7 +359,7 @@ int main(int ac, char **av)
> 		test_misc_msrs();
> 		test_mce_msrs();
> 		test_x2apic_msrs();
>-		test_cmd_msrs();
>+		test_mitigation_msrs();
> 	}
> 
> 	return report_summary();
>-- 
>2.50.0.rc0.604.gd4ff7b7c86-goog
>