Message-ID: <CAGG=3QW2ey2w91TxqJ6tzfJOswhTce2e0QTW7kAWyvxeiO+VNg@mail.gmail.com>
Date:   Thu, 17 Mar 2022 12:45:30 -0700
From:   Bill Wendling <morbo@...gle.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Nick Desaulniers <ndesaulniers@...gle.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Juergen Gross <jgross@...e.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>, llvm@...ts.linux.dev,
        LKML <linux-kernel@...r.kernel.org>,
        linux-toolchains <linux-toolchains@...r.kernel.org>
Subject: Re: [PATCH v5] x86: use builtins to read eflags

On Thu, Mar 17, 2022 at 11:52 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> [ I got cc'd in the middle of the discussion, so I might be missing
> some context or pointing something out that was already discussed ]
>
> On Thu, Mar 17, 2022 at 11:00 AM Nick Desaulniers
> <ndesaulniers@...gle.com> wrote:
> >
> > > >One change you may see due to this patch is the compiler moving the
> > > >"pop %..." instruction away from the "pushf" instruction. This could
> > > >happen if the compiler determines that it could produce better code by
> > > >doing so---e.g. to reduce register pressure. The "gcc -O2" example
> > > >below shows this code movement.
>
> Honestly, that part worries me a _lot_.
>
> Why?
>
> Because clang in particular has already messed up eflags handling
> once, by spilling condition codes (in the form of eflags) onto the
> stack, and then loading them later with a "popf".
>
> And it did so across a function call THAT MODIFIED 'IF'. This was a
> major bug in clang back in 2015 or so, and made it unusable for the
> kernel.
>
> See for example
>
>     https://lore.kernel.org/all/CA+icZUU7y5ATSLV_0TGzi5m5deWADLmAMBkAT32FKGyUWNSJSA@mail.gmail.com/
>
> for some kernel discussion, and separately
>
>     https://lists.llvm.org/pipermail/llvm-dev/2015-July/088774.html
>
> for just llvm discussions.
>
I can't account for LLVM's past sins. That particular issue was fixed
of course. This change will only generate "pushf/popf" (actually just
"pushf" here) only when explicitly used.

> It is perhaps telling that the LLVM discussion I found seems to talk
> more about the performance impact, not about the fact that THE
> GENERATED CODE WAS WRONG.
>
I think that's a bit unfair. There seemed to be a general consensus
that the code was wrong and needed to be fixed. However, the compiler
is also expected to generate the most performant code that it can.

