Re: [PATCH 2/2] riscv: Add a Zalrsc-only alternative for synchronization in start.S

[Yao Zi <ziyao@disroot.org>]

On Sun, Aug 03, 2025 at 09:21:23AM +0800, Yixun Lan wrote:
> Hi Yao,
> 
> On 09:21 Sat 02 Aug     , Yao Zi wrote:
> > Add an alternative implementation that use Zalrsc extension only for
> > HART lottery and SMP locking to support SMP on cores without "Zaamo"
> > extension available. The Zaamo implementation is still used by default
> > since since the Zalrsc one requires more instructions.
> ~~~~~~~~~~~~~two 'since'
> 
> to slightly improve it..
> .., The Zaamo implementation is prioritized selected if both extension available,
> since the Zalrsc one requires more instructions.

Thanks for catching the typo. This improvement also looks good to me.

> while I can understand the logic, but if we interpret from the code below,
> it's a little bit weird:
>  if (RISCV_ISA_ZAAMO) not enabled:
>  	use zalrsc implementation
> 
> instead of 
>  if (RISCV_ISA_ZALRSC) is enabled:
>  	use zalrsc implementation
> 
> I mean, to select Zalrsc implementation, enabling RISCV_ISA_ZALRSC is not enough,
> but RISCV_ISA_ZAAMO should be explicitly disabled,

This is true, but I don't think it matters: the two implementations
aren't two different features, but code details to the same feature. I
couldn't come up with a reason to explicitly select the Zalrsc one when
Zaamo is available, especially when the Zaamo one takes less code space.

> in fact RISCV_ISA_ZALRSC is superfluous here

Yes, it's not used in this patch, but it has its place: without such an
option, one couldn't distinguish whether Zalrsc is available, it would
be hard to determine whether "A"/"Zalrsc" should be passed as part of
the ISA string when CONFIG_RISCV_ISA_ZAAMO is set to n.

> make it further, it would be great if we could do some Kconfig sanity check..
> (I have one more comment for configs/ibex-ast2700_defconfig in patch 1/2)

If I understand it correctly, these code is actually for SMP systems.
Ideally we should group them inside "#if CONFIG_IS_ENABLED(SMP)" and
make SMP depend on RISCV_RISA_ZAAMO || RISCV_ISA_ZALRSC, which I
originally want to make a separate patch (it's not a must to make SMP
possible on Zalrsc-only systems).

Thanks,
Yao Zi

> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  arch/riscv/cpu/start.S | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 7bafdfd390a..6324ff585d4 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -151,8 +151,15 @@ call_harts_early_init:
> >  	 */
> >  	la	t0, hart_lottery
> >  	li	t1, 1
> > +#if CONFIG_IS_ENABLED
> >  	amoswap.w s2, t1, 0(t0)
> >  	bnez	s2, wait_for_gd_init
> > +#else
> > +	lr.w	s2, (t0)
> > +	bnez	s2, wait_for_gd_init
> > +	sc.w	s2, t1, (t0)
> > +	bnez	s2, wait_for_gd_init
> > +#endif
> >  #else
> >  	/*
> >  	 * FIXME: gp is set before it is initialized. If an XIP U-Boot ever
> > @@ -177,7 +184,12 @@ call_harts_early_init:
> >  #if !CONFIG_IS_ENABLED(XIP)
> >  #ifdef CONFIG_AVAILABLE_HARTS
> >  	la	t0, available_harts_lock
> > +#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >  	amoswap.w.rl zero, zero, 0(t0)
> > +#else
> > +	fence	rw, w
> > +	sw	zero, 0(t0)
> > +#endif
> >  #endif
> >  
> >  wait_for_gd_init:
> > @@ -190,7 +202,14 @@ wait_for_gd_init:
> >  #ifdef CONFIG_AVAILABLE_HARTS
> >  	la	t0, available_harts_lock
> >  	li	t1, 1
> > -1:	amoswap.w.aq t1, t1, 0(t0)
> > +1:
> > +#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> > +	amoswap.w.aq t1, t1, 0(t0)
> > +#else
> > +	lr.w.aq	t1, 0(t0)
> > +	bnez	t1, 1b
> > +	sc.w.rl t1, t1, 0(t0)
> > +#endif
> >  	bnez	t1, 1b
> >  
> >  	/* register available harts in the available_harts mask */
> > @@ -200,7 +219,12 @@ wait_for_gd_init:
> >  	or	t2, t2, t1
> >  	SREG	t2, GD_AVAILABLE_HARTS(gp)
> >  
> > +#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >  	amoswap.w.rl zero, zero, 0(t0)
> > +#else
> > +	fence	rw, w
> > +	sw	zero, 0(t0)
> > +#endif
> >  #endif
> >  
> >  	/*
> > -- 
> > 2.50.1
> > 
> 
> -- 
> Yixun Lan (dlan)