Re: [PATCH] sh: lib: Restore r4 in shift helpers

[John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>]

Hi Florian,

On Thu, 2026-09-17 at 09:17 +0200, John Paul Adrian Glaubitz wrote:
> Hi Florian.
> 
> On Tue, 2026-07-14 at 12:41 +0200, Florian Fuchs wrote:
> > Commit 940d4113f330 ("sh: New gcc support") added new shift helpers
> > that use r4 as a scratch register while dispatching to the selected shift
> > sequence. But, the helpers return without restoring r4. With GCC 17, the
> > register allocator can keep a live value in r4 across the helper call.
> > Clobbering it results in runtime data corruption. Restore r4 before
> > jumping to the selected shift sequence.
> > 
> > Fixes: 940d4113f330 ("sh: New gcc support")
> > Signed-off-by: Florian Fuchs <fuchsfl@gmail.com>
> > ---
> > Without the patch, the early boot on e.g J2 gets a kernel BUG at
> > mm/percpu.c:2604 / "can't handle more than one group."
> >         PCPU_SETUP_BUG_ON(pcpu_verify_alloc_info(ai) < 0);
> > As the static condition in mm/percpu-km.c wasn't true:
> >         ai->nr_groups != 1
> > nr_groups contained 60 - the clobbered value from the shift helper.
> > 
> > This change was tested on the J2 core on the Mimas v2 board. It can
> > theoretically also target other SH2 devices, but I don't have any other
> > than J2 sadly.
> > 
> > The flow of operations matches now the state in the libgcc, see also
> > in gcc: libgcc/config/sh/lib1funcs.S
> > ---
> >  arch/sh/lib/ashlsi3.S | 3 ++-
> >  arch/sh/lib/ashrsi3.S | 3 ++-
> >  arch/sh/lib/lshrsi3.S | 3 ++-
> >  3 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/sh/lib/ashlsi3.S b/arch/sh/lib/ashlsi3.S
> > index 4df4401cdf31..73a9d709b169 100644
> > --- a/arch/sh/lib/ashlsi3.S
> > +++ b/arch/sh/lib/ashlsi3.S
> > @@ -63,8 +63,9 @@ __ashlsi3_r0:
> >  	mova	ashlsi3_table,r0
> >  	mov.b	@(r0,r4),r4
> >  	add	r4,r0
> > +	mov.l	@r15+,r4
> >  	jmp	@r0
> > -	mov.l	@r15+,r0
> > +	mov	r4,r0
> > 
> 
> Please correct me if I'm wrong, but after reading the code in [1], it looks
> to me as your patch doesn't preserve r4 but it's actually storing it into
> r0 to be used by the selected shift sequence.
> 
> With the previous code, r4 is pushed onto the stack first but not restored
> before the shift sequence is jumped to, so what your patch does not make sure
> that r4 is restored across the complete call of the shift helper but rather
> restore the input value in r4 from the stack before calling the jump sequence.
> 
> What am I missing?
> 
> Adrian
> 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sh/lib/ashlsi3.S

Could you comment on this and maybe rephrase your commit message if you agree
with my analysis? The patch itself is fine, but I think the description is
somewhat inaccurate.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913