> That compiler bug would basically enable or disable interrupts in
> random places - because clang developers thought that 'eflags' is only
> about the condition codes.
>
> > EFLAGS is defined as the lower 16 bits of FLAGS register, yeah?
>
> No. EFLAGS is a 32-bit register.
>
> A lot of the high bits end up being reserved, but not all of them. AC,
> VIF/VIP are all in the upper 16 bits.
>
> Also:
>
> > So I don't understand your point; the finest grain resolution
> > the compiler has to work with is the whole EFLAGS register, not
> > individual bits like IF, DF, or AC.  (I triple checked this with the
> > maintainer of LLVM's x86 backend)
>
> You can actually operate on EFLAGS at multiple granularities.
>
>  - normal pushf/popf. Don't do it unless you are doing system software.
>
>  - You *can* (but you really *really* should not) operate on only the
> lower 16 bits, with "popfw". Don't do it - it doesn't help, requires
> an operand size override, and is just bad.
>
>  - you can use lahf/sahc to load/store only the arithmetic flags
> into/from AH. Deprecated, and going away, but historically supported.
>
>  - you can obviously use arithmetic and setcc to modify/read the
> arithmetic flags properly.
>
> A compiler should NEVER think that it can access eflags natively with
> pushf/popf. Clang thought it could do so, and clang was WRONG.
>
> The absolutely only case that a compiler should ever worry about and
> do is that last case above: arithmetic instructions that set the
> flags, and 'jcc', 'setcc', 'cmov', 'adc' etc that test it./
>
> Yes, yes, that complete mental breakdown with pushf/popf did get
> fixed, but it really makes me very wary of thinking that we should
> ever use a built-in that compiler writers really fundamentally got so
> wrong before.
>
> What would make me think that you'd get it right now? In user space,
> you'll basically never actually see the whole system flags issues, so
> your test-cases would never work or be very contrieved. You'd have to
> really work at it to see the problems.
>
As pointed out above, the LLVM people agreed with your sentiment here:
never generate "pushf/popf" unless explicitly told to do so. This
patch isn't trying to walk that back, just that when "pushf/popf" is
explicitly asked for it allows LLVM and GCC to generate the best
instruction sequence.

> > > The memory barrier is needed for IF changes, for example.
> >
> > Shouldn't native_irq_disable/native_irq_enable be declaring that they
> > clobber "cc" then?
> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers-and-Scratch-Registers
>
> No. Once again: compilers don't understand system flags in eflags.
>
> In fact, gcc pretty much documents that "cc" clobbers doesn't do
> anything on x86, and isn't needed. It just assumes that the arithmetic
> flags always get clobbered, because on x86 that's pretty much the
> case. They have sometimes been added for documentation purposes in the
> kernel, but that's all they are.
>
> So you can find that "cc" clobber in a few places in the kernel, but
> it's pointless.
>
> The reason pushf/popf need to have a memory clobber is so that the
> compiler will not move it around other things that have memory
> clobbers.
>
> Because memory clobbers are things that compilers *UNDERSTAND*.
>
> Put another way: when you see that "memory" clobber in an inline asm,
> don't think that it means "this modifies or uses memory".
>
> That may be the documentation, and that may be how compiler people
> think about them, but that's just wrong.
>
> What *kernel* people think is "this makes the compiler not do stupid things".
>
> Because compilers really don't understand things like system registers
> and system flags, and the subtle issues they have, and the ordering
> requirements they have. Never have, and never will.
>
> And compilers really don't understand - or care about - things like
> "cc", because compiler people think it's about arithmetic flags, and
> would do things like "oh, I'll just save and restore the flags over
> this since the asm modifies it".
>
> Note that "eflags" is just the tip of the iceberg here. This is true
> for a lot of other cases.
>
> Yes, when you have something like a "disable interrupts", the compiler
> really must not move memory accesses around it, because the interrupt
> flag may in fact be protecting that memory access from becoming a
> deadlock.
>
> But the whole "you can't move _other_ things that you don't even
> understand around this either" is equally important. A "disable
> interrupts" could easily be protecting a "read and modify a CPU MSR
> value" too - no real "memory" access necessarily involved, but
> "memory" is the only way we can tell you "don't move this".
>
And yet that's not guaranteed. From
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html:

```
Note that the compiler can move even volatile asm instructions
relative to other code, including across jump instructions. For
example, on many targets there is a system register that controls the
rounding mode of floating-point operations. Setting it with a volatile
asm statement, as in the following PowerPC example, does not work
reliably.

  asm volatile("mtfsf 255, %0" : : "f" (fpenv));
  sum = x + y;

The solution is to reference the "sum" variable in the asm:

  asm volatile ("mtfsf 255,%1" : "=X" (sum) : "f" (fpenv));
```

Note that the solution _isn't_ to add a "memory" clobber, because it's
not guaranteed to work, as it's explicitly defined to be a read/write
_memory_ barrier, despite what kernel writers wish it would do.

Your assertion that compilers don't know about control registers isn't
exactly true. In the case of "pushf/popf", those instructions know
about the eflags registers. All subsequent instructions that read or
modify eflags also know about it. In essence, the compiler can
determine its own clobber list, which include MSRs.

-bw

> Particularly since both the clang people and gcc people have actively
> been trying to weaken the "volatile" semantics (which would have
> generally been the logically consistent model - thinking if inline asm
> to be visible in the machine model, and not being able to move them
> around for those reasons).
>
> > > This feels like "let's fix LLVM by hacking the kernel in dangerous ways" once again!
> >
> > Even if the behavior of llvm was changed, you'd _still_ get better
> > codegen for _both_ compilers by using the intrinsics.
>
> No.
>
> "moving instructions around a bit" is not better code generation when
> the end result DOES NOT WORK.
>
> And honestly, we have absolutely zero reason to believe that it would work.
>
> If it turns out that "pop to memory" is a big deal for performance,
> let's just change the "=rm" to "=r" and be done with it. Problem
> solved. We already used to do that on the "pushf" side, but these days
> we avoid "popfl" entirely and do a conditional jump over a "sti"
> instead.
>
> Because maybe moving that "pop" by a few inbstructions helps a bit,
> but if you move the "pushf" noticeably, you've just created a big big
> problem.
>
> A bug you've had once already.
>
>                  Linus

Powered by blists - more mailing lists