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

Pawan Gupta
2022-03-31T08:46:29Z

On Thu, Mar 31, 2022 at 09:48:14AM +0200, Ricardo Cañuelo wrote:
>Borislav Petkov <bp@alien8.de> 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);