Re: [PATCH v2] x86/speculation/srbds: do not try to turn mitigation off when not supported

[Pawan Gupta]

On Thu, Mar 31, 2022 at 09:48:14AM +0200, Ricardo Cañuelo wrote:
> Borislav Petkov <bp@xxxxxxxxx> writes:
> > So we could also do the below to denote what the situation is and
> > therefore clear the bug flag for such CPUs.
> >
> > The thing is: I want this to be as clear as possible because bugs.c is
> > already a nightmare and just slapping more logic to it without properly
> > thinking it through is going to be a serious pain to deal with later...
>
> Thanks Boris,
>
> I agree that the more explicit the better, I'll give this a try. I saw
> Pawan's suggestion as well but that one is similar to the originally
> proposed patch in that the logic/checks are split between two functions,
> this solution based on clearing the bug flag seems clearer considering
> the comment just before the code block:
>
> 	/*
> 	 * Check to see if this is one of the MDS_NO systems supporting
> 	 * TSX that are only exposed to SRBDS when TSX is enabled.
> 	 */

If we clear the bug flag and not write to the MSR to disable the
microcode mitigation, there will be unnecessary performance loss due to
microcode mitigation. Specifically RDRAND/RDSEED/EGET_KEY will run
slower.

Yes, the logic/checks between two functions is messy.

Does this simplify things a bit?

---
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6296e1ebed1d..0be6c0eabb71 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -431,34 +431,22 @@ static const char * const srbds_strings[] = {
 	[SRBDS_MITIGATION_HYPERVISOR]	= "Unknown: Dependent on hypervisor status",
 };
 
-static bool srbds_off;
+static bool srbds_dis_ucode_mitigation;
 
 void update_srbds_msr(void)
 {
 	u64 mcu_ctrl;
 
-	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
-		return;
-
-	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
-		return;
-
-	if (srbds_mitigation == SRBDS_MITIGATION_UCODE_NEEDED)
+	if (!boot_cpu_has_bug(X86_BUG_SRBDS) ||
+	    !boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
 		return;
 
 	rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
 
-	switch (srbds_mitigation) {
-	case SRBDS_MITIGATION_OFF:
-	case SRBDS_MITIGATION_TSX_OFF:
+	if (srbds_dis_ucode_mitigation)
 		mcu_ctrl |= RNGDS_MITG_DIS;
-		break;
-	case SRBDS_MITIGATION_FULL:
+	else
 		mcu_ctrl &= ~RNGDS_MITG_DIS;
-		break;
-	default:
-		break;
-	}
 
 	wrmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
 }
@@ -481,9 +469,13 @@ static void __init srbds_select_mitigation(void)
 		srbds_mitigation = SRBDS_MITIGATION_HYPERVISOR;
 	else if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
 		srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED;
-	else if (cpu_mitigations_off() || srbds_off)
+	else if (cpu_mitigations_off())
 		srbds_mitigation = SRBDS_MITIGATION_OFF;
 
+	if (srbds_mitigation == SRBDS_MITIGATION_OFF ||
+	    srbds_mitigation == SRBDS_MITIGATION_TSX_OFF)
+		srbds_dis_ucode_mitigation = true;
+
 	update_srbds_msr();
 	pr_info("%s\n", srbds_strings[srbds_mitigation]);
 }
@@ -496,7 +488,9 @@ static int __init srbds_parse_cmdline(char *str)
 	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
 		return 0;
 
-	srbds_off = !strcmp(str, "off");
+	if (!strcmp(str, "off"))
+		srbds_mitigation = SRBDS_MITIGATION_OFF;
+
 	return 0;
 }
 early_param("srbds", srbds_parse_cmdline);