linux-kernel - Re: [PATCH] x86: use builtins to read eflags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Dec 2021 12:00:21 -0800
From:   Bill Wendling <morbo@...gle.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H . Peter Anvin" <hpa@...or.com>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Juergen Gross <jgross@...e.com>,
        Andy Lutomirski <luto@...nel.org>, llvm@...ts.linux.dev,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86: use builtins to read eflags

()On Wed, Dec 15, 2021 at 3:26 PM Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Wed, Dec 15, 2021 at 01:18:47PM -0800, Bill Wendling wrote:
> > GCC and Clang both have builtins to read from and write to the
> > EFLAGS register. This allows the compiler to determine the best way
> > to generate the code, which can improve code generation.
>
> Only because clang is still brain-dead wrt "rm" constraints, right?
>
That's one reason. That needs to be addressed, but builtins are
usually better than using inline assembly.

> > Signed-off-by: Bill Wendling <morbo@...gle.com>
> > ---
> >  arch/x86/include/asm/irqflags.h | 24 +++++-------------------
> >  1 file changed, 5 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
> > index c5ce9845c999..574fb44b82f7 100644
> > --- a/arch/x86/include/asm/irqflags.h
> > +++ b/arch/x86/include/asm/irqflags.h
> > @@ -15,25 +15,11 @@
> >   * Interrupt control:
> >   */
> >
> > -/* Declaration required for gcc < 4.9 to prevent -Werror=missing-prototypes */
> > -extern inline unsigned long native_save_fl(void);
> > -extern __always_inline unsigned long native_save_fl(void)
> > -{
> > -     unsigned long flags;
> > -
> > -     /*
> > -      * "=rm" is safe here, because "pop" adjusts the stack before
> > -      * it evaluates its effective address -- this is part of the
> > -      * documented behavior of the "pop" instruction.
> > -      */
> > -     asm volatile("# __raw_save_flags\n\t"
> > -                  "pushf ; pop %0"
> > -                  : "=rm" (flags)
> > -                  : /* no input */
> > -                  : "memory");
> > -
> > -     return flags;
> > -}
> > +#ifdef CONFIG_X86_64
> > +#define native_save_fl()     __builtin_ia32_readeflags_u64()
> > +#else
> > +#define native_save_fl()     __builtin_ia32_readeflags_u32()
> > +#endif
>
> Also note the thing was extern inline, and there's actually an
> out-of-line symbol for them too. The out-of-line thing is explicitly
> using %rax due to paravirt muck.
>
> I'm thinking you wrecked that bit.

If you prefer, it could be written like so:

extern inline unsigned long native_save_fl(void);
extern __always_inline unsigned long native_save_fl(void)
{
#ifdef CONFIG_X86_64
    return __builtin_ia32_readeflags_u64();
#else
    return __builtin_ia32_readeflags_u32();
#endif
}

-bw