Re: [PATCH v5] x86: use builtins to read eflags

[Linus Torvalds <torvalds@linux-foundation.org>]

On Thu, Mar 17, 2022 at 1:13 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Linus Torvalds:
>
> > You can actually operate on EFLAGS at multiple granularities.
> >
> >  - normal pushf/popf. Don't do it unless you are doing system software.
>
> There's one exception: PUSHF/twiddle/POPF/PUSHF/compare is the
> recommended sequence to detect CPUID support on i386 (in userspace and
> elsewhere).

Yeah.

I do think that hand-crafted sequences using pushf/popf work. But I
think they should be in one single inline asm statement.

Obviously the kernel use of

        asm volatile("# __raw_save_flags\n\t"
                     "pushf ; pop %0"
                     : "=rm" (flags)
                     : /* no input */
                     : "memory");

is exactly that (and yes, I can well believe that we should make "=rm"
be "=r"), or at least show that the "m" case is much more expensive
some way).

Is it optimal that we put the push/pop right next to each other? No.
But it avoids a *lot* of problems.

And is that "memory" clobber because it modifies the memory location
just below the current stack pointer?

No, not really - outside the kernel that might be an issue, but we
already have to build the kernel with -mno-red-zone, so if the
compiler uses that memory location, that would be a *HUGE* compiler
bug already.

So the "memory" clobber has absolutely nothing to do with the fact
that 'pushf' updates the stack pointer, writes to that location, and
the popf then undoes it.

It's literally because we don't want the compiler to move non-spill
memory accesses around it (or other asm statements wiht memory
clobbers), regardless of the fact that the sequence doesn't really
read or write memory in any way that is relevant to the compiler.

> >  - you can use lahf/sahc to load/store only the arithmetic flags
> > into/from AH. Deprecated, and going away, but historically supported.
>
> And these instructions were missing from the original long mode, but
> they were added back.

They're also actively being deprecated, so the "adding back" ends up
being (maybe) temporary.

> GCC doesn't have barriers in the built-ins (if we are talking about
> __builtin_ia32_readeflags_u64 and __builtin_ia32_writeeflags_u64).  I
> expect they are actually pretty useless, and were merely added for
> completeness of the intrinsics headers.

Yeah, without any kinds of ordering guarantees, I think those builtins
are basically only so in name. They might as well return a random
value - they're not *useful*, because they don't have any defined
behavior.

I mean, we *could* certainly use "read eflags" in the kernel, and yes,
in theory it would be lovely if we didn't have to encode it as a
"pushf/pop" sequence, and the compiler tracked the stack pointer for
us, and perhaps combined it with other stack pointer changes to the
point where the "popf" would never happen, it would just undo the %rsp
change at function exit time.

So yes, a builtin can improve code generation.

But if there is no documented barrier guarantees, how do we know that
the read of the eflags the compiler does doesn't migrate around the
code that sets IF or whatever?

So then we'd need compiler intrinsics for 'sti' and 'cli' too. And
then we'd need to have a way to describe the ordering requirements for
*those* wrt all the other things we do (ie locks or whatever other
code that need to be protected from interrupts).

None of which exists.

And even if it ends up existing in newer compiler versions, we'd have
to wait for the better part of a decade (or more) for that to have
percolated everywhere.

And even _then_, the builtin syntax is often easily clumsier and less
flexible than what we get with inline asm (I'm thinking of the
fundamentally mis-designed memory ordering intrinsics).

                    Linus