Re: [PATCH v5 10/16] x86/bugs: Restructure retbleed mitigation

Borislav Petkov
2025-04-28T19:00:13Z

On Fri, Apr 18, 2025 at 11:17:15AM -0500, David Kaplan wrote:
> @@ -187,11 +189,6 @@ void __init cpu_select_mitigations(void)
> /* Select the proper CPU mitigations before patching alternatives: */
> spectre_v1_select_mitigation();
> spectre_v2_select_mitigation();
> - /*
> - * retbleed_select_mitigation() relies on the state set by
> - * spectre_v2_select_mitigation(); specifically it wants to know about
> - * spectre_v2=ibrs.
> - */
> retbleed_select_mitigation();
> /*
> * spectre_v2_user_select_mitigation() relies on the state set by
> @@ -219,12 +216,14 @@ void __init cpu_select_mitigations(void)
> * After mitigations are selected, some may need to update their
> * choices.
> */
> + retbleed_update_mitigation();

Is there any particular reason for the retbleed update function to go first...

> mds_update_mitigation();
> taa_update_mitigation();
> mmio_update_mitigation();
> rfds_update_mitigation();

... before those?

I'm under the assumption that the new scheme would get rid of this magical
ordering requirement between the mitigations...

Your commit message is alluding to that but we need to specify this clearly
for future cleanups/changes here.

> spectre_v1_apply_mitigation();
> + retbleed_apply_mitigation();

This too.

> mds_apply_mitigation();
> taa_apply_mitigation();
> mmio_apply_mitigation();

...

> -do_cmd_auto:
> - case RETBLEED_CMD_AUTO:
> + if (retbleed_mitigation == RETBLEED_MITIGATION_AUTO) {
> + /* Intel mitigation selected in retbleed_update_mitigation() */
> if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> if (IS_ENABLED(CONFIG_MITIGATION_UNRET_ENTRY))
> @@ -1212,18 +1187,65 @@ static void __init retbleed_select_mitigation(void)
> else if (IS_ENABLED(CONFIG_MITIGATION_IBPB_ENTRY) &&
> boot_cpu_has(X86_FEATURE_IBPB))
> retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
> + else
> + retbleed_mitigation = RETBLEED_MITIGATION_NONE;
> }
> + }
> +}

I'd flip that outer check in order to save an indentation level here:

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 9d6ce4a167be..207a472d1a6e 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1182,18 +1182,19 @@ static void __init retbleed_select_mitigation(void)
break;
}

- if (retbleed_mitigation == RETBLEED_MITIGATION_AUTO) {
- /* Intel mitigation selected in retbleed_update_mitigation() */
- if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
- boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
- if (IS_ENABLED(CONFIG_MITIGATION_UNRET_ENTRY))
- retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
- else if (IS_ENABLED(CONFIG_MITIGATION_IBPB_ENTRY) &&
- boot_cpu_has(X86_FEATURE_IBPB))
- retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
- else
- retbleed_mitigation = RETBLEED_MITIGATION_NONE;
- }
+ if (retbleed_mitigation != RETBLEED_MITIGATION_AUTO)
+ return;
+
+ /* Intel mitigation selected in retbleed_update_mitigation() */
+ if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
+ boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
+ if (IS_ENABLED(CONFIG_MITIGATION_UNRET_ENTRY))
+ retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
+ else if (IS_ENABLED(CONFIG_MITIGATION_IBPB_ENTRY) &&
+ boot_cpu_has(X86_FEATURE_IBPB))
+ retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
+ else
+ retbleed_mitigation = RETBLEED_MITIGATION_NONE;
}
}

Thx.

--
Regards/Gruss,
Boris.