From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 3D06215AC for ; Fri, 18 Mar 2022 00:31:16 +0000 (UTC) Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 22I0Pv9f001636; Thu, 17 Mar 2022 19:25:57 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 22I0Ptsh001632; Thu, 17 Mar 2022 19:25:55 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 17 Mar 2022 19:25:55 -0500 From: Segher Boessenkool To: Linus Torvalds Cc: Florian Weimer , Nick Desaulniers , "H. Peter Anvin" , Bill Wendling , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" , Nathan Chancellor , Juergen Gross , Peter Zijlstra , Andy Lutomirski , llvm@lists.linux.dev, LKML , linux-toolchains Subject: Re: [PATCH v5] x86: use builtins to read eflags Message-ID: <20220318002555.GP614@gate.crashing.org> References: <20220210223134.233757-1-morbo@google.com> <20220301201903.4113977-1-morbo@google.com> <878rt8gwxa.fsf@oldenburg.str.redhat.com> Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i On Thu, Mar 17, 2022 at 01:36:19PM -0700, Linus Torvalds wrote: > asm volatile("# __raw_save_flags\n\t" > "pushf ; pop %0" > : "=rm" (flags) > : /* no input */ > : "memory"); > 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. There is the problem though that this might extend the stack, which isn't marked up anywhere, so the static checkers do not see the stack overflow, and it won't be noticed until runtime. Or do the checkers consider such cases? > 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. Well, that, or the write of the code didn't consider this, just went "writes memory, so we clobber". > > 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. No ordering wrt any other code, yes. Which is not anything you can solve in only the builtin -- you need to consider the other code that you order with as well, change that code as well. > 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. Yes, and they are much easier to write, and can be written correctly by lookenspeepers who just *have* to twist every knob that they can -- more easily than inline asm anyway, which is unforgiving to the extreme. They are also much easier to read usually, make for cleaner code. They certainly have their place. But they should stay there, too :-) Segher