Re: [PATCH] crypto: arm64/sha256 - fix build when CONFIG_PREEMPT_VOLUNTARY=y

[Eric Biggers <ebiggers@kernel.org>]

On Thu, May 08, 2025 at 01:12:28PM +0200, Ard Biesheuvel wrote:
> On Wed, 7 May 2025 at 19:09, Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > From: Eric Biggers <ebiggers@google.com>
> >
> > Fix the build of sha256-ce.S when CONFIG_PREEMPT_VOLUNTARY=y by passing
> > the correct label to the cond_yield macro.  Also adjust the code to
> > execute only one branch instruction when CONFIG_PREEMPT_VOLUNTARY=n.
> >
> > Fixes: 6e36be511d28 ("crypto: arm64/sha256 - implement library instead of shash")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202505071811.yYpLUbav-lkp@intel.com/
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  arch/arm64/lib/crypto/sha256-ce.S | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/lib/crypto/sha256-ce.S b/arch/arm64/lib/crypto/sha256-ce.S
> > index a8461d6dad634..f3e21c6d87d2e 100644
> > --- a/arch/arm64/lib/crypto/sha256-ce.S
> > +++ b/arch/arm64/lib/crypto/sha256-ce.S
> > @@ -121,14 +121,15 @@ CPU_LE(   rev32           v19.16b, v19.16b        )
> >
> >         /* update state */
> >         add             dgav.4s, dgav.4s, dg0v.4s
> >         add             dgbv.4s, dgbv.4s, dg1v.4s
> >
> > +       /* return early if voluntary preemption is needed */
> > +       cond_yield      1f, x5, x6
> > +
> 
> This will yield needlessly when the condition hits during the final iteration.
> 
> >         /* handled all input blocks? */
> > -       cbz             x2, 1f
> > -       cond_yield      3f, x5, x6
> > -       b               0b
> > +       cbnz            x2, 0b

cond_yield doesn't actually yield, though.  It just checks whether yielding is
needed.  So the behavior is the same: on the last iteration this function
returns 0 (i.e. 0 blocks remaining), regardless of whether it gets to the end by
jumping there due to TSK_TI_PREEMPT being set or by falling through after seeing
nblocks==0.  We could keep the nblocks==0 check first, but the cond_yield check
is lightweight and it's probably better to avoid the extra branch instruction on
every other iteration.

- Eric