Re: [PATCH v2 3/3] crypto: arm64/aegis128 - implement plain NEON version

From: Nick Desaulniers <ndesaulniers@google.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
	<linux-crypto@vger.kernel.org>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Eric Biggers <ebiggers@kernel.org>
Subject: Re: [PATCH v2 3/3] crypto: arm64/aegis128 - implement plain NEON version
Date: Mon, 12 Aug 2019 10:31:24 -0700	[thread overview]
Message-ID: <CAKwvOd=9KP9j7SkyCJ6xBWmVQn8nSsP78PasdtBO5aDFcSm2Rg@mail.gmail.com> (raw)
In-Reply-To: <CAKv+Gu9KKvCXAm=1hJ-owkL4BTi=hmCXA8ag_rTvdnUgn_zvUg@mail.gmail.com>

On Mon, Aug 12, 2019 at 10:22 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Mon, 12 Aug 2019 at 19:50, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > On Sun, Aug 11, 2019 at 3:59 PM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > > diff --git a/crypto/Makefile b/crypto/Makefile
> > > index 99a9fa9087d1..0d2cdd523fd9 100644
> > > --- a/crypto/Makefile
> > > +++ b/crypto/Makefile
> > > @@ -98,7 +98,14 @@ CFLAGS_aegis128-neon-inner.o += -mfpu=crypto-neon-fp-armv8
> > >  aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o
> > >  endif
> > >  ifeq ($(ARCH),arm64)
> > > -CFLAGS_aegis128-neon-inner.o += -ffreestanding -mcpu=generic+crypto
> > > +aegis128-cflags-y := -ffreestanding -mcpu=generic+crypto
> > > +aegis128-cflags-$(CONFIG_CC_IS_GCC) += -ffixed-q16 -ffixed-q17 -ffixed-q18 \
> > > +                                      -ffixed-q19 -ffixed-q20 -ffixed-q21 \
> > > +                                      -ffixed-q22 -ffixed-q23 -ffixed-q24 \
> > > +                                      -ffixed-q25 -ffixed-q26 -ffixed-q27 \
> > > +                                      -ffixed-q28 -ffixed-q29 -ffixed-q30 \
> > > +                                      -ffixed-q31
> >
> > I've filed https://bugs.llvm.org/show_bug.cgi?id=42974 for a feature
> > request for this in Clang.
> >
>
> Good. But even GCC has issues here. Most notably, something like
>
> register uint8x16_t foo asm ("v16");
>
> should permit a register that is excluded from general allocation to
> be used explicitly, but this throws a warning on GCC and an error with
> Clang.

Consider filing bugs against GCC's issue tracker so that they're aware
of the issue if you think there's more that can be improved on their
end (for bugs in Clang, I'm always happy to help submit bug reports).
What is the warning?

for -ffixed-q* and `asm ("v16")`, on aarch64, what are the q registers
and v registers?  I assume they're related to NEON, but I'd only even
worked with w* and x* GPRs.  I *think* the explicit register syntax
works for GPRs in Clang; maybe the v* and q* registers being broken is
just oversight and can be fixed.

> > With those 2 recommendations:
> > Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> > in regards to compiling w/ Clang.  Someone else should review the
> > implementation of this crypto routine.
-- 
Thanks,
~Nick Desaulniers