Kconfig: Explicitly disable asm goto w/ outputs on gcc-11 (and earlier)

PATCH In review Sean Christopherson 7 participants 24 messages Feb 8, 2024 – Feb 15, 2024

Subsystems (21)

Networking Arm (32-bit) x86 KVM +17 more subsystems

lore

Share & tools

lore

Sean Christopherson

author maintainer

2024-02-08 22:06 UTC #0 lore raw

PATCH 1/1 Kconfig: Explicitly disable asm goto w/ outputs on gcc-11 (and earlier)

Explicitly require gcc-12+ to enable asm goto with outputs on gcc to avoid what is effectively a data corruption bug on gcc-11. As per
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html, "asm goto" is *supposed* be implicitly volatile, but gcc-11 fails to treat it as such. When compiling with -O2, failure to treat the asm block as volatile can result in the entire block being discarded during optimization.

Even worse, forcing "asm volatile goto" keeps the block, but generates completely bogus code.

Hardcode the gcc-12 or later requirement as trying to pipe the assembled output to stdout, e.g. to query the generated code via objdump, doesn't work due to the assembler wanting to seek throughout the output file.

Note, gcc-11 is the first gcc version that supports goto w/ outputs (obviously with a loose definition of "supports").

E.g. given KVM's code sequence:

  vmcs12->guest_pdptr0 = vmcs_read64(GUEST_PDPTR0);
  vmcs12->guest_pdptr1 = vmcs_read64(GUEST_PDPTR1);
  vmcs12->guest_pdptr2 = vmcs_read64(GUEST_PDPTR2);
  vmcs12->guest_pdptr3 = vmcs_read64(GUEST_PDPTR3);

where vmcs_read64() eventually becomes:

	asm_volatile_goto("1: vmread %[field], %[output]\n\t"
			  "jna %l[do_fail]\n\t"

			  _ASM_EXTABLE(1b, %l[do_exception])

			  : [output] "=r" (value)
			  : [field] "r" (field)
			  : "cc"
			  : do_fail, do_exception);

	return value;

do_fail:

	instrumentation_begin();
	vmread_error(field);
	instrumentation_end();
	return 0;

do_exception:

	kvm_spurious_fault();
	return 0;

the sequence of VMREADs should generate:

nopl 0x0(%rax,%rax,1)
mov $0x280a,%eax
vmread %rax,%rax

   jbe    0xffffffff81099849 <sync_vmcs02_to_vmcs12+1929>
   mov    %rax,0xd8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280c,%eax
   vmread %rax,%rax
   jbe    0xffffffff8109982c <sync_vmcs02_to_vmcs12+1900>
   mov    %rax,0xe0(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280e,%eax
   vmread %rax,%rax
   jbe    0xffffffff8109980f <sync_vmcs02_to_vmcs12+1871>
   mov    %rax,0xe8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x2810,%eax
   vmread %rax,%rax
   jbe    0xffffffff810997f2 <sync_vmcs02_to_vmcs12+1842>
   mov    %rax,0xf0(%rbx)
   jmp    0xffffffff81099297 <sync_vmcs02_to_vmcs12+471>

but gcc-11 will omit the asm block for the VMREAD to GUEST_PDPTR3 and skip straight to one of the "return 0" statements:

nopl 0x0(%rax,%rax,1)
mov $0x280a,%r13d
vmread %r13,%r13

   jbe    0xffffffff810996cd <sync_vmcs02_to_vmcs12+1949>
   mov    %r13,0xd8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280c,%r13d
   vmread %r13,%r13
   jbe    0xffffffff810996ae <sync_vmcs02_to_vmcs12+1918>
   mov    %r13,0xe0(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280e,%r13d
   vmread %r13,%r13
   jbe    0xffffffff8109968f <sync_vmcs02_to_vmcs12+1887>
   mov    %r13,0xe8(%rbx)
   nopl   0x0(%rax,%rax,1)
   xor    %r12d,%r12d      <= return 0
   mov    %r12,0xf0(%rbx)  <= store result to vmcs12->guest_pdptr3
   jmp    0xffffffff8109912c <sync_vmcs02_to_vmcs12+508>

and with "volatile" forced, gcc-11 generates the correct-at-first-glance, but terribly broken sequence of:

nopl 0x0(%rax,%rax,1)
mov $0x280a,%r13d
vmread %r13,%r13

   jbe    0xffffffff810999a4 <sync_vmcs02_to_vmcs12+1988>
   mov    %r13,0xd8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280c,%r13d
   vmread %r13,%r13
   jbe    0xffffffff81099985 <sync_vmcs02_to_vmcs12+1957>
   mov    %r13,0xe0(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280e,%r13d
   vmread %r13,%r13
   jbe    0xffffffff81099966 <sync_vmcs02_to_vmcs12+1926>
   mov    %r13,0xe8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x2810,%eax
   vmread %rax,%rax
   jbe    0xffffffff8109994a <sync_vmcs02_to_vmcs12+1898>
   xor    %r12d,%r12d     <= WTF gcc!?!?!
   mov    %r12,0xf0(%rbx)

Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979

Fixes 587f17018a2c ("Kconfig: add config option for asm goto w/ outputs")

Cc Nick Desaulniers <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc [email protected]

Signed-off-by Sean Christopherson <[email protected]>

---

Linus, I'm sending to you directly as this seems urgent enough to apply straightaway, and this obviously affects much more than the build system.

 init/Kconfig | 5 +++++
 1 file changed, 5 insertions(+)

init/Kconfig +5 -0

index deda3d14135b..f4e46d64c1e7 100644
@@ -82,6 +82,11 @@ config CC_CAN_LINK_STATIC
 	default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(USERCFLAGS) $(USERLDFLAGS) $(m32-flag) -static)
 
 config CC_HAS_ASM_GOTO_OUTPUT
+	# gcc-11 has a nasty bug where it doesn't treat asm goto as volatile,
+	# which can result in asm blocks being dropped when compiling with -02.
+	# Note, explicitly forcing volatile doesn't entirely fix the bug!
+	# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979
+	depends on !CC_IS_GCC || GCC_VERSION >= 120000
 	def_bool $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
 
 config CC_HAS_ASM_GOTO_TIED_OUTPUT

base-commit: 047371968ffc470769f541d6933e262dc7085456

Signature · 1 line

2.43.0.687.g38aa6559b0-goog

Nick Desaulniers

2024-02-09 17:03 UTC #1 lore raw

+ Andrew

Andrew, here's the full thread, I cut most of it out:
https://lore.kernel.org/lkml/[email protected]/
.

Quoted 9 lines · Sean Christopherson

Explicitly require gcc-12+ to enable asm goto with outputs on gcc to avoid what is effectively a data corruption bug on gcc-11. As per
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html, "asm goto" is *supposed* be implicitly volatile, but gcc-11 fails to treat it as such. When compiling with -O2, failure to treat the asm block as volatile can result in the entire block being discarded during optimization.
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979

Shoot, I was cc'ed on that bug (I think I noticed it in testing as well, and pointed it out to Andrew on IRC who then cc'ed me to it). I probably should have asked if that would cause issues at some point for the kernel.

I took a look at the test case added in that bug; it doesn't compile until gcc-13 (specifically gcc 13.2, not gcc 13.1). I'm curious since the bug says the fix was backported to gcc-12 and gcc-13. Are there specific versions of those that contain the fix? If so, should Sean amend his version checks below? For instance, was the fix backported to gcc 12.3, so users of gcc 12.2 would still have issues? I can't tell in godbolt since the added test case doesn't compile until gcc 13.2. https://godbolt.org/z/eqaa7dfo3

My implementation in clang still has some issues, too. It's hard to get new control flow right, and there are minimal users outside the kernel to help us validate.

So as much of a fan of feature detection as I am, I admit some of these edge cases aren't perfect, and we may need to result to version detection when such bugs become observable to users.

I'm happy to ack this patch, but I would like to wait for feedback from Andrew as to whether we can be even more precise with avoiding more specific versions of gcc 12 and 13 (if necessary).

Quoted 20 lines

Fixes 587f17018a2c ("Kconfig: add config option for asm goto w/ outputs")
Cc Nick Desaulniers <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Sean Christopherson <[email protected]>
---
Linus, I'm sending to you directly as this seems urgent enough to apply straightaway, and this obviously affects much more than the build system.
 init/Kconfig | 5 +++++
 1 file changed, 5 insertions(+)
init/Kconfig +0 -0
index deda3d14135b..f4e46d64c1e7 100644

init/Kconfig config CC_CAN_LINK_STATIC · line 89

Quoted 8 lines

init/Kconfig +5 -0

@@ -82,6 +82,11 @@ config CC_CAN_LINK_STATIC
        default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(USERCFLAGS) $(USERLDFLAGS) $(m32-flag) -static)
 
 config CC_HAS_ASM_GOTO_OUTPUT
+       # gcc-11 has a nasty bug where it doesn't treat asm goto as volatile,
+       # which can result in asm blocks being dropped when compiling with -02.
+       # Note, explicitly forcing volatile doesn't entirely fix the bug!
+       # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979
+       depends on !CC_IS_GCC || GCC_VERSION >= 120000

LGTM; but we might need to be more specific about avoiding certain min versions of gcc 13 and 12.

Quoted 8 lines

init/Kconfig +0 -0
        def_bool $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
 
 config CC_HAS_ASM_GOTO_TIED_OUTPUT
base-commit: 047371968ffc470769f541d6933e262dc7085456 --
2.43.0.687.g38aa6559b0-goog

Signature · 2 lines

Thanks,
~Nick Desaulniers

Andrew Pinski (QUIC)

2024-02-09 17:14 UTC #2 lore raw

Quoted 11 lines

-----Original Message-----
From: Nick Desaulniers <[email protected]>
Sent: Friday, February 9, 2024 9:03 AM
To: Sean Christopherson <[email protected]>; Andrew Pinski (QUIC) <[email protected]>
Cc: Linus Torvalds <[email protected]>; linux- [email protected]; Masahiro Yamada <[email protected]>; Peter Zijlstra <[email protected]>; [email protected]
Subject: Re: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11 (and earlier)

