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

Kaplan, David
2025-04-28T20:55:17Z

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Monday, April 28, 2025 2:00 PM
> To: Kaplan, David <David.Kaplan@amd.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>; Peter Zijlstra <peterz@infradead.org>;
> Josh Poimboeuf <jpoimboe@kernel.org>; Pawan Gupta
> <pawan.kumar.gupta@linux.intel.com>; Ingo Molnar <mingo@redhat.com>; Dave
> Hansen <dave.hansen@linux.intel.com>; x86@kernel.org; H . Peter Anvin
> <hpa@zytor.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v5 10/16] x86/bugs: Restructure retbleed mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> 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?

It's really just following the same order as the order that the select functions are called, which largely matches the order in existing code.

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

While this is mostly true, there's still a few dependencies with the update functions which are clearly noted with comments.

>
> 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;
> }
> }
>

Yeah this makes sense.

Thanks --David Kaplan