Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Jul 2022 08:17:26 -0300
From:   Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
To:     Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org,
        Daniel Sneddon <daniel.sneddon@linux.intel.com>,
        antonio.gomez.iglesias@linux.intel.com,
        Josh Poimboeuf <jpoimboe@kernel.org>
Subject: Re: [PATCH] x86/bugs: Switch to "auto" when "ibrs" selected on
 Enhanced IBRS parts
Message-ID: <Ys/7RiC9Z++38tzq@quatroqueijos>
References: <0456b35fb9ef957d9a9138e0913fb1a3fd445dff.1657747493.git.pawan.kumar.gupta@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0456b35fb9ef957d9a9138e0913fb1a3fd445dff.1657747493.git.pawan.kumar.gupta@linux.intel.com>
Precedence: bulk

On Wed, Jul 13, 2022 at 10:32:37PM -0700, Pawan Gupta wrote:
> Currently spectre_v2=ibrs forces write to MSR_IA32_SPEC_CTRL at every
> entry and exit. On Enhanced IBRS parts setting MSR_IA32_SPEC_CTRL[IBRS]
> only once at bootup is sufficient. MSR write at every kernel entry/exit
> incur unnecessary penalty that can be avoided.
> 
> When Enhanced IBRS feature is present, switch from "ibrs" to "auto" mode
> so that appropriate mitigation is selected.
> 
> Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> Cc: stable@vger.kernel.org # 5.10+
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/bugs.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 0dd04713434b..7d7ebfdfbeda 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1303,6 +1303,12 @@ static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
>  		return SPECTRE_V2_CMD_AUTO;
>  	}
>  
> +	if (cmd == SPECTRE_V2_CMD_IBRS && boot_cpu_has(X86_FEATURE_IBRS_ENHANCED)) {
> +		pr_err("%s selected but CPU supports Enhanced IBRS. Switching to AUTO select\n",
> +		       mitigation_options[i].option);
> +		return SPECTRE_V2_CMD_AUTO;
> +	}
> +
>  	spec_v2_print_cond(mitigation_options[i].option,
>  			   mitigation_options[i].secure);
>  	return cmd;
> 
> base-commit: 72a8e05d4f66b5af7854df4490e3135168694b6b
> -- 
> 2.35.3
> 
> 

Shouldn't we just use the mitigation the user asked for if it is still
possible? We could add the warning advising the user that a different
mitigation could be used instead with less penalty, but if the user asked for
IBRS and that is available, it should be used.

One of the reasons for that is testing. I know it was useful enough for me and
it helped me find some bugs.

Cascardo.