On Fri, Feb 9, 2024 at 10:43 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Fri, Feb 09, 2024, Linus Torvalds wrote: > > On Fri, 9 Feb 2024 at 09:14, Andrew Pinski (QUIC) > > <quic_apinski@xxxxxxxxxxx> wrote: > > > > > > 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. > > 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 > > > 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/20180907222109.163802-1-ndesaulniers@xxxxxxxxxx/ seems like in ~2018 I was trying to, but can't seem to find the thread where Linus pushed back. > > 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. > > Note, this is 100% reproducible across multiple systems, though AFAICT it's > somewhat dependent on the .config. Holler if anyone wants the .config. -- Thanks, ~Nick Desaulniers
