Date:   Wed, 22 Feb 2023 00:25:42 -0800
From:   Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
To:     KP Singh <kpsingh@kernel.org>
Cc:     linux-kernel@vger.kernel.org, pjt@google.com, evn@google.com,
        jpoimboe@kernel.org, tglx@linutronix.de, x86@kernel.org,
        hpa@zytor.com, peterz@infradead.org, kim.phillips@amd.com,
        alexandre.chartre@oracle.com, daniel.sneddon@linux.intel.com,
        corbet@lwn.net, bp@alien8.de, linyujun809@huawei.com,
        jmattson@google.com,
        =?utf-8?B?Sm9zw6k=?= Oliveira <joseloliveira11@gmail.com>,
        Rodrigo Branco <rodrigo@kernelhacking.com>,
        Alexandra Sandulescu <aesa@google.com>, stable@vger.kernel.org
Subject: Re: [PATCH v2 1/2] x86/speculation: Allow enabling STIBP with legacy
 IBRS
Message-ID: <20230222082542.bjgwgydfdswkqwre@desk>
References: <20230221184908.2349578-1-kpsingh@kernel.org>
 <20230222030728.v4ldlndtnx6gqd6x@desk>
 <CACYkzJ4efHx2oZUW82m3DGw7ssLq37EFOV57X=kT5fm=6Q7WbQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACYkzJ4efHx2oZUW82m3DGw7ssLq37EFOV57X=kT5fm=6Q7WbQ@mail.gmail.com>
Precedence: bulk

On Tue, Feb 21, 2023 at 09:49:57PM -0800, KP Singh wrote:
> > > @@ -1193,13 +1209,8 @@ spectre_v2_user_select_mitigation(void)
> > >                       "always-on" : "conditional");
> > >       }
> > >
> > > -     /*
> > > -      * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible,
> > > -      * STIBP is not required.
> > > -      */
> > > -     if (!boot_cpu_has(X86_FEATURE_STIBP) ||
> > > -         !smt_possible ||
> > > -         spectre_v2_in_ibrs_mode(spectre_v2_enabled))
> > > +     if (!boot_cpu_has(X86_FEATURE_STIBP) || !smt_possible ||
> > > +         !spectre_v2_user_needs_stibp(spectre_v2_enabled))
> >
> > As pointed out in other discussions, it will be great if can get rid of
> > eIBRS check, and do what the user asked for; or atleast print a warning
> 
> I think I will keep it as pr_info as, with eIBRS, the user does not
> really need STIBP and the mitigation is still effective.

Thats fair.

> > about not setting STIBP bit explicitly.
> 
> That is a bit more complicated as, for now, the user is not really
> exposed to STIBP explicitly yet.

> I would prefer to do it as a follow up and fix this bug first.

On a second thought, STIBP bit being explicitly set or not shouldn't
matter as long as user is getting the STIBP protection that it asked
for.

A print may just help catch some bugs sooner than later.

> It's a bit gnarly and I think we really need to think about the
> options that are exposed to the user [especially in light of Intel /
> AMD subtelties].

With AMD's AutoIBRS support landing in mainline, and both (AutoIBRS and
eIBRS) sharing the same =eibrs mitigation mode, those subtelties becomes
more important.

Following up on Andrew's comment in the other thread, I hope AutoIBRS
does not require setting STIBP explicitly?:

  /sigh so we're still talking about 3 different things then.

  1) Intel's legacy IBRS
  2) AMD's regular IBRS
  3) AMD's AutoIBRS

  which all have different relevant behaviours for userspace.  Just so
  it's written out coherently in at least one place...
  [...]
  For any AMD configuration setting STIBP, there must be an IBPB after
  having set STIBP.   Setting STIBP alone does not evict previously
  created shared predictions.  This one can go subtly wrong for anyone
  who assumes that Intel STIBP and AMD STIBP have the same behaviour.