MIME-Version: 1.0
References: <20211207113252.162701ed@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <20211208044037.GA11399@gondor.apana.org.au> <20211207212907.6e91821b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <20211220150343.4e12a4d2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <20211220161125.78bc4d66@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <20211220165251.400813dc@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <CAMj1kXG+FGBHr=+vUwVz-u5n7oHpRxikLsOogVW0bOvNow3jHQ@mail.gmail.com>
In-Reply-To: <CAMj1kXG+FGBHr=+vUwVz-u5n7oHpRxikLsOogVW0bOvNow3jHQ@mail.gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Tue, 21 Dec 2021 08:24:48 +0100
Message-ID: <CAMj1kXGV+DHZSOw7t8NgZojMsA6bq-VENz-WxQH+rb8yFj0zyA@mail.gmail.com>
Subject: Re: x86 AES crypto alignment
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Herbert Xu <herbert@gondor.apana.org.au>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Sabrina Dubroca <sd@queasysnail.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 21 Dec 2021 at 07:59, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 21 Dec 2021 at 01:52, Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Mon, 20 Dec 2021 16:11:25 -0800 Jakub Kicinski wrote:
> > > On Mon, 20 Dec 2021 15:03:43 -0800 Jakub Kicinski wrote:
> > > > Hm, I'm benchmarking things now and it appears to be a regression
> > > > introduced somewhere around 5.11 / 5.12. I don't see the memcpy
> > > > eating 20% of performance on 5.10. Bisection time.
> > >
> > > 83c83e658863 ("crypto: aesni - refactor scatterlist processing")
> > >
> > > is what introduced the regression.
> >
> > Something like this?
> >
> > ---->8-----------
> >
> > From: Jakub Kicinski <kuba@kernel.org>
> > Date: Mon, 20 Dec 2021 16:29:26 -0800
> > Subject: [PATCH] x86/aesni: don't require alignment
> >
> > Looks like we take care of the meta-data (key, iv etc.) alignment
> > anyway and data can safely be unaligned. In fact we were feeding
> > unaligned data into crypto routines up until commit 83c83e658863
> > ("crypto: aesni - refactor scatterlist processing") switched to
> > use the full skcipher API.
> >
> > This fixes 21% performance regression in kTLS.
> >
> > Tested by booting with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
> > (and running thru various kTLS packets).
> >
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
> but it needs a Fixes tag.
>
> Could you check whether this means that gcm_context_data in
> gcmaes_crypt_by_sg() does not have to be aligned either? It would be
> nice if we could drop that horrible hack as well.
>

I guess you meant by "we take care of the meta-data (key, iv etc.)
alignment anyway" that we have these hacks for gcm_context_data (which
carries the key) and the IV, using oversized buffers on the stack and
open coded realignment.

It would be really nice if we could just get rid of all of that as
well, and just use {v}movdqu to load those items.


>
> > ---
> >  arch/x86/crypto/aesni-intel_glue.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> > index e09f4672dd38..41901ba9d3a2 100644
> > --- a/arch/x86/crypto/aesni-intel_glue.c
> > +++ b/arch/x86/crypto/aesni-intel_glue.c
> > @@ -1107,7 +1107,7 @@ static struct aead_alg aesni_aeads[] = { {
> >                 .cra_flags              = CRYPTO_ALG_INTERNAL,
> >                 .cra_blocksize          = 1,
> >                 .cra_ctxsize            = sizeof(struct aesni_rfc4106_gcm_ctx),
> > -               .cra_alignmask          = AESNI_ALIGN - 1,
> > +               .cra_alignmask          = 0,
> >                 .cra_module             = THIS_MODULE,
> >         },
> >  }, {
> > @@ -1124,7 +1124,7 @@ static struct aead_alg aesni_aeads[] = { {
> >                 .cra_flags              = CRYPTO_ALG_INTERNAL,
> >                 .cra_blocksize          = 1,
> >                 .cra_ctxsize            = sizeof(struct generic_gcmaes_ctx),
> > -               .cra_alignmask          = AESNI_ALIGN - 1,
> > +               .cra_alignmask          = 0,
> >                 .cra_module             = THIS_MODULE,
> >         },
> >  } };
> > --
> > 2.31.1
> >