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

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

[Eric Biggers]

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
+
 	/* handled all input blocks? */
-	cbz		x2, 1f
-	cond_yield	3f, x5, x6
-	b		0b
+	cbnz		x2, 0b
 
 	/* store new state */
 1:	st1		{dgav.4s, dgbv.4s}, [x0]
 	mov		x0, x2
 	ret

base-commit: 20e9579f11b6cbdf0556d9cd85a0aa7653caf341
-- 
2.49.0

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

[Herbert Xu]

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

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Ard Biesheuvel]

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
>
>         /* store new state */
>  1:     st1             {dgav.4s, dgbv.4s}, [x0]
>         mov             x0, x2
>         ret
>
> base-commit: 20e9579f11b6cbdf0556d9cd85a0aa7653caf341
> --
> 2.49.0
>

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

[Eric Biggers]

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

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

[Ard Biesheuvel]

On Thu, 8 May 2025 at 18:05, Eric Biggers <ebiggers@kernel.org> wrote:
>
> 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.
>

Ah yes, you're right.

Hopefully we'll soon be able to get rid of it entirely.