Re: [v2 PATCH] crypto: powerpc/poly1305 - Add poly1305_emit_arch wrapper

[Eric Biggers <ebiggers@kernel.org>]

On Sat, May 10, 2025 at 05:34:01PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, May 09, 2025 at 10:33:08PM -0700, Eric Biggers wrote:
> > On Sat, May 10, 2025 at 01:10:22PM +0800, Herbert Xu wrote:
> > > On Fri, May 09, 2025 at 09:44:50PM -0700, Eric Biggers wrote:
> > > >
> > > > This fixes "-cpu Power10", but older CPUs (e.g. "-cpu POWER9") are still
> > > > failing.
> > > 
> > > You're right.  I'll revert this and apply the following patch
> > > instead.
> > > 
> > > BTW this thing is still hopelessly broken if it's called from
> > > softirq context because there is no SIMD fallback.  Yes I removed
> > > the SIMD check but it was already broken before that as it simply
> > > switched from the 4-block version to the 1-block version if SIMD
> > > is not available rather than actually doing something that is
> > > safe in softirq context.
> > > 
> > > Perhaps we should just remove this altogether until it's fixed.
> > 
> > Yes, the PowerPC Poly1305 code incorrectly uses VSX without first checking
> > crypto_simd_usable().  And PowerPC also doesn't support VSX in softirqs, or at
> > least it doesn't claim to (it doesn't override may_use_simd(), so it gets the
> > default from include/asm-generic/simd.h which returns false in softirq context).
> > Maybe add 'depends on BROKEN' to CRYPTO_POLY1305_P10 for now, and give the
> > PowerPC folks (Cc'ed) a chance to fix this before removing the code.
> 
> What doe "may_use_simd" even *mean*?  At its declaration site it says
> "whether it is allowable at this time to issue SIMD instructions or
> access the SIMD register file", but that is 100% meaningless, you can do
> SIMD in GPRs.
> 
> On PowerPC we have two separate register files dedicated to SIMD-like
> stuff, the VMX and the VSX register files.  Which of those is this
> function supposed to care about?
> 
> It looks like the whole "may_use_simd" thing is a misguided abstraction
> unfortunately :-(

may_use_simd() a.k.a crypto_simd_usable() is supposed to check whether vector /
SIMD registers can be used in the current context, provided that the appropriate
architecture-specific functions like kernel_fpu_begin() and kernel_fpu_end() are
used.  In the case of architectures that support the use of multiple sets of
vector / SIMD registers in kernel mode, it would have to check for the
intersection of the calling context requirements for all of them, since it
doesn't specify a particular set.

The reason that may_use_simd() a.k.a. crypto_simd_usable() got pulled out into
an abstraction shared across all architectures is that it's used by
non-architecture-specific code, such as crypto/simd.c, and also the crypto
self-tests which inject 'false' return values to test the no-SIMD code paths.

I think the users other than the self-tests are on the way out, though.  Most of
the users of crypto/simd.c just got removed, with CRYPTO_AES_GCM_P10 being the
last one.  A new non-architecture-specific user of crypto_simd_usable() just got
added in include/crypto/internal/sha2.h for some reason (despite me nacking the
patch), but that should be reverted.

So if it's really the case that VMX and VSX are both supported for kernel-mode
use but have different requirements on the calling context, you could make the
PowerPC crypto code use more precise checks like may_use_vsx().  Just the crypto
self-tests won't be able to test the no-SIMD code paths that way, unfortunately.

- Eric