Quoted 5 lines

+ Andrew
Andrew, here's the full thread, I cut most of it out:
https://lore.kernel.org/lkml/20240208220604.140859-1-
[email protected]/

So the exact versions of GCC where this is/was fixed are:
12.4.0 (not released yet)
13.2.0
14.1.0 (not released yet)

This information is listed in the bug report via the "known to work" field though since 12.4.0 is not released yet, it is listed as 12.3.1. (13.2.0 was not released at the time it was backported so it is listed as 13.1.1). The target milestone in this case lists the lowest version # where the fix is/will be included.

Thanks,
Andrew Pinski

Quoted 89 lines

.
Quoted 9 lines · Sean Christopherson
Explicitly require gcc-12+ to enable asm goto with outputs on gcc to avoid what is effectively a data corruption bug on gcc-11. As per
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html, "asm goto" is *supposed* be implicitly volatile, but gcc-11 fails to treat it as such. When compiling with -O2, failure to treat the asm block as volatile can result in the entire block being discarded during optimization.
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979
Shoot, I was cc'ed on that bug (I think I noticed it in testing as well, and pointed it out to Andrew on IRC who then cc'ed me to it). I probably should have asked if that would cause issues at some point for the kernel.
I took a look at the test case added in that bug; it doesn't compile until gcc-13 (specifically gcc 13.2, not gcc 13.1). I'm curious since the bug says the fix was backported to gcc-12 and gcc-13. Are there specific versions of those that contain the fix? If so, should Sean amend his version checks below? For instance, was the fix backported to gcc 12.3, so users of gcc 12.2 would still have issues? I can't tell in godbolt since the added test case doesn't compile until gcc 13.2. https://godbolt.org/z/eqaa7dfo3
My implementation in clang still has some issues, too. It's hard to get new control flow right, and there are minimal users outside the kernel to help us validate.
So as much of a fan of feature detection as I am, I admit some of these edge cases aren't perfect, and we may need to result to version detection when such bugs become observable to users.
I'm happy to ack this patch, but I would like to wait for feedback from Andrew as to whether we can be even more precise with avoiding more specific versions of gcc 12 and 13 (if necessary).
Quoted 8 lines
Fixes 587f17018a2c ("Kconfig: add config option for asm goto w/
outputs")
Cc Nick Desaulniers <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Sean Christopherson <[email protected]>
Quoted 4 lines
---
Linus, I'm sending to you directly as this seems urgent enough to apply straightaway, and this obviously affects much more than the build
system.
Quoted 9 lines
 init/Kconfig | 5 +++++
 1 file changed, 5 insertions(+)
init/Kconfig index renamed from init/Kconfig +0 -0
deda3d14135b..f4e46d64c1e7 100644
init/Kconfig config CC_CAN_LINK_STATIC · line 87
Quoted 6 lines
init/Kconfig +2 -0
@@ -82,6 +82,11 @@ config CC_CAN_LINK_STATIC
        default $(success,$(srctree)/scripts/cc-can-link.sh $(CC)
 $(CLANG_FLAGS) $(USERCFLAGS) $(USERLDFLAGS) $(m32-flag) -static)
 
 config CC_HAS_ASM_GOTO_OUTPUT
+       # gcc-11 has a nasty bug where it doesn't treat asm goto as volatile,
+       # which can result in asm blocks being dropped when compiling with -
02.
init/Kconfig config CC_CAN_LINK_STATIC · line 89
Quoted 3 lines
init/Kconfig +3 -0
+       # Note, explicitly forcing volatile doesn't entirely fix the bug!
+       # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979
+       depends on !CC_IS_GCC || GCC_VERSION >= 120000
LGTM; but we might need to be more specific about avoiding certain min versions of gcc 13 and 12.
Quoted 10 lines
init/Kconfig +0 -0
        def_bool $(success,echo 'int foo(int x) { asm goto ("":
"=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
config CC_HAS_ASM_GOTO_TIED_OUTPUT
base-commit: 047371968ffc470769f541d6933e262dc7085456 --
2.43.0.687.g38aa6559b0-goog
--
Thanks,
~Nick Desaulniers

Linus Torvalds

2024-02-09 17:56 UTC #3 lore raw

Quoted 5 lines · Andrew Pinski (QUIC)

So the exact versions of GCC where this is/was fixed are:
12.4.0 (not released yet)
13.2.0
14.1.0 (not released yet)

Looking at the patch that the bugzilla says is the fix, it *looks* like it's just the "mark volatile" that is missing.

But Sean says that even if we mark "asm goto" as volatile manually, it still fails.

So there seems to be something else going on in addition to just the volatile.

Side note: the reason we have that "asm_volatile_goto()" define in the kernel is that we *used* to have a _different_ workaround for a gcc bug in this area:

/*
* GCC 'asm goto' miscompiles certain code sequences:
*
* http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670
*
* Work it around via a compiler barrier quirk suggested by Jakub Jelinek. *
* (asm goto is automatically volatile - the naming reflects this.) */
#define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)

and looking at that (old) bugzilla there seems to be a lot of "seems to be fixed", but it's not entirely clear.

We've removed that workaround in commit 43c249ea0b1e ("compiler-gcc.h:
remove ancient workaround for gcc PR 58670"), I'm wondering if maybe that removal was a bit optimistic.

Linus

Sean Christopherson

author maintainer

2024-02-09 18:43 UTC #4 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 5 lines · Andrew Pinski (QUIC)
So the exact versions of GCC where this is/was fixed are:
12.4.0 (not released yet)
13.2.0
14.1.0 (not released yet)

Quoted 7 lines

Looking at the patch that the bugzilla says is the fix, it *looks* like it's just the "mark volatile" that is missing.
But Sean says that even if we mark "asm goto" as volatile manually, it still fails.
So there seems to be something else going on in addition to just the volatile.

Aha! Yeah, there's a second bug that set things up so that the "not implicitly volatile" bug could rear its head. (And now I feel less bad for not suspecting the compiler sooner, because it didn't occur to me that gcc could possibly think the asm blob had no used outputs).

With "volatile" forced, gcc generates code for the asm blob, but doesn't actually consume the output of the VMREAD. As a result, the optimization pass sees the unused output and throws it away because the blob isn't treated as volatile.

vmread %rax,%rax <= output register is unused

   jbe    0xffffffff8109994a <sync_vmcs02_to_vmcs12+1898>
   xor    %r12d,%r12d     <= one of the "return 0" statements
   mov    %r12,0xf0(%rbx) <= store the output

Quoted 15 lines

Side note: the reason we have that "asm_volatile_goto()" define in the kernel is that we *used* to have a _different_ workaround for a gcc bug in this area:
/*
* GCC 'asm goto' miscompiles certain code sequences:
*
* http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670
*
* Work it around via a compiler barrier quirk suggested by Jakub Jelinek. *
* (asm goto is automatically volatile - the naming reflects this.) */
#define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)

Quoted 6 lines

and looking at that (old) bugzilla there seems to be a lot of "seems to be fixed", but it's not entirely clear.
We've removed that workaround in commit 43c249ea0b1e ("compiler-gcc.h:
remove ancient workaround for gcc PR 58670"), I'm wondering if maybe that removal was a bit optimistic.

FWIW, reverting that does restore correct behavior on gcc-11.

Note, this is 100% reproducible across multiple systems, though AFAICT it's somewhat dependent on the .config. Holler if anyone wants the .config.

Nick Desaulniers

2024-02-09 18:55 UTC #5 lore raw

Quoted 23 lines · Sean Christopherson

Quoted 8 lines · Linus Torvalds
Quoted 5 lines · Andrew Pinski (QUIC)
So the exact versions of GCC where this is/was fixed are:
12.4.0 (not released yet)
13.2.0
14.1.0 (not released yet)
Quoted 7 lines
Looking at the patch that the bugzilla says is the fix, it *looks* like it's just the "mark volatile" that is missing.
But Sean says that even if we mark "asm goto" as volatile manually, it still fails.
So there seems to be something else going on in addition to just the volatile.
Aha! Yeah, there's a second bug that set things up so that the "not implicitly volatile" bug could rear its head. (And now I feel less bad for not suspecting the compiler sooner, because it didn't occur to me that gcc could possibly think the asm blob had no used outputs).

Quoted 28 lines

With "volatile" forced, gcc generates code for the asm blob, but doesn't actually consume the output of the VMREAD. As a result, the optimization pass sees the unused output and throws it away because the blob isn't treated as volatile.
vmread %rax,%rax <= output register is unused
   jbe    0xffffffff8109994a <sync_vmcs02_to_vmcs12+1898>
   xor    %r12d,%r12d     <= one of the "return 0" statements
   mov    %r12,0xf0(%rbx) <= store the output
Quoted 19 lines
Side note: the reason we have that "asm_volatile_goto()" define in the kernel is that we *used* to have a _different_ workaround for a gcc bug in this area:
/*
* GCC 'asm goto' miscompiles certain code sequences:
*
* http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670
*
* Work it around via a compiler barrier quirk suggested by Jakub Jelinek. *
* (asm goto is automatically volatile - the naming reflects this.) */
#define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
and looking at that (old) bugzilla there seems to be a lot of "seems to be fixed", but it's not entirely clear.
We've removed that workaround in commit 43c249ea0b1e ("compiler-gcc.h:

Interesting, I thought I had proposed removing that earlier and Linus had yelled about doing so.
https://lore.kernel.org/lkml/[email protected]/
seems like in ~2018 I was trying to, but can't seem to find the thread where Linus pushed back.

Quoted 4 lines

Quoted 2 lines
remove ancient workaround for gcc PR 58670"), I'm wondering if maybe that removal was a bit optimistic.
FWIW, reverting that does restore correct behavior on gcc-11.

And also pessimizes all asm gotos (for gcc) including ones that don't contain output as described in 43c249ea0b1e. The version checks would at least not pessimize those.

