RE: [PATCH v3 10/35] x86/bugs: Restructure gds mitigation

[Kaplan, David]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Brendan Jackman <jackmanb@xxxxxxxxxx>
> Sent: Monday, February 10, 2025 11:06 AM
> To: Kaplan, David <David.Kaplan@xxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Borislav Petkov <bp@xxxxxxxxx>; Peter
> Zijlstra <peterz@xxxxxxxxxxxxx>; Josh Poimboeuf <jpoimboe@xxxxxxxxxx>; Pawan
> Gupta <pawan.kumar.gupta@xxxxxxxxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>;
> Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>; x86@xxxxxxxxxx; H . Peter Anvin
> <hpa@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 10/35] x86/bugs: Restructure gds mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, 8 Jan 2025 at 21:28, David Kaplan <david.kaplan@xxxxxxx> wrote:
> >
> > Restructure gds mitigation to use select/apply functions to create
> > consistent vulnerability handling.
> >
> > Define new AUTO mitigation for gds.
> >
> > Signed-off-by: David Kaplan <david.kaplan@xxxxxxx>
> > ---
> >  arch/x86/kernel/cpu/bugs.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index fedd693b2218..58ac99b74bd3 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -76,6 +76,7 @@ static void __init srbds_apply_mitigation(void);
> > static void __init l1d_flush_select_mitigation(void);
> >  static void __init srso_select_mitigation(void);  static void __init
> > gds_select_mitigation(void);
> > +static void __init gds_apply_mitigation(void);
> >
> >  /* The base value of the SPEC_CTRL MSR without task-specific bits set
> > */
> >  u64 x86_spec_ctrl_base;
> > @@ -220,6 +221,7 @@ void __init cpu_select_mitigations(void)
> >         mmio_apply_mitigation();
> >         rfds_apply_mitigation();
> >         srbds_apply_mitigation();
> > +       gds_apply_mitigation();
> >  }
> >
> >  /*
> > @@ -811,6 +813,7 @@ early_param("l1d_flush", l1d_flush_parse_cmdline);
> >
> >  enum gds_mitigations {
> >         GDS_MITIGATION_OFF,
> > +       GDS_MITIGATION_AUTO,
> >         GDS_MITIGATION_UCODE_NEEDED,
> >         GDS_MITIGATION_FORCE,
> >         GDS_MITIGATION_FULL,
> > @@ -819,7 +822,7 @@ enum gds_mitigations {  };
> >
> >  static enum gds_mitigations gds_mitigation __ro_after_init =
> > -       IS_ENABLED(CONFIG_MITIGATION_GDS) ? GDS_MITIGATION_FULL :
> GDS_MITIGATION_OFF;
> > +       IS_ENABLED(CONFIG_MITIGATION_GDS) ? GDS_MITIGATION_AUTO
> :
> > + GDS_MITIGATION_OFF;
> >
> >  static const char * const gds_strings[] = {
> >         [GDS_MITIGATION_OFF]            = "Vulnerable",
> > @@ -860,6 +863,7 @@ void update_gds_msr(void)
> >         case GDS_MITIGATION_FORCE:
> >         case GDS_MITIGATION_UCODE_NEEDED:
> >         case GDS_MITIGATION_HYPERVISOR:
> > +       case GDS_MITIGATION_AUTO:
> >                 return;
> >         }
> >
> > @@ -883,13 +887,16 @@ static void __init gds_select_mitigation(void)
> >
> >         if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> >                 gds_mitigation = GDS_MITIGATION_HYPERVISOR;
> > -               goto out;
> > +               return;
> >         }
> >
> >         if (cpu_mitigations_off())
> >                 gds_mitigation = GDS_MITIGATION_OFF;
> >         /* Will verify below that mitigation _can_ be disabled */
> >
> > +       if (gds_mitigation == GDS_MITIGATION_AUTO)
> > +               gds_mitigation = GDS_MITIGATION_FULL;
> > +
> >         /* No microcode */
> >         if (!(x86_arch_cap_msr & ARCH_CAP_GDS_CTRL)) {
> >                 if (gds_mitigation == GDS_MITIGATION_FORCE) { @@
> > -902,7 +909,7 @@ static void __init gds_select_mitigation(void)
> >                 } else {
> >                         gds_mitigation = GDS_MITIGATION_UCODE_NEEDED;
> >                 }
> > -               goto out;
> > +               return;
> >         }
> >
> >         /* Microcode has mitigation, use it */ @@ -923,9 +930,16 @@
> > static void __init gds_select_mitigation(void)
> >                  */
> >                 gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> >         }
> > +}
> > +
> > +static void __init gds_apply_mitigation(void) {
> > +       if (!boot_cpu_has_bug(X86_BUG_GDS))
> > +               return;
> > +
> > +       if (x86_arch_cap_msr & ARCH_CAP_GDS_CTRL)
> > +               update_gds_msr();
>
> IMO it's a shame to be looking at MSR bits in here instead of just relying on the
> direct output of the select/update functions.
>
> I think in this case we can just remove the conditional since if
> !ARCH_CAP_GDS_CTRL then gds_mitigation must be FORCE or
> UCODE_NEEDED in which case update_gds_msr() is a nop.
>
> Now I make these comments I realise maybe my expectation about these three
> functions is not actually the same as yours. Here's how I envisaged your design:
>
> - select: Look around at the hardware and the cmdline and decide what we think we
> wanna do in fairly abstract terms. Record that result in *_mitigation.
>
> - update: Look around at the other mitigations and potentially change our mind (or
> perhaps just update *_mitigation to reflect mitigation that is being done regardless
> for other vulns, which also mitigate this vuln).
>
> - apply: Poke the hardware/cap flags/static keys/etc to actuate the decision we
> made in the previous steps.
>
> Let me know if that's aligned with your vision or not.

Yes, that is well-aligned.  Basically select picks a mitigation based on the hardware and cmdline but not the mitigations of any other bugs.  Then update re-evaluates that and may change our mind, and apply pokes the hardware.

On the ARCH_CAP_GDS_CTRL, I thought that check is really just to check if the MSR is present, before we try to read it.

--David Kaplan