Message-ID: <20170425192154.GO128305@google.com>
Date:   Tue, 25 Apr 2017 12:21:54 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     Paul Gortmaker <paul.gortmaker@...driver.com>,
        Will Deacon <will.deacon@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Catalin Marinas <catalin.marinas@....com>,
        Greg Hackmann <ghackmann@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        Robin Murphy <robin.murphy@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Grant Grundler <grundler@...omium.org>,
        Michael Davidson <md@...gle.com>
Subject: Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to
 ASM_EXPORT

El Tue, Apr 25, 2017 at 07:06:30PM +0100 Ard Biesheuvel ha dit:

> On 25 April 2017 at 18:39, Matthias Kaehlcke <mka@...omium.org> wrote:
> > Hi,
> >
> > El Tue, Apr 18, 2017 at 04:35:02PM +0100 Ard Biesheuvel ha dit:
> >
> >> On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@...driver.com> wrote:
> >> > On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@...omium.org> wrote:
> >> >> The operand is an integer constant, make the constness explicit by
> >> >> adding the modifier. This is needed for clang to generate valid code
> >> >> and also works with gcc.
> >> >
> >> > Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
> >> > only use for syntax checking (and hence I don't care if it is the latest and
> >> > greatest) and this commit breaks it:
> >> >
> >> > arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
> >> > operand prefix '%c'
> >> >   asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
> >> >
> >> > I'm currently reverting this change locally so I can continue to use the old
> >> > toolchain:
> >> >
> >> > $ aarch64-linux-gnu-gcc --version
> >> > aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
> >> > GCC 2013.11) 4.8.3 20131202 (prerelease)
> >> > Copyright (C) 2013 Free Software Foundation, Inc.
> >> >
> >> > $ aarch64-linux-gnu-as --version
> >> > GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
> >> > 2013.11) 2.24.0.20131220
> >> > Copyright 2013 Free Software Foundation, Inc.
> >> >
> >> > Maybe it is finally too old and nobody cares, but I thought it worth a mention.
> >> >
> >>
> >> Thanks for the report. I think we care more about GCC 4.8 than about
> >> Clang, which argues for reverting this patch.
> >>
> >> I understand these issues must be frustrating if you are working on
> >> this stuff, but to me, it is not entirely obvious why we want to
> >> support Clang in the first place (i.e., what does it buy you if your
> >> distro/environment is not already using Clang for userland), and why
> >> the burden is on Linux to make modifications to support Clang,
> >> especially when it comes to GCC extensions such as inline assembly
> >> syntax.
> >>
> >> It is ultimately up to the maintainers to decide what to do with this
> >> patch, but my vote would be to revert it, especially given that the %c
> >> placeholder prefix is not documented anywhere, and appears to simply
> >> trigger some GCC internals that happen to do the right thing in this
> >> case.
> >>
> >> However, the I -> i change is arguably an improvement, and considering
> >> that the following
> >>
> >> asm("foo: .long %0" :: "i"(some value))
> >>
> >> doesn't compile with clang either, I suggest you (Matthias) file a bug
> >> against Clang to get this fixed, and we can propose another patch just
> >> for the I->i change.
> >
> > I consulted with folks with more expertise in this area than myself.
> > This is their analysis of the situation:
> >
> > "The ARM ARM specifies that the correct AArch64 instruction assembly
> > syntax is to have a hash sign (#) before an immediate.
> >
> 
> It does not specify that at all:
> 
> """
> The A64 assembly language does not require the # character to
> introduce constant immediate operands, but an assembler must allow
> immediate values introduced with or without the # character. ARM
> recommends that an A64 disassembler outputs a # before an immediate
> operand.
> """
> (ARM DDI 0487A.g page C1-121)
> 
> IOW, it only /recommends/ the # sign for *dis*assemblers. Big difference.

Indeed, thanks for the clarification.

> > Therefore, every time an inline assembly constraint is used that
> > specifies to print an immediate (like 'i' or 'I'), the immediate
> > (e.g. 42) should be printed with the hash (e.g. #42).
> >
> > Therefore, if you're using an immediate constraint where the hash sign
> > must not be printed, you have to use the "c" operand modifier. The "c"
> > operand modifier apparently got introduced to gcc after the 4.8
> > release.
> >
> 
> My problem with the %c modifier is that it is completely undocumented,
> and appears in an internal GCC code generation code path. IOW, the GCC
> developers could also remove it at any time (although this is highly
> unlikely, of course)

clang documents it, but for GCC it is only mentioned under "x86
Operand Modifiers".

> > The binutils assembler and the clang integrated assembler accept
> > immediates without the hash sign as a non-official extension.
> 
> Nope. *That* is mandated by the ARM ARM, see above.
> 
> > Some of
> > the immediate constraints on gcc seem to not print out the hash sign
> > either; which is why the variant in the linux kernel works with gcc.
> >
> 
> Yes, and since it is perfectly legal for the "i" constraint not to
> have a #, I don't understand what the big deal is tbh.
> 
> > In summary, it seems to me that the inline assembly with the %c0
> > operand is the correct one and gcc 4.8 is simply too old to support
> > this."
> >
> 
> OK, so we're back to having to choose between GCC 4.8 and Clang.

That's not what I suggested.

> > If the above is correct it seems that the solution is not to "fix"
> > clang, but to use different instructions for gcc<=4.8 and newer
> > compilers. I am aware that this is not a popular option.
> >
> > What do you think?
> >
> 
> Perhaps I should just rework the code not to rely on inline asm at all.

That sounds like a good option if it is not too much of a hassle.

> I will take a look,

Thanks!

Matthias

Powered by blists - more mailing lists