Quoted 3 lines

Note, this is 100% reproducible across multiple systems, though AFAICT it's somewhat dependent on the .config. Holler if anyone wants the .config.

Signature · 2 lines

Thanks,
~Nick Desaulniers

Linus Torvalds

2024-02-09 19:01 UTC #6 lore raw

Quoted 4 lines · Nick Desaulniers

And also pessimizes all asm gotos (for gcc) including ones that don't contain output as described in 43c249ea0b1e. The version checks would at least not pessimize those.

Yeah, no, we should limit that workaround only to the asm goto with outputs case.

We should also probably get rid of the existing "asm_volatile_goto()" macro name entirely. That name was always pretty horrific, in that it didn't even mark the asm as volatile even in the case where it did anything.

So the name of that macro made little sense, and the new workaround should be only for asm goto with outputs. So I'd suggest jmaking a new macro with that name:

   #define asm_goto_output(x...)

and on gcc use that old workaround, and on clang just make it be a plain "asm goto".

Hmm?

Linus

Nick Desaulniers

2024-02-09 19:20 UTC #7 lore raw

Quoted 7 lines · Linus Torvalds

Quoted 4 lines · Nick Desaulniers
And also pessimizes all asm gotos (for gcc) including ones that don't contain output as described in 43c249ea0b1e. The version checks would at least not pessimize those.

Quoted 7 lines

Yeah, no, we should limit that workaround only to the asm goto with outputs case.
We should also probably get rid of the existing "asm_volatile_goto()" macro name entirely. That name was always pretty horrific, in that it didn't even mark the asm as volatile even in the case where it did anything.

+1.

43c249ea0b1e could have done so, but I'm guessing "tree wide changes in Linux are not fun" was perhaps the reason it wasn't done so then.

I also think folks are too aggressive putting volatile on asm statements that might not need them; it's definitely less cognitive burden to just always put volatile on inline asm but I suspect that's leaving some performance on the floor in certain cases. (I had a patch to clang to warn when the volatile was unnecessary in cases where it was explicit, but that was shot down in code review as being harassing to users).

Quoted 11 lines

So the name of that macro made little sense, and the new workaround should be only for asm goto with outputs. So I'd suggest jmaking a new macro with that name:
   #define asm_goto_output(x...)
and on gcc use that old workaround, and on clang just make it be a plain "asm goto".
Hmm?

Thinking through the tradeoffs, the Kconfig approach would pessimize GCC versions with the "lack of implicit volatile" bug to not use asm goto w/ outputs at all.

Having a new macro would just make the volatile qualifier explicit, which is a no-op on gcc versions that don't contain the bug (while allowing them to use asm goto with outputs, which is probably better for codegen).

So I agree a new macro seems better than disabling things for kconfig. (Assuming the only issue is the need to make volatile explicit for a few GCC versions; it's not clear to me from Sean's latest response if there's more than one bug here). I'm not signing up to shave either yak though.

Signature · 2 lines

Thanks,
~Nick Desaulniers

Linus Torvalds

2024-02-09 20:39 UTC #8 lore raw

In reply to Linus Torvalds

Quoted 6 lines · Linus Torvalds

We should also probably get rid of the existing "asm_volatile_goto()" macro name entirely. That name was always pretty horrific, in that it didn't even mark the asm as volatile even in the case where it did anything.

Quoted 8 lines

So the name of that macro made little sense, and the new workaround should be only for asm goto with outputs. So I'd suggest jmaking a new macro with that name:
   #define asm_goto_output(x...)
and on gcc use that old workaround, and on clang just make it be a plain "asm goto".

So here's a suggested patch that does this.

It's largely done with "git grep" and "sed -i", plus some manual fixups for the (few) cases where we have outputs.

It looks superficially sane to me, and it passed an allmodconfig build with gcc, but I'm not going to claim that it is really tested.

Sean? Does this work for the case you noticed?

Basically this gets rid of the old "asm_volatile_goto()" entirely as useless, but replaces it with "asm_goto_outputs()" for the places where we have outputs.

And then for gcc, it makes those cases

(a) use "asm volatile goto" to fix the fact that some versions of gcc
will have missed the "volatile"

(b) adds that extra empty asm as a second barrier after the "real"
asm goto statement

That (b) is very much voodoo programming, but it matches the old magic barrier thing that Jakub Jelinek suggested for the really *old* gcc bug wrt plain (non-output) "asm goto". The underlying bug for _that_ was fixed long ago:

    http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670

We removed that for plain "asm goto" workaround a couple of years ago, so "asm_volatile_goto()" has been a no-op since June 2022, but this now resurrects that hack for the output case.

I'm not loving it, but Sean seemed to confirm that it fixes the code generation problem, so ...

Adding Uros to the cc, since he is both involved with gcc and with the previous asm goto workaround removal, so maybe he has other suggestions. Uros, see

    https://lore.kernel.org/all/[email protected]/

for background.

Also adding Jakub since I'm re-using the hack he suggested for a different - but similar - case. He may have strong opinions too, and may object to that particular monkey-see-monkey-do voodoo programming.

Linus

 arch/arc/include/asm/jump_label.h           |  4 ++--
 arch/arm/include/asm/jump_label.h           |  4 ++--
 arch/arm64/include/asm/alternative-macros.h |  4 ++--
 arch/arm64/include/asm/jump_label.h         |  4 ++--
 arch/csky/include/asm/jump_label.h          |  4 ++--
 arch/loongarch/include/asm/jump_label.h     |  4 ++--
 arch/mips/include/asm/jump_label.h          |  4 ++--
 arch/parisc/include/asm/jump_label.h        |  4 ++--
 arch/powerpc/include/asm/jump_label.h       |  4 ++--
 arch/powerpc/include/asm/uaccess.h          | 12 ++++++------
 arch/powerpc/kernel/irq_64.c                |  2 +-
 arch/riscv/include/asm/arch_hweight.h       |  4 ++--
 arch/riscv/include/asm/bitops.h             |  8 ++++----
 arch/riscv/include/asm/checksum.h           |  2 +-
 arch/riscv/include/asm/cpufeature.h         |  4 ++--
 arch/riscv/include/asm/jump_label.h         |  4 ++--
 arch/riscv/lib/csum.c                       | 10 +++++-----
 arch/s390/include/asm/jump_label.h          |  4 ++--
 arch/sparc/include/asm/jump_label.h         |  4 ++--
 arch/um/include/asm/cpufeature.h            |  2 +-
 arch/x86/include/asm/cpufeature.h           |  2 +-
 arch/x86/include/asm/jump_label.h           |  6 +++---
 arch/x86/include/asm/rmwcc.h                |  2 +-
 arch/x86/include/asm/special_insns.h        |  2 +-
 arch/x86/include/asm/uaccess.h              | 10 +++++-----
 arch/x86/kvm/svm/svm_ops.h                  |  6 +++---
 arch/x86/kvm/vmx/vmx.c                      |  4 ++--
 arch/x86/kvm/vmx/vmx_ops.h                  |  6 +++---
 arch/xtensa/include/asm/jump_label.h        |  4 ++--
 include/linux/compiler-gcc.h                | 19 +++++++++++++++++++
 include/linux/compiler_types.h              |  4 ++--
 net/netfilter/nft_set_pipapo_avx2.c         |  2 +-
 samples/bpf/asm_goto_workaround.h           |  8 ++++----
 tools/arch/x86/include/asm/rmwcc.h          |  2 +-
 tools/include/linux/compiler_types.h        |  4 ++--
 35 files changed, 96 insertions(+), 77 deletions(-)

arch/arc/include/asm/jump_label.h +2 -2

index 9d9618079739..a339223d9e05 100644
@@ -31,7 +31,7 @@
 static __always_inline bool arch_static_branch(struct static_key *key,
 					       bool branch)
 {
-	asm_volatile_goto(".balign "__stringify(JUMP_LABEL_NOP_SIZE)"	\n"
+	asm goto(".balign "__stringify(JUMP_LABEL_NOP_SIZE)"		\n"
 		 "1:							\n"
 		 "nop							\n"
 		 ".pushsection __jump_table, \"aw\"			\n"

@@ -47,7 +47,7 @@ static __always_inline bool arch_static_branch(struct static_key *key,
 static __always_inline bool arch_static_branch_jump(struct static_key *key,
 						    bool branch)
 {
-	asm_volatile_goto(".balign "__stringify(JUMP_LABEL_NOP_SIZE)"	\n"
+	asm goto(".balign "__stringify(JUMP_LABEL_NOP_SIZE)"		\n"
 		 "1:							\n"
 		 "b %l[l_yes]						\n"
 		 ".pushsection __jump_table, \"aw\"			\n"

arch/arm/include/asm/jump_label.h +2 -2

index e12d7d096fc0..e4eb54f6cd9f 100644
@@ -11,7 +11,7 @@
 
 static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 		 WASM(nop) "\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".word 1b, %l[l_yes], %c0\n\t"

@@ -25,7 +25,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 
 static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 		 WASM(b) " %l[l_yes]\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".word 1b, %l[l_yes], %c0\n\t"

arch/arm64/include/asm/alternative-macros.h +2 -2

index 210bb43cff2c..d328f549b1a6 100644
@@ -229,7 +229,7 @@ alternative_has_cap_likely(const unsigned long cpucap)
 	if (!cpucap_is_possible(cpucap))
 		return false;
 
-	asm_volatile_goto(
+	asm goto(
 	ALTERNATIVE_CB("b	%l[l_no]", %[cpucap], alt_cb_patch_nops)
 	:
 	: [cpucap] "i" (cpucap)

@@ -247,7 +247,7 @@ alternative_has_cap_unlikely(const unsigned long cpucap)
 	if (!cpucap_is_possible(cpucap))
 		return false;
 
-	asm_volatile_goto(
+	asm goto(
 	ALTERNATIVE("nop", "b	%l[l_yes]", %[cpucap])
 	:
 	: [cpucap] "i" (cpucap)

arch/arm64/include/asm/jump_label.h +2 -2

index 48ddc0f45d22..6aafbb789991 100644
@@ -18,7 +18,7 @@
 static __always_inline bool arch_static_branch(struct static_key * const key,
 					       const bool branch)
 {
-	asm_volatile_goto(
+	asm goto(
 		"1:	nop					\n\t"
 		 "	.pushsection	__jump_table, \"aw\"	\n\t"
 		 "	.align		3			\n\t"

@@ -35,7 +35,7 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
 static __always_inline bool arch_static_branch_jump(struct static_key * const key,
 						    const bool branch)
 {
-	asm_volatile_goto(
+	asm goto(
 		"1:	b		%l[l_yes]		\n\t"
 		 "	.pushsection	__jump_table, \"aw\"	\n\t"
 		 "	.align		3			\n\t"

arch/csky/include/asm/jump_label.h +2 -2

index 98a3f4b168bd..ef2e37a10a0f 100644
@@ -12,7 +12,7 @@
 static __always_inline bool arch_static_branch(struct static_key *key,
 					       bool branch)
 {
-	asm_volatile_goto(
+	asm goto(
 		"1:	nop32					\n"
 		"	.pushsection	__jump_table, \"aw\"	\n"
 		"	.align		2			\n"

@@ -29,7 +29,7 @@ static __always_inline bool arch_static_branch(struct static_key *key,
 static __always_inline bool arch_static_branch_jump(struct static_key *key,
 						    bool branch)
 {
-	asm_volatile_goto(
+	asm goto(
 		"1:	bsr32		%l[label]		\n"
 		"	.pushsection	__jump_table, \"aw\"	\n"
 		"	.align		2			\n"

arch/loongarch/include/asm/jump_label.h +2 -2

index 3cea299a5ef5..29acfe3de3fa 100644
@@ -22,7 +22,7 @@
 
 static __always_inline bool arch_static_branch(struct static_key * const key, const bool branch)
 {
-	asm_volatile_goto(
+	asm goto(
 		"1:	nop			\n\t"
 		JUMP_TABLE_ENTRY
 		:  :  "i"(&((char *)key)[branch]) :  : l_yes);

@@ -35,7 +35,7 @@ static __always_inline bool arch_static_branch(struct static_key * const key, co
 
 static __always_inline bool arch_static_branch_jump(struct static_key * const key, const bool branch)
 {
-	asm_volatile_goto(
+	asm goto(
 		"1:	b	%l[l_yes]	\n\t"
 		JUMP_TABLE_ENTRY
 		:  :  "i"(&((char *)key)[branch]) :  : l_yes);

arch/mips/include/asm/jump_label.h +2 -2

index 081be98c71ef..ff5d388502d4 100644
@@ -39,7 +39,7 @@ extern void jump_label_apply_nops(struct module *mod);
 
 static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\t" B_INSN " 2f\n\t"
+	asm goto("1:\t" B_INSN " 2f\n\t"
 		"2:\t.insn\n\t"
 		".pushsection __jump_table,  \"aw\"\n\t"
 		WORD_INSN " 1b, %l[l_yes], %0\n\t"

@@ -53,7 +53,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 
 static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\t" J_INSN " %l[l_yes]\n\t"
+	asm goto("1:\t" J_INSN " %l[l_yes]\n\t"
 		".pushsection __jump_table,  \"aw\"\n\t"
 		WORD_INSN " 1b, %l[l_yes], %0\n\t"
 		".popsection\n\t"

arch/parisc/include/asm/jump_label.h +2 -2

index 94428798b6aa..317ebc5edc9f 100644
@@ -12,7 +12,7 @@
 
 static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 		 "nop\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".align %1\n\t"

@@ -29,7 +29,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 
 static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 		 "b,n %l[l_yes]\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".align %1\n\t"

arch/powerpc/include/asm/jump_label.h +2 -2

index 93ce3ec25387..2f2a86ed2280 100644
@@ -17,7 +17,7 @@
 
 static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 		 "nop # arch_static_branch\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".long 1b - ., %l[l_yes] - .\n\t"

@@ -32,7 +32,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 
 static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 		 "b %l[l_yes] # arch_static_branch_jump\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".long 1b - ., %l[l_yes] - .\n\t"

arch/powerpc/include/asm/uaccess.h +6 -6

index f1f9890f50d3..de10437fd206 100644
@@ -74,7 +74,7 @@ __pu_failed:							\
 /* -mprefixed can generate offsets beyond range, fall back hack */
 #ifdef CONFIG_PPC_KERNEL_PREFIXED
 #define __put_user_asm_goto(x, addr, label, op)			\
-	asm_volatile_goto(					\
+	asm goto(					\
 		"1:	" op " %0,0(%1)	# put_user\n"		\
 		EX_TABLE(1b, %l2)				\
 		:						\

@@ -83,7 +83,7 @@ __pu_failed:							\
 		: label)
 #else
 #define __put_user_asm_goto(x, addr, label, op)			\
-	asm_volatile_goto(					\
+	asm goto(					\
 		"1:	" op "%U1%X1 %0,%1	# put_user\n"	\
 		EX_TABLE(1b, %l2)				\
 		:						\

@@ -97,7 +97,7 @@ __pu_failed:							\
 	__put_user_asm_goto(x, ptr, label, "std")
 #else /* __powerpc64__ */
 #define __put_user_asm2_goto(x, addr, label)			\
-	asm_volatile_goto(					\
+	asm goto(					\
 		"1:	stw%X1 %0, %1\n"			\
 		"2:	stw%X1 %L0, %L1\n"			\
 		EX_TABLE(1b, %l2)				\

@@ -146,7 +146,7 @@ do {								\
 /* -mprefixed can generate offsets beyond range, fall back hack */
 #ifdef CONFIG_PPC_KERNEL_PREFIXED
 #define __get_user_asm_goto(x, addr, label, op)			\
-	asm_volatile_goto(					\
+	asm_goto_output(					\
 		"1:	"op" %0,0(%1)	# get_user\n"		\
 		EX_TABLE(1b, %l2)				\
 		: "=r" (x)					\

@@ -155,7 +155,7 @@ do {								\
 		: label)
 #else
 #define __get_user_asm_goto(x, addr, label, op)			\
-	asm_volatile_goto(					\
+	asm_goto_output(					\
 		"1:	"op"%U1%X1 %0, %1	# get_user\n"	\
 		EX_TABLE(1b, %l2)				\
 		: "=r" (x)					\

@@ -169,7 +169,7 @@ do {								\
 	__get_user_asm_goto(x, addr, label, "ld")
 #else /* __powerpc64__ */
 #define __get_user_asm2_goto(x, addr, label)			\
-	asm_volatile_goto(					\
+	asm_goto_output(					\
 		"1:	lwz%X1 %0, %1\n"			\
 		"2:	lwz%X1 %L0, %L1\n"			\
 		EX_TABLE(1b, %l2)				\

arch/powerpc/kernel/irq_64.c +1 -1

index 938e66829eae..d5c48d1b0a31 100644
@@ -230,7 +230,7 @@ notrace __no_kcsan void arch_local_irq_restore(unsigned long mask)
 	 * This allows interrupts to be unmasked without hard disabling, and
 	 * also without new hard interrupts coming in ahead of pending ones.
 	 */
-	asm_volatile_goto(
+	asm goto(
 "1:					\n"
 "		lbz	9,%0(13)	\n"
 "		cmpwi	9,0		\n"

arch/riscv/include/asm/arch_hweight.h +2 -2

index c20236a0725b..85b2c443823e 100644
@@ -20,7 +20,7 @@
 static __always_inline unsigned int __arch_hweight32(unsigned int w)
 {
 #ifdef CONFIG_RISCV_ISA_ZBB
-	asm_volatile_goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
+	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
 				      RISCV_ISA_EXT_ZBB, 1)
 			  : : : : legacy);

@@ -51,7 +51,7 @@ static inline unsigned int __arch_hweight8(unsigned int w)
 static __always_inline unsigned long __arch_hweight64(__u64 w)
 {
 # ifdef CONFIG_RISCV_ISA_ZBB
-	asm_volatile_goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
+	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
 				      RISCV_ISA_EXT_ZBB, 1)
 			  : : : : legacy);

arch/riscv/include/asm/bitops.h +4 -4

index 9ffc35537024..329d8244a9b3 100644
@@ -39,7 +39,7 @@ static __always_inline unsigned long variable__ffs(unsigned long word)
 {
 	int num;
 
-	asm_volatile_goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
+	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
 				      RISCV_ISA_EXT_ZBB, 1)
 			  : : : : legacy);

@@ -95,7 +95,7 @@ static __always_inline unsigned long variable__fls(unsigned long word)
 {
 	int num;
 
-	asm_volatile_goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
+	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
 				      RISCV_ISA_EXT_ZBB, 1)
 			  : : : : legacy);

@@ -154,7 +154,7 @@ static __always_inline int variable_ffs(int x)
 	if (!x)
 		return 0;
 
-	asm_volatile_goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
+	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
 				      RISCV_ISA_EXT_ZBB, 1)
 			  : : : : legacy);

@@ -209,7 +209,7 @@ static __always_inline int variable_fls(unsigned int x)
 	if (!x)
 		return 0;
 
-	asm_volatile_goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
+	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
 				      RISCV_ISA_EXT_ZBB, 1)
 			  : : : : legacy);

arch/riscv/include/asm/checksum.h +1 -1

index a5b60b54b101..88e6f1499e88 100644
@@ -53,7 +53,7 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl)
 	    IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) {
 		unsigned long fold_temp;
 
-		asm_volatile_goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
+		asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
 					      RISCV_ISA_EXT_ZBB, 1)
 		    :
 		    :

arch/riscv/include/asm/cpufeature.h +2 -2

index 5a626ed2c47a..0bd11862b760 100644
@@ -80,7 +80,7 @@ riscv_has_extension_likely(const unsigned long ext)
 			   "ext must be < RISCV_ISA_EXT_MAX");
 
 	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) {
-		asm_volatile_goto(
+		asm goto(
 		ALTERNATIVE("j	%l[l_no]", "nop", 0, %[ext], 1)
 		:
 		: [ext] "i" (ext)

@@ -103,7 +103,7 @@ riscv_has_extension_unlikely(const unsigned long ext)
 			   "ext must be < RISCV_ISA_EXT_MAX");
 
 	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) {
-		asm_volatile_goto(
+		asm goto(
 		ALTERNATIVE("nop", "j	%l[l_yes]", 0, %[ext], 1)
 		:
 		: [ext] "i" (ext)

arch/riscv/include/asm/jump_label.h +2 -2

index 14a5ea8d8ef0..4a35d787c019 100644
@@ -17,7 +17,7 @@
 static __always_inline bool arch_static_branch(struct static_key * const key,
 					       const bool branch)
 {
-	asm_volatile_goto(
+	asm goto(
 		"	.align		2			\n\t"
 		"	.option push				\n\t"
 		"	.option norelax				\n\t"

@@ -39,7 +39,7 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
 static __always_inline bool arch_static_branch_jump(struct static_key * const key,
 						    const bool branch)
 {
-	asm_volatile_goto(
+	asm goto(
 		"	.align		2			\n\t"
 		"	.option push				\n\t"
 		"	.option norelax				\n\t"

arch/riscv/lib/csum.c +5 -5

index af3df5274ccb..74af3ab520b6 100644
@@ -53,7 +53,7 @@ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
 		 * support, so nop when Zbb is available and jump when Zbb is
 		 * not available.
 		 */
-		asm_volatile_goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
+		asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
 					      RISCV_ISA_EXT_ZBB, 1)
 				  :
 				  :

@@ -170,7 +170,7 @@ do_csum_with_alignment(const unsigned char *buff, int len)
 		 * support, so nop when Zbb is available and jump when Zbb is
 		 * not available.
 		 */
-		asm_volatile_goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
+		asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
 					      RISCV_ISA_EXT_ZBB, 1)
 				  :
 				  :

@@ -178,7 +178,7 @@ do_csum_with_alignment(const unsigned char *buff, int len)
 				  : no_zbb);
 
 #ifdef CONFIG_32BIT
-		asm_volatile_goto(".option push			\n\
+		asm_goto_output(".option push			\n\
 		.option arch,+zbb				\n\
 			rori	%[fold_temp], %[csum], 16	\n\
 			andi	%[offset], %[offset], 1		\n\

@@ -193,7 +193,7 @@ do_csum_with_alignment(const unsigned char *buff, int len)
 
 		return (unsigned short)csum;
 #else /* !CONFIG_32BIT */
-		asm_volatile_goto(".option push			\n\
+		asm_goto_output(".option push			\n\
 		.option arch,+zbb				\n\
 			rori	%[fold_temp], %[csum], 32	\n\
 			add	%[csum], %[fold_temp], %[csum]	\n\

@@ -257,7 +257,7 @@ do_csum_no_alignment(const unsigned char *buff, int len)
 		 * support, so nop when Zbb is available and jump when Zbb is
 		 * not available.
 		 */
-		asm_volatile_goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
+		asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
 					      RISCV_ISA_EXT_ZBB, 1)
 				  :
 				  :

arch/s390/include/asm/jump_label.h +2 -2

index 895f774bbcc5..bf78cf381dfc 100644
@@ -25,7 +25,7 @@
  */
 static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("0:	brcl 0,%l[label]\n"
+	asm goto("0:	brcl 0,%l[label]\n"
 			  ".pushsection __jump_table,\"aw\"\n"
 			  ".balign	8\n"
 			  ".long	0b-.,%l[label]-.\n"

@@ -39,7 +39,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 
 static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("0:	brcl 15,%l[label]\n"
+	asm goto("0:	brcl 15,%l[label]\n"
 			  ".pushsection __jump_table,\"aw\"\n"
 			  ".balign	8\n"
 			  ".long	0b-.,%l[label]-.\n"

arch/sparc/include/asm/jump_label.h +2 -2

index 94eb529dcb77..2718cbea826a 100644
@@ -10,7 +10,7 @@
 
 static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 		 "nop\n\t"
 		 "nop\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"

@@ -26,7 +26,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 
 static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 		 "b %l[l_yes]\n\t"
 		 "nop\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"

arch/um/include/asm/cpufeature.h +1 -1

index 4b6d1b526bc1..66fe06db872f 100644
@@ -75,7 +75,7 @@ extern void setup_clear_cpu_cap(unsigned int bit);
  */
 static __always_inline bool _static_cpu_has(u16 bit)
 {
-	asm_volatile_goto("1: jmp 6f\n"
+	asm goto("1: jmp 6f\n"
 		 "2:\n"
 		 ".skip -(((5f-4f) - (2b-1b)) > 0) * "
 			 "((5f-4f) - (2b-1b)),0x90\n"

arch/x86/include/asm/cpufeature.h +1 -1

index a26bebbdff87..a1273698fc43 100644
@@ -168,7 +168,7 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
  */
 static __always_inline bool _static_cpu_has(u16 bit)
 {
-	asm_volatile_goto(
+	asm goto(
 		ALTERNATIVE_TERNARY("jmp 6f", %P[feature], "", "jmp %l[t_no]")
 		".pushsection .altinstr_aux,\"ax\"\n"
 		"6:\n"

arch/x86/include/asm/jump_label.h +3 -3

index 071572e23d3a..cbbef32517f0 100644
@@ -24,7 +24,7 @@
 
 static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("1:"
+	asm goto("1:"
 		"jmp %l[l_yes] # objtool NOPs this \n\t"
 		JUMP_TABLE_ENTRY
 		: :  "i" (key), "i" (2 | branch) : : l_yes);

@@ -38,7 +38,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 
 static __always_inline bool arch_static_branch(struct static_key * const key, const bool branch)
 {
-	asm_volatile_goto("1:"
+	asm goto("1:"
 		".byte " __stringify(BYTES_NOP5) "\n\t"
 		JUMP_TABLE_ENTRY
 		: :  "i" (key), "i" (branch) : : l_yes);

@@ -52,7 +52,7 @@ static __always_inline bool arch_static_branch(struct static_key * const key, co
 
 static __always_inline bool arch_static_branch_jump(struct static_key * const key, const bool branch)
 {
-	asm_volatile_goto("1:"
+	asm goto("1:"
 		"jmp %l[l_yes]\n\t"
 		JUMP_TABLE_ENTRY
 		: :  "i" (key), "i" (branch) : : l_yes);

arch/x86/include/asm/rmwcc.h +1 -1

index 4b081e0d3306..363266cbcada 100644
@@ -13,7 +13,7 @@
 #define __GEN_RMWcc(fullop, _var, cc, clobbers, ...)			\
 ({									\
 	bool c = false;							\
-	asm_volatile_goto (fullop "; j" #cc " %l[cc_label]"		\
+	asm goto (fullop "; j" #cc " %l[cc_label]"		\
 			: : [var] "m" (_var), ## __VA_ARGS__		\
 			: clobbers : cc_label);				\
 	if (0) {							\

arch/x86/include/asm/special_insns.h +1 -1

index d6cd9344f6c7..48f8dd47cf68 100644
@@ -205,7 +205,7 @@ static inline void clwb(volatile void *__p)
 #ifdef CONFIG_X86_USER_SHADOW_STACK
 static inline int write_user_shstk_64(u64 __user *addr, u64 val)
 {
-	asm_volatile_goto("1: wrussq %[val], (%[addr])\n"
+	asm goto("1: wrussq %[val], (%[addr])\n"
 			  _ASM_EXTABLE(1b, %l[fail])
 			  :: [addr] "r" (addr), [val] "r" (val)
 			  :: fail);

arch/x86/include/asm/uaccess.h +5 -5

index 5c367c1290c3..237dc8cdd12b 100644
@@ -133,7 +133,7 @@ extern int __get_user_bad(void);
 
 #ifdef CONFIG_X86_32
 #define __put_user_goto_u64(x, addr, label)			\
-	asm_volatile_goto("\n"					\
+	asm goto("\n"					\
 		     "1:	movl %%eax,0(%1)\n"		\
 		     "2:	movl %%edx,4(%1)\n"		\
 		     _ASM_EXTABLE_UA(1b, %l2)			\

@@ -295,7 +295,7 @@ do {									\
 } while (0)
 
 #define __get_user_asm(x, addr, itype, ltype, label)			\
-	asm_volatile_goto("\n"						\
+	asm_goto_output("\n"						\
 		     "1:	mov"itype" %[umem],%[output]\n"		\
 		     _ASM_EXTABLE_UA(1b, %l2)				\
 		     : [output] ltype(x)				\

@@ -375,7 +375,7 @@ do {									\
 	__typeof__(_ptr) _old = (__typeof__(_ptr))(_pold);		\
 	__typeof__(*(_ptr)) __old = *_old;				\
 	__typeof__(*(_ptr)) __new = (_new);				\
-	asm_volatile_goto("\n"						\
+	asm_goto_output("\n"						\
 		     "1: " LOCK_PREFIX "cmpxchg"itype" %[new], %[ptr]\n"\
 		     _ASM_EXTABLE_UA(1b, %l[label])			\
 		     : CC_OUT(z) (success),				\

@@ -394,7 +394,7 @@ do {									\
 	__typeof__(_ptr) _old = (__typeof__(_ptr))(_pold);		\
 	__typeof__(*(_ptr)) __old = *_old;				\
 	__typeof__(*(_ptr)) __new = (_new);				\
-	asm_volatile_goto("\n"						\
+	asm_goto_output("\n"						\
 		     "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n"		\
 		     _ASM_EXTABLE_UA(1b, %l[label])			\
 		     : CC_OUT(z) (success),				\

@@ -477,7 +477,7 @@ struct __large_struct { unsigned long buf[100]; };
  * aliasing issues.
  */
 #define __put_user_goto(x, addr, itype, ltype, label)			\
-	asm_volatile_goto("\n"						\
+	asm goto("\n"							\
 		"1:	mov"itype" %0,%1\n"				\
 		_ASM_EXTABLE_UA(1b, %l2)				\
 		: : ltype(x), "m" (__m(addr))				\

arch/x86/kvm/svm/svm_ops.h +3 -3

index 36c8af87a707..4e725854c63a 100644
@@ -8,7 +8,7 @@
 
 #define svm_asm(insn, clobber...)				\
 do {								\
-	asm_volatile_goto("1: " __stringify(insn) "\n\t"	\
+	asm goto("1: " __stringify(insn) "\n\t"	\
 			  _ASM_EXTABLE(1b, %l[fault])		\
 			  ::: clobber : fault);			\
 	return;							\

@@ -18,7 +18,7 @@ fault:								\
 
 #define svm_asm1(insn, op1, clobber...)				\
 do {								\
-	asm_volatile_goto("1: "  __stringify(insn) " %0\n\t"	\
+	asm goto("1: "  __stringify(insn) " %0\n\t"	\
 			  _ASM_EXTABLE(1b, %l[fault])		\
 			  :: op1 : clobber : fault);		\
 	return;							\

@@ -28,7 +28,7 @@ fault:								\
 
 #define svm_asm2(insn, op1, op2, clobber...)				\
 do {									\
-	asm_volatile_goto("1: "  __stringify(insn) " %1, %0\n\t"	\
+	asm goto("1: "  __stringify(insn) " %1, %0\n\t"	\
 			  _ASM_EXTABLE(1b, %l[fault])			\
 			  :: op1, op2 : clobber : fault);		\
 	return;								\

arch/x86/kvm/vmx/vmx.c +2 -2

index e262bc2ba4e5..1111d9d08903 100644
@@ -738,7 +738,7 @@ static int vmx_set_guest_uret_msr(struct vcpu_vmx *vmx,
  */
 static int kvm_cpu_vmxoff(void)
 {
-	asm_volatile_goto("1: vmxoff\n\t"
+	asm goto("1: vmxoff\n\t"
 			  _ASM_EXTABLE(1b, %l[fault])
 			  ::: "cc", "memory" : fault);

@@ -2784,7 +2784,7 @@ static int kvm_cpu_vmxon(u64 vmxon_pointer)
 
 	cr4_set_bits(X86_CR4_VMXE);
 
-	asm_volatile_goto("1: vmxon %[vmxon_pointer]\n\t"
+	asm goto("1: vmxon %[vmxon_pointer]\n\t"
 			  _ASM_EXTABLE(1b, %l[fault])
 			  : : [vmxon_pointer] "m"(vmxon_pointer)
 			  : : fault);

arch/x86/kvm/vmx/vmx_ops.h +3 -3

index f41ce3c24123..8060e5fc6dbd 100644
@@ -94,7 +94,7 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field)
 
 #ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
 
-	asm_volatile_goto("1: vmread %[field], %[output]\n\t"
+	asm_goto_output("1: vmread %[field], %[output]\n\t"
 			  "jna %l[do_fail]\n\t"
 
 			  _ASM_EXTABLE(1b, %l[do_exception])

@@ -188,7 +188,7 @@ static __always_inline unsigned long vmcs_readl(unsigned long field)
 
 #define vmx_asm1(insn, op1, error_args...)				\
 do {									\
-	asm_volatile_goto("1: " __stringify(insn) " %0\n\t"		\
+	asm goto("1: " __stringify(insn) " %0\n\t"			\
 			  ".byte 0x2e\n\t" /* branch not taken hint */	\
 			  "jna %l[error]\n\t"				\
 			  _ASM_EXTABLE(1b, %l[fault])			\

@@ -205,7 +205,7 @@ fault:									\
 
 #define vmx_asm2(insn, op1, op2, error_args...)				\
 do {									\
-	asm_volatile_goto("1: "  __stringify(insn) " %1, %0\n\t"	\
+	asm goto("1: "  __stringify(insn) " %1, %0\n\t"			\
 			  ".byte 0x2e\n\t" /* branch not taken hint */	\
 			  "jna %l[error]\n\t"				\
 			  _ASM_EXTABLE(1b, %l[fault])			\

arch/xtensa/include/asm/jump_label.h +2 -2

index c812bf85021c..46c8596259d2 100644
@@ -13,7 +13,7 @@
 static __always_inline bool arch_static_branch(struct static_key *key,
 					       bool branch)
 {
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 			  "_nop\n\t"
 			  ".pushsection __jump_table,  \"aw\"\n\t"
 			  ".word 1b, %l[l_yes], %c0\n\t"

@@ -38,7 +38,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key,
 	 * make it reachable and wrap both into a no-transform block
 	 * to avoid any assembler interference with this.
 	 */
-	asm_volatile_goto("1:\n\t"
+	asm goto("1:\n\t"
 			  ".begin no-transform\n\t"
 			  "_j %l[l_yes]\n\t"
 			  "2:\n\t"

include/linux/compiler-gcc.h +19 -0

index aebb65bf95a7..c1a963be7d28 100644
@@ -64,6 +64,25 @@
 		__builtin_unreachable();	\
 	} while (0)
 
+/*
+ * GCC 'asm goto' with outputs miscompiles certain code sequences:
+ *
+ *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110420
+ *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110422
+ *
+ * Work it around via the same compiler barrier quirk that we used
+ * to use for the old 'asm goto' workaround.
+ *
+ * Also, always mark such 'asm goto' statements as volatile: all
+ * asm goto statements are supposed to be volatile as per the
+ * documentation, but some versions of gcc didn't actually do
+ * that for asms with outputs:
+ *
+ *    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98619
+ */
+#define asm_goto_output(x...) \
+	do { asm volatile goto(x); asm (""); } while (0)
+
 #if defined(CONFIG_ARCH_USE_BUILTIN_BSWAP)
 #define __HAVE_BUILTIN_BSWAP32__
 #define __HAVE_BUILTIN_BSWAP64__

include/linux/compiler_types.h +2 -2

index 6f1ca49306d2..663d8791c871 100644
@@ -362,8 +362,8 @@ struct ftrace_likely_data {
 #define __member_size(p)	__builtin_object_size(p, 1)
 #endif
 
-#ifndef asm_volatile_goto
-#define asm_volatile_goto(x...) asm goto(x)
+#ifndef asm_goto_output
+#define asm_goto_output(x...) asm goto(x)
 #endif
 
 #ifdef CONFIG_CC_HAS_ASM_INLINE

net/netfilter/nft_set_pipapo_avx2.c +1 -1

index 90e275bb3e5d..a3a8ddca9918 100644
@@ -57,7 +57,7 @@
 
 /* Jump to label if @reg is zero */
 #define NFT_PIPAPO_AVX2_NOMATCH_GOTO(reg, label)			\
-	asm_volatile_goto("vptest %%ymm" #reg ", %%ymm" #reg ";"	\
+	asm goto("vptest %%ymm" #reg ", %%ymm" #reg ";"	\
 			  "je %l[" #label "]" : : : : label)
 
 /* Store 256 bits from YMM register into memory. Contrary to bucket load

samples/bpf/asm_goto_workaround.h +4 -4

index 7048bb3594d6..634e81d83efd 100644
@@ -4,14 +4,14 @@
 #define __ASM_GOTO_WORKAROUND_H
 
 /*
- * This will bring in asm_volatile_goto and asm_inline macro definitions
+ * This will bring in asm_goto_output and asm_inline macro definitions
  * if enabled by compiler and config options.
  */
 #include <linux/types.h>
 
-#ifdef asm_volatile_goto
-#undef asm_volatile_goto
-#define asm_volatile_goto(x...) asm volatile("invalid use of asm_volatile_goto")
+#ifdef asm_goto_output
+#undef asm_goto_output
+#define asm_goto_output(x...) asm volatile("invalid use of asm_goto_output")
 #endif
 
 /*

tools/arch/x86/include/asm/rmwcc.h +1 -1

index 11ff975242ca..e2ff22b379a4 100644
@@ -4,7 +4,7 @@
 
 #define __GEN_RMWcc(fullop, var, cc, ...)				\
 do {									\
-	asm_volatile_goto (fullop "; j" cc " %l[cc_label]"		\
+	asm goto (fullop "; j" cc " %l[cc_label]"		\
 			: : "m" (var), ## __VA_ARGS__ 			\
 			: "memory" : cc_label);				\
 	return 0;							\

tools/include/linux/compiler_types.h +2 -2

index 1bdd834bdd57..d09f9dc172a4 100644
@@ -36,8 +36,8 @@
 #include <linux/compiler-gcc.h>
 #endif
 
-#ifndef asm_volatile_goto
-#define asm_volatile_goto(x...) asm goto(x)
+#ifndef asm_goto_output
+#define asm_goto_output(x...) asm goto(x)
 #endif
 
 #endif /* __LINUX_COMPILER_TYPES_H */

Sean Christopherson

author maintainer

2024-02-09 21:46 UTC #9 lore raw

Quoted 1 line · Linus Torvalds

Sean? Does this work for the case you noticed?

Yep. You can quite literally see the effect of the asm(""). A "good" sequence directly propagates the result from the VMREAD's destination register to its final destination

  <+1756>:  mov    $0x280e,%r13d
  <+1762>:  vmread %r13,%r13
  <+1766>:  jbe    0x209fa <sync_vmcs02_to_vmcs12+1834>
  <+1768>:  mov    %r13,0xe8(%rbx)

whereas the "bad" sequence bounces through a different register.

  <+1780>:  mov    $0x2810,%eax
  <+1785>:  vmread %rax,%rax
  <+1788>:  jbe    0x209e4 <sync_vmcs02_to_vmcs12+1812>
  <+1790>:  mov    %rax,%r12
  <+1793>:  mov    %r12,0xf0(%rbx)

Quoted 5 lines

That (b) is very much voodoo programming, but it matches the old magic barrier thing that Jakub Jelinek suggested for the really *old* gcc bug wrt plain (non-output) "asm goto". The underlying bug for _that_ was fixed long ago:

Quoted 8 lines

    http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670
We removed that for plain "asm goto" workaround a couple of years ago, so "asm_volatile_goto()" has been a no-op since June 2022, but this now resurrects that hack for the output case.
I'm not loving it, but Sean seemed to confirm that it fixes the code generation problem, so ...

Yeah, I'm in the same boat.

David Laight

2024-02-10 17:22 UTC #10 lore raw

From: Sean Christopherson

Quoted 14 lines

Sent: 09 February 2024 21:47
Quoted 1 line · Linus Torvalds
Sean? Does this work for the case you noticed?
Yep. You can quite literally see the effect of the asm(""). A "good" sequence directly propagates the result from the VMREAD's destination register to its final destination
  <+1756>:  mov    $0x280e,%r13d
  <+1762>:  vmread %r13,%r13
  <+1766>:  jbe    0x209fa <sync_vmcs02_to_vmcs12+1834>
  <+1768>:  mov    %r13,0xe8(%rbx)

Quoted 7 lines

whereas the "bad" sequence bounces through a different register.

  <+1780>:  mov    $0x2810,%eax
  <+1785>:  vmread %rax,%rax
  <+1788>:  jbe    0x209e4 <sync_vmcs02_to_vmcs12+1812>
  <+1790>:  mov    %rax,%r12
  <+1793>:  mov    %r12,0xf0(%rbx)

...

Annoying, but I doubt it is measurable in this case. Firstly it could easily be a 'free' register rename. Secondly isn't vmread horribly slow anyway, so an extra clock or two won't matter?

The double register move that OPTIMER_HIDE_VAR() often generates is another matter entirely :-)
In the old days the peephole optimiser would (should?) have removed most of these.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Uros Bizjak

2024-02-11 11:12 UTC #11 lore raw

In reply to Linus Torvalds

Quoted 51 lines · Linus Torvalds

Quoted 6 lines · Linus Torvalds
We should also probably get rid of the existing "asm_volatile_goto()" macro name entirely. That name was always pretty horrific, in that it didn't even mark the asm as volatile even in the case where it did anything.
Quoted 8 lines
So the name of that macro made little sense, and the new workaround should be only for asm goto with outputs. So I'd suggest jmaking a new macro with that name:
   #define asm_goto_output(x...)
and on gcc use that old workaround, and on clang just make it be a plain "asm goto".
So here's a suggested patch that does this.
It's largely done with "git grep" and "sed -i", plus some manual fixups for the (few) cases where we have outputs.
It looks superficially sane to me, and it passed an allmodconfig build with gcc, but I'm not going to claim that it is really tested.
Sean? Does this work for the case you noticed?
Basically this gets rid of the old "asm_volatile_goto()" entirely as useless, but replaces it with "asm_goto_outputs()" for the places where we have outputs.
And then for gcc, it makes those cases
(a) use "asm volatile goto" to fix the fact that some versions of gcc
will have missed the "volatile"
(b) adds that extra empty asm as a second barrier after the "real"
asm goto statement
That (b) is very much voodoo programming, but it matches the old magic barrier thing that Jakub Jelinek suggested for the really *old* gcc bug wrt plain (non-output) "asm goto". The underlying bug for _that_ was fixed long ago:
    http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670
We removed that for plain "asm goto" workaround a couple of years ago, so "asm_volatile_goto()" has been a no-op since June 2022, but this now resurrects that hack for the output case.

Quoted 8 lines

I'm not loving it, but Sean seemed to confirm that it fixes the code generation problem, so ...
Adding Uros to the cc, since he is both involved with gcc and with the previous asm goto workaround removal, so maybe he has other suggestions. Uros, see
    https://lore.kernel.org/all/[email protected]/

I'd suggest the original poster to file a bug report in the GCC bugzilla. This way, the bug can be properly analysed and eventually fixed. The detailed instructions are available at
https://gcc.gnu.org/bugs/

Quoted 5 lines

for background.
Also adding Jakub since I'm re-using the hack he suggested for a different - but similar - case. He may have strong opinions too, and may object to that particular monkey-see-monkey-do voodoo programming.

This big-hammer approach will effectively disable all optimizations around the asm, and should really be used only as a last resort. As learned from the PR58670 (linked above), these workarounds can stay in the kernel forever, and even when the compiler is fixed, there is little incentive to remove them - developers from the kernel world do not want to touch them, because "the workaround just works", and developers from the compiler world do not want to touch them either, because of unknown hidden consequences of removal.

Please note, that the kernel is a heavy user of asm statements, and in GCC some forms of asm statements were developed specifically for kernel use, e.g. CC-output, and asm goto in all of its flavors. They have little or no use outside the kernel. If the kernel disables all optimizations around these statements, there actually remain no real-world testcases of how compiler optimizations interact with asm statements, and when new optimizations are introduced to the compiler, asm statemets are left behind (*).

(*) When the GCC compiler nears its release, people start to test it on their code bases, and the kernel is one of the most important code bases as far as the compiler is concerned. We count on bugreports from these tests to fix possible fall-out from new optimizations, and the big-hammer workaround will just hide the possible problems.

So, I'd suggest at least limit the workaround to known-bad compilers.

Thanks,
Uros.

Linus Torvalds

2024-02-11 20:00 UTC #12 lore raw

Quoted 5 lines · Uros Bizjak

I'd suggest the original poster to file a bug report in the GCC bugzilla. This way, the bug can be properly analysed and eventually fixed. The detailed instructions are available at
https://gcc.gnu.org/bugs/

Yes, please. Sean?

In order to *not* confuse it with the "asm goto with output doesn't imply volatile" bugs, could you make a bug report that talks purely about the code generation issue that happens even with a manually added volatile (your third code sequence in your original email)?

Quoted 1 line

So, I'd suggest at least limit the workaround to known-bad compilers.

Right now, I don't think we know which ones are bad.

If it was just the "add a volatile by hand" issue, we'd have a good pointer to exactly which issue it is.

The other bugzilla entries I saw talked about ICE errors - and one of them is quite likely to be the cause of this, because it really looks like the code issue is some internal confusion. The asm that Sean uses doesn't even have an unused result, so even the volatile shouldn't matter, and seems purely a case of "limiting optimizations partially hides the issue".

And then us adding another empty asm obviously does just even more of the same, to the point that now the code works.

So I don't think we actually know which compiler version is the fixed one. We only know that gcc-11 doesn't work for Sean.

Linus

Jakub Jelinek

2024-02-11 20:12 UTC #13 lore raw

Quoted 7 lines · Linus Torvalds

Quoted 5 lines · Uros Bizjak
I'd suggest the original poster to file a bug report in the GCC bugzilla. This way, the bug can be properly analysed and eventually fixed. The detailed instructions are available at
https://gcc.gnu.org/bugs/

Quoted 6 lines

Yes, please. Sean?
In order to *not* confuse it with the "asm goto with output doesn't imply volatile" bugs, could you make a bug report that talks purely about the code generation issue that happens even with a manually added volatile (your third code sequence in your original email)?

Preferably for all the different cases where you suspect a compiler bug. At minimum preprocessed source + compiler options + detailed description where do you think the bug is (or small runtime testcase but that is harder for issues derived from the kernel obviously).
GCC 11 is still supported upstream, so bugs reproduceable even with just that should be filed in gcc.gnu.org/bugzilla/, if something is only reproduceable with older compilers, guess it belongs in some distribution's bugtrackers if those still support those compilers.
Once filed we can bisect, analyze them, fix.
ICE bugs are even easier to file, all we need is preprocessed source, command line options and gcc version/architecture. gcc -freport-bug in most cases should be able to create everything in one file for the bugreport.

As for the workarounds in the kernel, I'd also like to see only workarounds for specific compiler versions (once filed, bisected and analyzed, workaround could be either based on affected compiler versions, or kernel could try to check for the compiler bug in question and only add workaround if that bug reproduces on a short testcase. Sure, this would be easier in autoconf style checks, but could be done even in kernel's makefiles.

Jakub

Sean Christopherson

author maintainer

2024-02-13 00:15 UTC #14 lore raw

In reply to Linus Torvalds

Quoted 7 lines · Linus Torvalds

Quoted 5 lines · Uros Bizjak
I'd suggest the original poster to file a bug report in the GCC bugzilla. This way, the bug can be properly analysed and eventually fixed. The detailed instructions are available at
https://gcc.gnu.org/bugs/

Quoted 6 lines

Yes, please. Sean?
In order to *not* confuse it with the "asm goto with output doesn't imply volatile" bugs, could you make a bug report that talks purely about the code generation issue that happens even with a manually added volatile (your third code sequence in your original email)?

Will do. Got a bug report ready, just waiting for a GCC Bugzilla account to be created for me so I can file it...

Sean Christopherson

author maintainer

2024-02-14 17:20 UTC #15 lore raw

Quoted 10 lines · Sean Christopherson

Quoted 9 lines · Linus Torvalds
Quoted 5 lines · Uros Bizjak
I'd suggest the original poster to file a bug report in the GCC bugzilla. This way, the bug can be properly analysed and eventually fixed. The detailed instructions are available at
https://gcc.gnu.org/bugs/
Yes, please. Sean?

Quoted 7 lines

Quoted 4 lines
In order to *not* confuse it with the "asm goto with output doesn't imply volatile" bugs, could you make a bug report that talks purely about the code generation issue that happens even with a manually added volatile (your third code sequence in your original email)?
Will do. Got a bug report ready, just waiting for a GCC Bugzilla account to be created for me so I can file it...

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921

Linus Torvalds

2024-02-14 18:44 UTC #16 lore raw

In reply to Uros Bizjak

Quoted 2 lines · Uros Bizjak

So, I'd suggest at least limit the workaround to known-bad compilers.

Based on the current state of

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921

I would suggest this attached kernel patch, which makes the manual "volatile" the default case (since it should make no difference except for the known gcc issue), and limits the extra empty asm serialization to gcc versions older than 12.1.0.

But Jakub is clearly currently trying to figure out exactly what was going wrong, so things may change. Maybe the commit he bisected to happened to just accidentally hide the real issue.

Linus

 include/linux/compiler-gcc.h   | 12 +++---------
 include/linux/compiler_types.h | 11 ++++++++++-
 2 files changed, 13 insertions(+), 10 deletions(-)

include/linux/compiler-gcc.h +3 -9

index c1a963be7d28..d181d2703bba 100644
@@ -67,21 +67,15 @@
 /*
  * GCC 'asm goto' with outputs miscompiles certain code sequences:
  *
- *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110420
- *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110422
+ *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
  *
  * Work it around via the same compiler barrier quirk that we used
  * to use for the old 'asm goto' workaround.
- *
- * Also, always mark such 'asm goto' statements as volatile: all
- * asm goto statements are supposed to be volatile as per the
- * documentation, but some versions of gcc didn't actually do
- * that for asms with outputs:
- *
- *    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98619
  */
+#if GCC_VERSION < 120100
 #define asm_goto_output(x...) \
 	do { asm volatile goto(x); asm (""); } while (0)
+#endif
 
 #if defined(CONFIG_ARCH_USE_BUILTIN_BSWAP)
 #define __HAVE_BUILTIN_BSWAP32__

include/linux/compiler_types.h +10 -1

index 663d8791c871..3bb5a9d16eaa 100644
@@ -362,8 +362,17 @@ struct ftrace_likely_data {
 #define __member_size(p)	__builtin_object_size(p, 1)
 #endif
 
+/*
+ * "asm goto" is documented to always be volatile, but some versions
+ * of gcc don't actually do that:
+ *
+ *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979
+ *
+ * So we'll just do it manually unless we have other more extensive
+ * workarounds.
+ */
 #ifndef asm_goto_output
-#define asm_goto_output(x...) asm goto(x)
+#define asm_goto_output(x...) asm volatile goto(x)
 #endif
 
 #ifdef CONFIG_CC_HAS_ASM_INLINE

Linus Torvalds

2024-02-15 00:11 UTC #17 lore raw

Quoted 6 lines · Linus Torvalds

Based on the current state of
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
I would suggest this attached kernel patch [...]

Well, that "current state" didn't last long, and it looks like Jakub found the real issue and posted a suggested fix.

Anyway, the end result is that the current kernel situation - that adds the workaround for all gcc versions - is the best that we can do for now.

Linus

Jakub Jelinek

2024-02-15 08:39 UTC #18 lore raw

Quoted 7 lines · Linus Torvalds

Quoted 5 lines · Linus Torvalds
Based on the current state of
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921

Quoted 8 lines

Quoted 1 line
I would suggest this attached kernel patch [...]
Well, that "current state" didn't last long, and it looks like Jakub found the real issue and posted a suggested fix.
Anyway, the end result is that the current kernel situation - that adds the workaround for all gcc versions - is the best that we can do for now.

Can it be guarded with
#if GCC_VERSION < 140100
so that it isn't forgotten? GCC 14.1 certainly will have a fix for this (so will GCC 13.3, 12.4 and 11.5).
Maybe it would be helpful to use
#if GCC_VERSION < 140000
while it is true that no GCC 14 snapshots until today (or whenever the fix will be committed) have the fix, for GCC trunk it is up to the distros to use the latest snapshot if they use it at all and would allow better testing of the kernel code without the workaround, so that if there are other issues they won't be discovered years later. Most userland code doesn't actually use asm goto with outputs...

Jakub

Linus Torvalds

2024-02-15 18:25 UTC #19 lore raw

Quoted 3 lines · Jakub Jelinek

Can it be guarded with
#if GCC_VERSION < 140100

Ack. I'll update the workaround to do that, and add the new and improved bugzilla pointer.

Thanks for fixing this so quickly.

Linus

Linus Torvalds

2024-02-15 19:18 UTC #20 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Jakub Jelinek
Can it be guarded with
#if GCC_VERSION < 140100
Ack. I'll update the workaround to do that, and add the new and improved bugzilla pointer.

.. and I also followed your suggestion to just consider any gcc-14 snapshots as fixed. That seemed safe, considering that in practice the actual bug that Sean reported seems to not actually trigger with any gcc version 12.1+ as per your bisect (and my minimal testing).

HOWEVER, when I was working through this, I noted that the *other* part of the workaround (the "missing volatile") doesn't seem to have been backported as aggressively.

IOW, I find that "Mark asm goto with outputs as volatile" in the gcc-12 and gcc-13 branches, but not in gcc-11.

So I did end up making the default "asm_goto_output()" macro always use "asm volatile goto()", so that we don't have to worry about the other gcc issue.

End result: the extra empty asm barrier is now dependent on gcc version in my tree, but the manual addition of 'volatile' is unconditional.

Because it looks to me like gcc-11.5 will have your fix for the pseudo ordering, but not Andrew Pinski's fix for missing a volatile.

Anyway, since the version dependencies were complex enough, I ended up going with putting that logic in our Kconfig files, rather than making the gcc-specific header file an ugly mess of #if's.

Our Kconfig files are pretty much designed for having complicated configuration dependencies, so it ends up being quite natural there:

  config GCC_ASM_GOTO_OUTPUT_WORKAROUND
        bool
        depends on CC_IS_GCC && CC_HAS_ASM_GOTO_OUTPUT
        # Fixed in GCC 14.1, 13.3, 12.4 and 11.5
        # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
        default y if GCC_VERSION < 110500
        default y if GCC_VERSION >= 120000 && GCC_VERSION < 120400
        default y if GCC_VERSION >= 130000 && GCC_VERSION < 130300

and having those kinds of horrid expressions as preprocessor code included in every single compilation unit seemed just nasty.

Linus

Jakub Jelinek

2024-02-15 19:26 UTC #21 lore raw

Quoted 1 line · Linus Torvalds

but the manual addition of 'volatile' is unconditional.

That is just fine. The extra keyword is not wrong, it is just redundant with fixed compilers, where it doesn't change anything in the behavior.

Jakub

Linus Torvalds

2024-02-09 18:56 UTC #22 lore raw

In reply to Sean Christopherson

Quoted 6 lines · Sean Christopherson

Quoted 4 lines
We've removed that workaround in commit 43c249ea0b1e ("compiler-gcc.h:
remove ancient workaround for gcc PR 58670"), I'm wondering if maybe that removal was a bit optimistic.
FWIW, reverting that does restore correct behavior on gcc-11.

Hmm. I suspect we'll just have to revert that commit then - although probably in some form where it only affects the "this has outputs" case.

Which is much less common than the non-output "asm goto" case.

It does cause gcc to generate fairly horrific code (as noted in the commit), but it's almost certainly still better code than what the non-asm-goto case results in.

We have very few uses of CC_HAS_ASM_GOTO_OUTPUT (and the related CC_HAS_ASM_GOTO_TIED_OUTPUT), but unsafe_get_user() in particular generates horrid code without it.

But it would be really good to understand *what* that secondary bug is, and the fix for it. Just to make sure that gcc is really fixed, and there isn't some pending bug that we just keep hiding with that extra empty volatile asm.

Andrew?

Linus

Andrew Pinski (QUIC)

2024-02-09 19:27 UTC #23 lore raw

Quoted 35 lines

-----Original Message-----
From: Linus Torvalds <[email protected]>
Sent: Friday, February 9, 2024 10:56 AM
To: Sean Christopherson <[email protected]>
Cc: Andrew Pinski (QUIC) <[email protected]>; Nick Desaulniers <[email protected]>; [email protected]; Masahiro Yamada <[email protected]>; Peter Zijlstra <[email protected]>; [email protected]
Subject: Re: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11 (and earlier)
Quoted 2 lines · Sean Christopherson
Quoted 2 lines
We've removed that workaround in commit 43c249ea0b1e ("compiler-
gcc.h:
Quoted 4 lines
Quoted 2 lines
remove ancient workaround for gcc PR 58670"), I'm wondering if maybe that removal was a bit optimistic.
FWIW, reverting that does restore correct behavior on gcc-11.
Hmm. I suspect we'll just have to revert that commit then - although probably in some form where it only affects the "this has outputs"
case.
Which is much less common than the non-output "asm goto" case.
It does cause gcc to generate fairly horrific code (as noted in the commit), but it's almost certainly still better code than what the non-asm-goto case results in.
We have very few uses of CC_HAS_ASM_GOTO_OUTPUT (and the related CC_HAS_ASM_GOTO_TIED_OUTPUT), but unsafe_get_user() in particular generates horrid code without it.

Quoted 6 lines

But it would be really good to understand *what* that secondary bug is, and the fix for it. Just to make sure that gcc is really fixed, and there isn't some pending bug that we just keep hiding with that extra empty volatile asm.
Andrew?

Let me first state inline-asm without output has always been volatile; asm goto or not (this has been true since 2.95.3 or even before).
The issue is with an output inline-asm is no longer volatile. But the documentation of GCC stated all asm goto was volatile.
So the side effect of making all asm goto as volatile does have an effect on the ones without an output since they were already treated internally and treated similarly to the inline-asm without an output. The fix that was done to GCC was now treat all asm goto as volatile instead of just the ones without output.
So basically what would happen is internally GCC would remove the asm goto with an output if the output was unused. This would cause havoc in the internal state of GCC because the CFG would not be up to date to what the rest of the IR was.

Oh and there was a bug in GCC 12.1.0-12.3.0, and GCC 13.1.0 releases which might be the reason why folks are hitting issues even with the volatile added back, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110420 was the fix there.

Note there was/is a new different for asm goto was fixed in the past few weeks where some of the optimizations was not ready to handle them (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110422); this was just fixed on all of the open release branches but NOT in any release yet; it will be in 11.5.0, 12.4.0, 13.3.0 and 14.1.0.

Quoted 1 line

Linus