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>
In-Reply-To: <20211220165251.400813dc@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Tue, 21 Dec 2021 07:59:54 +0100
Message-ID: <CAMj1kXG+FGBHr=+vUwVz-u5n7oHpRxikLsOogVW0bOvNow3jHQ@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 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.


> ---
>  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
>