On Wed, 8 Jan 2025 at 21:28, David Kaplan <david.kaplan@amd.com> 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@amd.com>
> ---
> 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.