Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190402112813.193378-1-glider@google.com> <20190405093931.GA28890@gmail.com>
In-Reply-To: <20190405093931.GA28890@gmail.com>
From:   Alexander Potapenko <glider@google.com>
Date:   Fri, 5 Apr 2019 13:53:29 +0200
Message-ID: <CAG_fn=VxwY4KmY5_X3RTdNw9DhSSbozHh5jRGs4YnkkYh6iiFw@mail.gmail.com>
Subject: Re: [PATCH v2] x86/asm: fix assembly constraints in bitops
To:     Ingo Molnar <mingo@kernel.org>
Cc:     Paul McKenney <paulmck@linux.ibm.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Peter Zijlstra <peterz@infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Dmitriy Vyukov <dvyukov@google.com>,
        James Y Knight <jyknight@google.com>, x86@kernel.org,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Borislav Petkov <bp@alien8.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Apr 5, 2019 at 11:39 AM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Alexander Potapenko <glider@google.com> wrote:
>
> > 1. Use memory clobber in bitops that touch arbitrary memory
> >
> > Certain bit operations that read/write bits take a base pointer and an
> > arbitrarily large offset to address the bit relative to that base.
> > Inline assembly constraints aren't expressive enough to tell the
> > compiler that the assembly directive is going to touch a specific memor=
y
> > location of unknown size, therefore we have to use the "memory" clobber
> > to indicate that the assembly is going to access memory locations other
> > than those listed in the inputs/outputs.
> > To indicate that BTR/BTS instructions don't necessarily touch the first
> > sizeof(long) bytes of the argument, we also move the address to assembl=
y
> > inputs.
> >
> > This particular change leads to size increase of 124 kernel functions i=
n
> > a defconfig build. For some of them the diff is in NOP operations, othe=
r
> > end up re-reading values from memory and may potentially slow down the
> > execution. But without these clobbers the compiler is free to cache
> > the contents of the bitmaps and use them as if they weren't changed by
> > the inline assembly.
> >
> > 2. Use byte-sized arguments for operations touching single bytes.
> >
> > Passing a long value to ANDB/ORB/XORB instructions makes the compiler
> > treat sizeof(long) bytes as being clobbered, which isn't the case. This
> > may theoretically lead to worse code in the case of heavy optimization.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Paul E. McKenney <paulmck@linux.ibm.com>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: James Y Knight <jyknight@google.com>
> > ---
> >  v2:
> >   -- renamed the patch
> >   -- addressed comment by Peter Zijlstra: don't use "+m" for functions
> >   returning void
> >   -- fixed input types for operations touching single bytes
> > ---
> >  arch/x86/include/asm/bitops.h | 41 +++++++++++++++--------------------
> >  1 file changed, 18 insertions(+), 23 deletions(-)
>
> I'm wondering what the primary motivation for the patch is:
>
>  - Does it fix an actual miscompilation, or only a theoretical miscompila=
tion?
Depends on what we name an actual miscompilation.
I've built a defconfig kernel and looked through some of the functions
generated by GCC 7.3.0 with and without this clobber, and didn't spot
any miscompilations.
However there is a (trivial) theoretical case where this code leads to
miscompilation: https://lkml.org/lkml/2019/3/28/393 using just GCC
8.3.0 with -O2.
It isn't hard to imagine someone writes such a function in the kernel somed=
ay.

So the primary motivation is to fix an existing misuse of the asm
directive, which happens to work in certain configurations now, but
isn't guaranteed to work under different circumstances.

>  - If it fixes an existing miscompilation:
>
>    - Does it fix a miscompilation triggered by current/future versions of=
 GCC?
>    - Does it fix a miscompilation triggered by current/future versions of=
 Clang?
>
>  - Also, is the miscompilation triggered by 'usual' kernel configs, or
>    does it require exotics such as weird debug options or GCC plugins,
>    etc?
>
> I.e. a bit more context would be useful.
>
> Thanks,
>
>         Ingo


--=20
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Stra=C3=9Fe, 33
80636 M=C3=BCnchen

Gesch=C3=A4ftsf=C3=BChrer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg