Received-SPF: pass (google.com: domain of linux-kernel+bounces-59839-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240208220604.140859-1-seanjc@google.com> <CAKwvOdk_obRUkD6WQHhS9uoFVe3HrgqH5h+FpqsNNgmj4cmvCQ@mail.gmail.com>
 <DM6PR02MB40587AD6ABBF1814E9CCFA7CB84B2@DM6PR02MB4058.namprd02.prod.outlook.com>
 <CAHk-=wi3p5C1n03UYoQhgVDJbh_0ogCpwbgVGnOdGn6RJ6hnKA@mail.gmail.com> <ZcZyWrawr1NUCiQZ@google.com>
In-Reply-To: <ZcZyWrawr1NUCiQZ@google.com>
From: Nick Desaulniers <ndesaulniers@google.com>
Date: Fri, 9 Feb 2024 10:55:42 -0800
Message-ID: <CAKwvOdmKaYYxf7vjvPf2vbn-Ly+4=JZ_zf+OcjYOkWCkgyU_kA@mail.gmail.com>
Subject: Re: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11
 (and earlier)
To: Sean Christopherson <seanjc@google.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>, 
	"Andrew Pinski (QUIC)" <quic_apinski@quicinc.com>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Masahiro Yamada <masahiroy@kernel.org>, 
	Peter Zijlstra <peterz@infradead.org>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 9, 2024 at 10:43=E2=80=AFAM Sean Christopherson <seanjc@google.=
com> wrote:
>
> On Fri, Feb 09, 2024, Linus Torvalds wrote:
> > On Fri, 9 Feb 2024 at 09:14, Andrew Pinski (QUIC)
> > <quic_apinski@quicinc.com> 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 vo=
latile.
>
> Aha!  Yeah, there's a second bug that set things up so that the "not impl=
icitly
> volatile" bug could rear its head.  (And now I feel less bad for not susp=
ecting
> the compiler sooner, because it didn't occur to me that gcc could possibl=
y 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 see=
s the
> unused output and throws it away because the blob isn't treated as volati=
le.
>
>    vmread %rax,%rax       <=3D output register is unused
>    jbe    0xffffffff8109994a <sync_vmcs02_to_vmcs12+1898>
>    xor    %r12d,%r12d     <=3D one of the "return 0" statements
>    mov    %r12,0xf0(%rbx) <=3D 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=3D58670
> >   *
> >   * Work it around via a compiler barrier quirk suggested by Jakub Jeli=
nek.
> >   *
> >   * (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@google.co=
m/
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.


--=20
Thanks,
~Nick Desaulniers