Messages in this thread | | | Date | Tue, 8 Oct 2019 09:37:20 +0100 | From | Andrew Murray <> | Subject | Re: [PATCH] arm64: lse: fix LSE atomics with LLVM's integrated assembler |
| |
On Mon, Oct 07, 2019 at 01:28:19PM -0700, Nick Desaulniers wrote: > On Mon, Oct 7, 2019 at 1:14 PM 'Sami Tolvanen' via Clang Built Linux > <clang-built-linux@googlegroups.com> wrote: > > > > Unlike gcc, clang considers each inline assembly block to be independent > > and therefore, when using the integrated assembler for inline assembly, > > any preambles that enable features must be repeated in each block. > > > > Instead of changing all inline assembly blocks that use LSE, this change > > adds -march=armv8-a+lse to KBUILD_CFLAGS, which works with both clang > > and gcc. > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > Thanks Sami, looks like this addresses: > Link: https://github.com/ClangBuiltLinux/linux/issues/671 > I tried adding `.arch armv8-a+lse` directives to all of the inline asm: > https://github.com/ClangBuiltLinux/linux/issues/573#issuecomment-535098996 > though that quickly ran aground in some failed assertion using the > alternatives system that I don't quite yet understand.
I think these issues somehow are related to the strange way we used to jump to the out-of-line fallbacks. Since around addfc38672c7 ("arm64: atomics: avoid out-of-line ll/sc atomics") this approach was removed.
I reproduced your patch on 5.4-rc2 and no longer get the clang build errors. Therefore this approach is viable option.
> > One thing to be careful about is that blankets the entire kernel in > `+lse`, allowing LSE atomics to be selected at any point. The > assembler directive in the header (or per inline asm block) is more > fine grained. I'm not sure that they would be guarded by the > alternative system. Maybe that's not an issue, but it is the reason I > tried to localize the assembler directive first. > > Note that Clang really wants the target arch specified by either: > 1. command line argument (as in this patch)
This makes me nervous, as we're telling the compiler that the machine we're building for has LSE - obviously it would be perfectly acceptable for the compiler to then throw in some LSE instructions at some point. Thus something may break further down the line.
> 2. per function function attribute > 3. per asm statement assembler directive
Keen to hear Will's thoughts - but I'd suggest this is probably the safest way forward.
Thanks,
Andrew Murray
> > 1 is the smallest incision. > > > --- > > arch/arm64/Makefile | 2 ++ > > arch/arm64/include/asm/lse.h | 2 -- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile > > index 84a3d502c5a5..7a7c0cb8ed60 100644 > > --- a/arch/arm64/Makefile > > +++ b/arch/arm64/Makefile > > @@ -36,6 +36,8 @@ lseinstr := $(call as-instr,.arch_extension lse,-DCONFIG_AS_LSE=1) > > ifeq ($(CONFIG_ARM64_LSE_ATOMICS), y) > > ifeq ($(lseinstr),) > > $(warning LSE atomics not supported by binutils) > > + else > > +KBUILD_CFLAGS += -march=armv8-a+lse > > endif > > endif > > > > diff --git a/arch/arm64/include/asm/lse.h b/arch/arm64/include/asm/lse.h > > index 80b388278149..8603a9881529 100644 > > --- a/arch/arm64/include/asm/lse.h > > +++ b/arch/arm64/include/asm/lse.h > > @@ -14,8 +14,6 @@ > > #include <asm/atomic_lse.h> > > #include <asm/cpucaps.h> > > > > -__asm__(".arch_extension lse"); > > - > > extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS]; > > extern struct static_key_false arm64_const_caps_ready; > > > > -- > > 2.23.0.581.g78d2f28ef7-goog > > > > -- > > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20191007201452.208067-1-samitolvanen%40google.com. > > > > -- > Thanks, > ~Nick Desaulniers
| |