Date:   Mon, 20 Feb 2023 22:10:28 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     Andrew Cooper <andrew.cooper3@citrix.com>
Cc:     KP Singh <kpsingh@kernel.org>, Dave Hansen <dave.hansen@intel.com>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        linux-kernel@vger.kernel.org, pjt@google.com, evn@google.com,
        tglx@linutronix.de, mingo@redhat.com, dave.hansen@linux.intel.com,
        x86@kernel.org, hpa@zytor.com, peterz@infradead.org,
        pawan.kumar.gupta@linux.intel.com, kim.phillips@amd.com,
        alexandre.chartre@oracle.com, daniel.sneddon@linux.intel.com,
        =?utf-8?B?Sm9zw6k=?= Oliveira <joseloliveira11@gmail.com>,
        Rodrigo Branco <rodrigo@kernelhacking.com>,
        Alexandra Sandulescu <aesa@google.com>,
        Jim Mattson <jmattson@google.com>
Subject: Re: [PATCH RESEND] x86/speculation: Fix user-mode spectre-v2
 protection with KERNEL_IBRS
Message-ID: <Y/PhxDIVsa4WU5gu@zn.tnic>
References: <20230220120127.1975241-1-kpsingh@kernel.org>
 <20230220121350.aidsipw3kd4rsyss@treble>
 <CACYkzJ5L9MLuE5Jz+z5-NJCCrUqTbgKQkXSqnQnCfTD_WNA7_Q@mail.gmail.com>
 <CACYkzJ6n=-tobhX0ONQhjHSgmnNjWnNe_dZnEOGtD8Y6S3RHbA@mail.gmail.com>
 <Y/OETPFTJbGtCADM@zn.tnic>
 <3a65d18f-b218-60e7-4748-f1609eb76fd5@citrix.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3a65d18f-b218-60e7-4748-f1609eb76fd5@citrix.com>
Precedence: bulk

On Mon, Feb 20, 2023 at 07:57:25PM +0000, Andrew Cooper wrote:
> "When IBRS or enhanced IBRS is enabled, STIBP is not needed."
> 
> This is misleading, if not strictly wrong.  The IBRS bit being set
> implies STIBP, which reads differently to "not needed".
> 
> 
> Now - eIBRS is "set once at start of day" which ends up becoming a
> global implicit STIBP.

Right.

> I think we're discussing the legacy IBRS case here.  i.e. what was
> retrofitted in microcode for existing parts?

Any IBRS actually. The one which is *not* the automatic, fire'n'forget
thing.

> The reason why it is "write 1 on each privilege increase, 0 on privilege
> decrease" is because on some CPUs its an inhibit control, and on some
> CPUs is a flush (i.e. its actually IBPB).
> 
> But these same CPUs don't actually have an ability to thread-tag the
> indirect predictor nicely so STIBP is also horribly expensive under the
> hood - so much so that we were firmly recommended to clear STIBP/IBRS
> when going idle so as to reduce the impact on the sibling.

Yap, we do that. And we do the write to 0 for IBRS on exit to
luserspace, probably for very similar reasons.

> IMO the proper way to do this is to set STIBP uniformly depending on
> whether you want it in userspace or not, and treat it logically
> separately to IBRS.  It doesn't hurt (any more) to have both bits set.

So we have this thing:

        /*
         * 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))
                return;

What you propose sounds cleaner but would definitely need more massaging
of this madness code. So I guess we could do only the
enable-STIBP-when-IBRS-enabled thing first and do more cleanups later.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette