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:   Wed, 9 Aug 2023 10:28:47 -0400
From:   Josh Poimboeuf <jpoimboe@kernel.org>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org, David.Kaplan@amd.com,
        Andrew.Cooper3@citrix.com, gregkh@linuxfoundation.org
Subject: Re: [RFC][PATCH 06/17] x86/cpu: Add SRSO untrain to retbleed=
Message-ID: <20230809142847.3prfwo6cclbydfep@treble>
References: <20230809071218.000335006@infradead.org>
 <20230809072200.850338672@infradead.org>
 <20230809134233.d7hlutglk2j3f4w3@treble>
 <20230809140603.GO212435@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230809140603.GO212435@hirez.programming.kicks-ass.net>
Precedence: bulk

On Wed, Aug 09, 2023 at 04:06:03PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 09, 2023 at 09:42:33AM -0400, Josh Poimboeuf wrote:
> > On Wed, Aug 09, 2023 at 09:12:24AM +0200, Peter Zijlstra wrote:
> > >  static enum retbleed_mitigation retbleed_mitigation __ro_after_init =
> > > @@ -796,6 +802,10 @@ static int __init retbleed_parse_cmdline
> > >  			retbleed_cmd = RETBLEED_CMD_AUTO;
> > >  		} else if (!strcmp(str, "unret")) {
> > >  			retbleed_cmd = RETBLEED_CMD_UNRET;
> > > +		} else if (!strcmp(str, "srso")) {
> > > +			retbleed_cmd = RETBLEED_CMD_UNRET_SRSO;
> > > +		} else if (!strcmp(str, "srso_alias")) {
> > > +			retbleed_cmd = RETBLEED_CMD_UNRET_SRSO_ALIAS;
> > 
> > It doesn't make sense for "srso_alias" to be a separate cmdline option,
> > as that option is a model-dependent variant of the SRSO mitigation.
> 
> so what I did with retbleed, and what should be fixed here too (I
> forgot) is run with:
> 
>   retbleed=force,unret
> 
> on any random machine (typically Intel, because I have a distinct lack
> of AMD machines :-() and look at the life kernel image to see if all the
> patching worked.
> 
> I suppose I should add:
> 
>   setup_force_cpu_bug(X86_BUG_SRSO);
> 
> to the 'force' option, then:
> 
>   retbleed=force,srso_alias
> 
> should function the same, irrespective of the hardware.
> 
> I'm also of the opinion that the kernel should do as told, even if it
> doesn't make sense. If you tell it nonsense, you get to keep the pieces.
> 
> So in that light, yes I think we should have separate options.

What if I want the SRSO mitigation regardless of CPU model?

> > > @@ -870,8 +915,17 @@ static void __init retbleed_select_mitig
> > >  	default:
> > >  		if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> > >  		    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> > > -			if (IS_ENABLED(CONFIG_CPU_UNRET_ENTRY))
> > > -				retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
> > > +			if (IS_ENABLED(CONFIG_CPU_UNRET_ENTRY)) {
> > > +				if (boot_cpu_has_bug(X86_BUG_RETBLEED))
> > > +					retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
> > > +
> > > +				if (boot_cpu_has_bug(X86_BUG_SRSO) && !boot_cpu_has(X86_FEATURE_SRSO_NO)) {
> > > +					if (boot_cpu_data.x86 == 0x19)
> > > +						retbleed_mitigation = RETBLEED_MITIGATION_UNRET_SRSO_ALIAS;
> > > +					else
> > > +						retbleed_mitigation = RETBLEED_MITIGATION_UNRET_SRSO;
> > 
> > It would be great to get confirmation from somebody at AMD that the SRSO
> > mitigations supersede the Retbleed one, i.e., that the SRSO mitigations
> > also fix Retbleed.
> 
> They should, the discussions we had back then explained the Zen1/2
> retbleed case in quite some detail and the srso case matches that
> exactly with the movabs. A larger instruction is used because we need a
> larger embedded sequence of instructions, but otherwise it is identical.
> 
> The comments provided for srso_alias state the BTB is untrained using
> the explicit aliasing.
> 
> That is to say, AFAIU any of this, yes both srso options untrain the BTB
> and mitigate the earlier retbleed thing.
> 
> SRSO then goes one step further with the RAP/RSB clobber.

Ah, nice.  Please add that information somewhere (e.g., one of the
commit logs).

> > > +				}
> > > +			}
> > >  			else if (IS_ENABLED(CONFIG_CPU_IBPB_ENTRY) && boot_cpu_has(X86_FEATURE_IBPB))
> > >  				retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
> > 
> > Here should have the microcode check too:
> > 
> > 				if (boot_cpu_has_bug(X86_BUG_SRSO) && !has_microcode)
> > 					pr_err("IBPB-extending microcode not applied; SRSO NOT mitigated\n");
> 
> That earlier printk is unconditional of the selected mitigation, you
> really want it printed again?

Hm, if you don't want it printed twice then remove it from the
RETBLEED_CMD_IBPB case too.

-- 
Josh