Re: [PATCH v4 08/21] lib/crc: Switch ARM and arm64 to 'ksimd' scoped guard API

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Nov 04, 2025 at 04:32:28PM +0100, Ard Biesheuvel wrote:
> On Mon, 3 Nov 2025 at 12:28, Jonathan Cameron
> <jonathan.cameron@huawei.com> wrote:
> >
> > On Fri, 31 Oct 2025 15:05:19 +0100
> > Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > > On Fri, 31 Oct 2025 at 14:52, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Fri, 31 Oct 2025 at 14:49, Jonathan Cameron
> > > > <jonathan.cameron@huawei.com> wrote:
> > > > >
> > > > > On Fri, 31 Oct 2025 11:39:07 +0100
> > > > > Ard Biesheuvel <ardb+git@google.com> wrote:
> > > > >
> > > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > >
> > > > > > Before modifying the prototypes of kernel_neon_begin() and
> > > > > > kernel_neon_end() to accommodate kernel mode FP/SIMD state buffers
> > > > > > allocated on the stack, move arm64 to the new 'ksimd' scoped guard API,
> > > > > > which encapsulates the calls to those functions.
> > > > > >
> > > > > > For symmetry, do the same for 32-bit ARM too.
> > > > > >
> > > > > > Reviewed-by: Eric Biggers <ebiggers@kernel.org>
> > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > > ---
> > > > > >  lib/crc/arm/crc-t10dif.h   | 16 +++++-----------
> > > > > >  lib/crc/arm/crc32.h        | 11 ++++-------
> > > > > >  lib/crc/arm64/crc-t10dif.h | 16 +++++-----------
> > > > > >  lib/crc/arm64/crc32.h      | 16 ++++++----------
> > > > > >  4 files changed, 20 insertions(+), 39 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/crc/arm/crc-t10dif.h b/lib/crc/arm/crc-t10dif.h
> > > > > > index 63441de5e3f1..aaeeab0defb5 100644
> > > > > > --- a/lib/crc/arm/crc-t10dif.h
> > > > > > +++ b/lib/crc/arm/crc-t10dif.h
> > > > >
> > > > > >  static __ro_after_init DEFINE_STATIC_KEY_FALSE(have_neon);
> > > > > > @@ -20,21 +19,16 @@ asmlinkage void crc_t10dif_pmull8(u16 init_crc, const u8 *buf, size_t len,
> > > > > >  static inline u16 crc_t10dif_arch(u16 crc, const u8 *data, size_t length)
> > > > > >  {
> > > > > >       if (length >= CRC_T10DIF_PMULL_CHUNK_SIZE) {
> > > > > > -             if (static_branch_likely(&have_pmull)) {
> > > > > > -                     if (likely(may_use_simd())) {
> > > > > > -                             kernel_neon_begin();
> > > > > > -                             crc = crc_t10dif_pmull64(crc, data, length);
> > > > > > -                             kernel_neon_end();
> > > > > > -                             return crc;
> > > > > > -                     }
> > > > > > +             if (static_branch_likely(&have_pmull) && likely(may_use_simd())) {
> > > > > > +                     scoped_ksimd()
> > > > > > +                             return crc_t10dif_pmull64(crc, data, length);
> > > > >
> > > > > >               } else if (length > CRC_T10DIF_PMULL_CHUNK_SIZE &&
> > > > > >                          static_branch_likely(&have_neon) &&
> > > > > >                          likely(may_use_simd())) {
> > > > >
> > > > > I briefly thought this was a functional change but it's not because
> > > > > of may_use_simd() being something that isn't going to change between
> > > > > the two evaluations.
> > > > >
> > > > > Would it hurt at all to pull that up to be
> > > > >         if (length >= CRC_T10DIF_PMULL_CHUNK_SIZE && likely(may_use_simd())) {
> > > > >                 if (static_branch_likely(&have_pmull)) {
> > > > >                         scoped_ksimd()
> > > > >                                 return crc_t10dif_pmull64(crc, data, length);
> > > > >
> > > > >                 } else if (length > CRC_T10DIF_PMULL_CHUNK_SIZE &&
> > > > >                            static_branch_likely(&have_neon)) {
> > > > >                 ...
> > > > >
> > > > > ?
> > > > >
> > > >
> > > > Yeah that would be a reasonable cleanup, I guess.
> > >
> > > Actually, looking more closely, that would result in may_use_simd()
> > > being evaluated even when the static keys are set to false, given that
> > > the compiler is unlikely to be able to figure out by itself that
> > > may_use_simd() has no side effects.
> > Yeah. That was why it was a question :)
> > Given everything is marked as likely I wasn't sure if we cared about when
> > the static keys aren't set.
> >
> 
> Yeah, it is rather doubtful that those annotations (or the use of
> static keys, for that matter) make a meaningful difference here.

Well, this change makes crc_t10dif_update() not usable during early boot
on arm64, as it will start hitting the
WARN_ON(!system_capabilities_finalized() in may_use_simd().

I doubt there are any current callers where that matters, but I've been
trying to avoid this sort of unnecessary pitfall in the CRC functions.

Checking the static key first eliminates this pitfall and is also more
efficient on CPUs that don't support the relevant CPU feature.

- Eric