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;
Date:   Fri, 5 Apr 2019 11:39:31 +0200
From:   Ingo Molnar <mingo@kernel.org>
To:     Alexander Potapenko <glider@google.com>
Cc:     paulmck@linux.ibm.com, hpa@zytor.com, peterz@infradead.org,
        linux-kernel@vger.kernel.org, dvyukov@google.com,
        jyknight@google.com, x86@kernel.org, mingo@redhat.com,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Borislav Petkov <bp@alien8.de>
Subject: Re: [PATCH v2] x86/asm: fix assembly constraints in bitops
Message-ID: <20190405093931.GA28890@gmail.com>
References: <20190402112813.193378-1-glider@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190402112813.193378-1-glider@google.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


* 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 memory
> 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 assembly
> inputs.
> 
> This particular change leads to size increase of 124 kernel functions in
> a defconfig build. For some of them the diff is in NOP operations, other
> 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 miscompilation?

 - 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