[SPAM Warning!]Re: [PATCH] Revert "riscv: Add a Zalrsc-only alternative for synchronization in start.S"
Yao Zi
ziyao at disroot.org
Mon Oct 13 09:06:45 CEST 2025
On Sun, Oct 12, 2025 at 02:49:59PM -0700, E Shattow wrote:
> Leo (and Tom?) please give Yao the attention they deserve for their
> contributions, starfive_visionfive2 has been broken for several weeks by
> this error and the revert needs to happen sooner rather than later. Thanks!
>
> Hi Yao,
>
> On 9/25/25 21:11, Yao Zi wrote:
> > On Thu, Sep 25, 2025 at 09:27:25AM -0700, E Shattow wrote:
> >>
> >>
> >> On 9/25/25 09:01, Yao Zi wrote:
> >>> This reverts commit a681cfecb4346107212f377e2075f6eb1bdc6a2b.
> >>>
> >>> It has been reported that the commit causes boot regression for SPL on
> >>> StarFive VisionFive 2 or compatible boards. Inspecting the code, I did
> >>> spot one logic error for deciding whether Zaamo or Zalrsc is used, and
> >>> it's still unclear what exactly causes the regression, let's revert it
> >>> for now.
> >>>
> >>> Reported-by: E Shattow <e at freeshell.de>
> >>> Link: https://lore.kernel.org/u-boot/1871663e-b918-4351-9e9e-97f9a4c73733@freeshell.de/
> >>> Signed-off-by: Yao Zi <ziyao at disroot.org>
> >>> ---
> >>>
> >>> The original series causing the problem[1] contains 3 patches, and I
> >>> think it should be enough to revert the change of start.S only, since
> >>> the others touch no code, and should be relatively safe. I'll fix the
> >>> reverted change up and get it work on VisionFive 2 when I got my new
> >>> board. Sorry for the inconvenience.
> >>>
> >>> [1]: https://lore.kernel.org/u-boot/20250902081932.21103-1-ziyao@disroot.org/
> >>>
> >>> arch/riscv/cpu/start.S | 26 +-------------------------
> >>> 1 file changed, 1 insertion(+), 25 deletions(-)
> >>>
> >>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> >>> index 6324ff585d4..7bafdfd390a 100644
> >>> --- a/arch/riscv/cpu/start.S
> >>> +++ b/arch/riscv/cpu/start.S
> >>> @@ -151,15 +151,8 @@ call_harts_early_init:
> >>> */
> >>> la t0, hart_lottery
> >>> li t1, 1
> >>> -#if CONFIG_IS_ENABLED(RISCV_ISA_ZAAMO)
> >>> 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
> >>> @@ -184,12 +177,7 @@ 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:
> >>> @@ -202,14 +190,7 @@ wait_for_gd_init:
> >>> #ifdef CONFIG_AVAILABLE_HARTS
> >>> la t0, available_harts_lock
> >>> li t1, 1
> >>> -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
> >>> +1: amoswap.w.aq t1, t1, 0(t0)
> >>> bnez t1, 1b
> >>>
> >>> /* register available harts in the available_harts mask */
> >>> @@ -219,12 +200,7 @@ 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
> >>>
> >>> /*
> >>
> >> Argument in favor of reverting three patches in the series are as: I do
> >> not understand the use of invisible config symbols in this way for a
> >> transition of something so dependent on toolchain implementation. How
> >> would I express this from menuconfig selections without directly editing
> >> symbols in the config ?
> >
> > I'm not sure what do you mean about "invisible", the two newly
> > introduced Kconfig symbols have their description, are visible in
> > menuconfig and thus could be changed by hand manually.
>
> Invisible means you cannot select the option with menuconfig. Visible
> symbols show up as user-selectable in the menuconfig interface.
But RISCV_ISA_ZAAMO and RISCV_ISA_ZALRSC are visible in menuconfig, you
could find them in "RISC-V architecture" -> "Standard extension for
Atomic Memory Operations" / "Standard extensions for LR/SC
instructions". I do provide description for them and don't realize why
you think they're invisible in the menu.
> >
> > I introduced these symbols for describing whether Zaamo/Zalrsc is
> > availalbe on the targetted platform, and expecting one of them to be
> > explicitly disabled by board-level config if a platform doesn't support
> > Zalrsc or Zaamo, like how CONFIG_RISCV_ISA_A is tackled in
> > ibex-ast2700_defconfig.
> >
> >> I only have some JH7110 boards (and one EIC7700X there is not U-Boot for
> >> this upstream yet) that I can offer to do some testing for, what all
> >> hardware for should this affect? It's every riscv board ?
> >
> > Technically speaking, yes, it introduces two Kconfig symbols that change
> > the logic of determining the ISA extension string for a build, so every
> > board is "affected".
>
> Yes, then this is a failure to test affected configurations, and further
> a failure to communicate this fact in the series and in the commit
> message. Now the `git bisect` story is utterly broken for visionfive2
> for a month worth of commits since.
>
> Because of some unfortunate luck the mail host I am using was offline
> for maintenance when your series posted. I was not aware of this
> breaking change sooner or else I would have resolved my own concerns now
> by having tested and found the error then, and especially if there was
> another revision or two of the series that made some mention asking to
> test this hardware specifically.
I should have been more explicit about affected platforms in the
original series, which may raise more attention on testing and review.
Will do it in v3, and I'm sorry for the mistake.
> >
> > However, among all the RISC-V board-level configs we've already
> > supported, only ibex-ast2700_defconfig explicitly disables
> > CONFIG_RISCV_ISA_A, which means all other ports are compiled with "a"
> > extension contained in ISA extension string before. For these ports, "a"
> > will still be added to the ISA extension string, since
> > CONFIG_RISCV_ISA_ZAAMO and CONFIG_RISCV_ISA_ZALRSC are default to "y".
> >
> > For ibex-ast2700_defconfig, as long as we adjust the defconfig to
> > disable CONFIG_RISCV_ISA_ZAAMO and CONFIG_RISCV_ISA_ZALRSC, it will be
> > built with the same ISA extension string as before (without "a"), too,
> > which is done in PATCH 2.
> >
> > I only revert PATCH 3 since it really causes problems: I'm willing to
> > rework it, and since it depends on PATCH 1, 2, they'll be probably added
> > back as-is latter when I send v3 if they have been reverted together. If
> > it's the case, I think reverting only PATCH 3 makes Git log clearer.
> >
> > If it's found PATCH 1, 2 really break something, I'm willing to revert
> > them. Thanks for the comment.
>
> It is not helpful to argue about code which you did not communicate
> upfront of what was tested and what was not. I would trust your opinion
> however it is equally important to verify your assertions, and that has
> not been done properly.
For verification for the first two patches, as it only modifies the
logic to determine availability of RISCV_ISA_A Kconfig option, and the
option is only used to construct the -march compiler option string, I've
written a script to automatically compare the "-march" argument passed
to compilers with or without the first two patches applied[1].
I've run it locally for all mainline U-Boot ports, and found no
-march argument difference with the two patches applied. Do you think
it's a clue strong enough to consider the two patches safe and
unnecessary to be reverted?
Best regards,
Yao Zi
[1]: https://gist.github.com/ziyao233/13490f0a51bd556bfeb6017427c3b594
> >
> >> Anyhow a revert of this one patch in the series is the minimum to
> >> restore working SPL for starfive visionfive2 (as Star64, and Mars
> >> CM/Lite). Thanks, Yao!
> >>
> >> Acked-by: E Shattow <e at freeshell.de>
> >>
> >
> > Best regards,
> > Yao Zi
>
> I emphasize that I appreciate your work, Yao.
>
> Sincerely,
>
> -E
More information about the U-Boot
mailing list