Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Tue, 31 Oct 2023 20:22:02 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     "Elliott, Robert (Servers)" <elliott@hpe.com>
Cc:     Roxana Nicolescu <roxana.nicolescu@canonical.com>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>
Subject: Re: [PATCH] crypto: x86/sha256 - autoload if SHA-NI detected
Message-ID: <20231101032202.GA1830@sol.localdomain>
References: <20231029051555.157720-1-ebiggers@kernel.org>
 <34843a86-6516-47d2-88dd-5ca0aa86a052@canonical.com>
 <MW5PR84MB1842C4652CCCB7738AE7FA4BABA0A@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <MW5PR84MB1842C4652CCCB7738AE7FA4BABA0A@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
Precedence: bulk

On Tue, Oct 31, 2023 at 02:52:53PM +0000, Elliott, Robert (Servers) wrote:
> > -----Original Message-----
> > From: Roxana Nicolescu <roxana.nicolescu@canonical.com>
> > Sent: Tuesday, October 31, 2023 8:19 AM
> > To: Eric Biggers <ebiggers@kernel.org>; linux-crypto@vger.kernel.org
> > Subject: Re: [PATCH] crypto: x86/sha256 - autoload if SHA-NI detected
> > 
> > On 29/10/2023 06:15, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > >
> > > The x86 SHA-256 module contains four implementations: SSSE3, AVX, AVX2,
> > > and SHA-NI.  Commit 1c43c0f1f84a ("crypto: x86/sha - load modules based
> > > on CPU features") made the module be autoloaded when SSSE3, AVX, or AVX2
> > > is detected.  The omission of SHA-NI appears to be an oversight, perhaps
> > > because of the outdated file-level comment.  This patch fixes this,
> > > though in practice this makes no difference because SSSE3 is a subset of
> > > the other three features anyway.  Indeed, sha256_ni_transform() executes
> > > SSSE3 instructions such as pshufb.
> > >
> > > Cc: Roxana Nicolescu <roxana.nicolescu@canonical.com>
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > 
> > Indeed, it was an oversight.
> > 
> > Reviewed-by: Roxana Nicolescu <roxana.nicolescu@canonical.com>
> > > ---
> > >   arch/x86/crypto/sha256_ssse3_glue.c | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/crypto/sha256_ssse3_glue.c
> > b/arch/x86/crypto/sha256_ssse3_glue.c
> > > index 4c0383a90e11..a135cf9baca3 100644
> > > --- a/arch/x86/crypto/sha256_ssse3_glue.c
> ...
> > >
> > >   static const struct x86_cpu_id module_cpu_ids[] = {
> > > +	X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL),
> 
> Unless something else has changed, this needs to be inside ifdefs, as discovered
> in the proposed patch series last year:
> 
> for sha1_sse3_glue.c:
> #ifdef CONFIG_AS_SHA1_NI
>         X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL),
> #endif
> 
> for sha256_sse3_glue.c:
> +#ifdef CONFIG_AS_SHA256_NI
> +       X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL),
> +#endif

Right, thanks for pointing that out.  It compiles either way, but we shouldn't
autoload on SHA-NI when the code using SHA-NI isn't being built.  Sent out v2
with this fixed.

- Eric