[PATCH] lib: sbi: Revert entry_count before doing hsm stop in hsm wait loop

[PATCH] lib: sbi: Revert entry_count before doing hsm stop in hsm wait loop

[Anup Patel]

Using hsm stop in hsm wait loop causes secondary harts to be stuck
forever in OpenSBI on RISC-V platforms where HSM hart hotplug is
available and all harts come-up at the same time during system
power-on.

For example, lets say we have two harts A and B on a RISC-V platform
with HSM hart hotplug which come-up at the same time during system
power-on. The hart A enters OpenSBI before hart B hence it becomes
the primary (or cold-boot) hart whereas hart B becomes the secondary
(or warm-boot) hart. The hart A follows the OpenSBI cold-boot path
and registers hsm device before hart B enters OpenSBI. The hart B
eventually enters OpenSBI and follows the OpenSBI warm-boot path
so it will increment it's own entry_count before entering hsm wait
loop where it sees hsm device and stops itself. Later as part of
the Linux boot-up sequence, hart A issues SBI HSM start call to
bring-up hart B but OpenSBI sees entry_count != init_count for
hart B in sbi_hsm_hart_start() hence hsm_device_hart_start() is
not called for hart B resulting in hart B stuck forever in OpenSBI.

To fix the above issue, revert entry_count before doing hsm stop
in hsm wait loop.

Fixes: d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 include/sbi/sbi_init.h |  2 ++
 lib/sbi/sbi_hsm.c      |  4 +++-
 lib/sbi/sbi_init.c     | 13 +++++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/sbi/sbi_init.h b/include/sbi/sbi_init.h
index c9013ea4..ad068674 100644
--- a/include/sbi/sbi_init.h
+++ b/include/sbi/sbi_init.h
@@ -16,6 +16,8 @@ struct sbi_scratch;
 
 void __noreturn sbi_init(struct sbi_scratch *scratch);
 
+void sbi_revert_entry_count(struct sbi_scratch *scratch);
+
 unsigned long sbi_entry_count(u32 hartindex);
 
 unsigned long sbi_init_count(u32 hartindex);
diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
index e8128a39..557ab131 100644
--- a/lib/sbi/sbi_hsm.c
+++ b/lib/sbi/sbi_hsm.c
@@ -176,8 +176,10 @@ static void sbi_hsm_hart_wait(struct sbi_scratch *scratch)
 		 * If the hsm_dev is ready and it support the hotplug, we can
 		 * use the hsm stop for more power saving
 		 */
-		if (hsm_device_has_hart_hotplug())
+		if (hsm_device_has_hart_hotplug()) {
+			sbi_revert_entry_count(scratch);
 			hsm_device_hart_stop();
+		}
 
 		wfi();
 	}
diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index 62c32682..84a63748 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -579,6 +579,19 @@ void __noreturn sbi_init(struct sbi_scratch *scratch)
 		init_warmboot(scratch, hartid);
 }
 
+void sbi_revert_entry_count(struct sbi_scratch *scratch)
+{
+	unsigned long *entry_count, *init_count;
+
+	if (!entry_count_offset || !init_count_offset)
+		sbi_hart_hang();
+
+	entry_count = sbi_scratch_offset_ptr(scratch, entry_count_offset);
+	init_count = sbi_scratch_offset_ptr(scratch, init_count_offset);
+
+	*entry_count = *init_count;
+}
+
 unsigned long sbi_entry_count(u32 hartindex)
 {
 	struct sbi_scratch *scratch;
-- 
2.43.0


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH] lib: sbi: Revert entry_count before doing hsm stop in hsm wait loop

[Nick Hu]

On Tue, May 27, 2025 at 8:48 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> Using hsm stop in hsm wait loop causes secondary harts to be stuck
> forever in OpenSBI on RISC-V platforms where HSM hart hotplug is
> available and all harts come-up at the same time during system
> power-on.
>
> For example, lets say we have two harts A and B on a RISC-V platform
> with HSM hart hotplug which come-up at the same time during system
> power-on. The hart A enters OpenSBI before hart B hence it becomes
> the primary (or cold-boot) hart whereas hart B becomes the secondary
> (or warm-boot) hart. The hart A follows the OpenSBI cold-boot path
> and registers hsm device before hart B enters OpenSBI. The hart B
> eventually enters OpenSBI and follows the OpenSBI warm-boot path
> so it will increment it's own entry_count before entering hsm wait
> loop where it sees hsm device and stops itself. Later as part of
> the Linux boot-up sequence, hart A issues SBI HSM start call to
> bring-up hart B but OpenSBI sees entry_count != init_count for
> hart B in sbi_hsm_hart_start() hence hsm_device_hart_start() is
> not called for hart B resulting in hart B stuck forever in OpenSBI.
>
> To fix the above issue, revert entry_count before doing hsm stop
> in hsm wait loop.
>
> Fixes: d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  include/sbi/sbi_init.h |  2 ++
>  lib/sbi/sbi_hsm.c      |  4 +++-
>  lib/sbi/sbi_init.c     | 13 +++++++++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/include/sbi/sbi_init.h b/include/sbi/sbi_init.h
> index c9013ea4..ad068674 100644
> --- a/include/sbi/sbi_init.h
> +++ b/include/sbi/sbi_init.h
> @@ -16,6 +16,8 @@ struct sbi_scratch;
>
>  void __noreturn sbi_init(struct sbi_scratch *scratch);
>
> +void sbi_revert_entry_count(struct sbi_scratch *scratch);
> +
>  unsigned long sbi_entry_count(u32 hartindex);
>
>  unsigned long sbi_init_count(u32 hartindex);
> diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
> index e8128a39..557ab131 100644
> --- a/lib/sbi/sbi_hsm.c
> +++ b/lib/sbi/sbi_hsm.c
> @@ -176,8 +176,10 @@ static void sbi_hsm_hart_wait(struct sbi_scratch *scratch)
>                  * If the hsm_dev is ready and it support the hotplug, we can
>                  * use the hsm stop for more power saving
>                  */
> -               if (hsm_device_has_hart_hotplug())
> +               if (hsm_device_has_hart_hotplug()) {
> +                       sbi_revert_entry_count(scratch);
>                         hsm_device_hart_stop();

Hi Anup,
Thanks for addressing the bug =)

However, there may still be an issue in the following scenario:
If the HSM state of core B is SBI_HSM_STATE_START_PENDING at this
point, it won't enter the warm boot path. As a result, the
`init_count` will end up being `entry_count` + 1 after the warm init
path.

Would it make sense to increment `entry_count` after the
`sbi_hsm_init()` call instead?

> +               }
>
>                 wfi();
>         }
> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index 62c32682..84a63748 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -579,6 +579,19 @@ void __noreturn sbi_init(struct sbi_scratch *scratch)
>                 init_warmboot(scratch, hartid);
>  }
>
> +void sbi_revert_entry_count(struct sbi_scratch *scratch)
> +{
> +       unsigned long *entry_count, *init_count;
> +
> +       if (!entry_count_offset || !init_count_offset)
> +               sbi_hart_hang();
> +
> +       entry_count = sbi_scratch_offset_ptr(scratch, entry_count_offset);
> +       init_count = sbi_scratch_offset_ptr(scratch, init_count_offset);
> +
> +       *entry_count = *init_count;
> +}
> +
>  unsigned long sbi_entry_count(u32 hartindex)
>  {
>         struct sbi_scratch *scratch;
> --
> 2.43.0
>

Best Regards,
Nick

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH] lib: sbi: Revert entry_count before doing hsm stop in hsm wait loop

[Anup Patel]

On Wed, May 28, 2025 at 3:56 PM Nick Hu <nick.hu@sifive.com> wrote:
>
> On Tue, May 27, 2025 at 8:48 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > Using hsm stop in hsm wait loop causes secondary harts to be stuck
> > forever in OpenSBI on RISC-V platforms where HSM hart hotplug is
> > available and all harts come-up at the same time during system
> > power-on.
> >
> > For example, lets say we have two harts A and B on a RISC-V platform
> > with HSM hart hotplug which come-up at the same time during system
> > power-on. The hart A enters OpenSBI before hart B hence it becomes
> > the primary (or cold-boot) hart whereas hart B becomes the secondary
> > (or warm-boot) hart. The hart A follows the OpenSBI cold-boot path
> > and registers hsm device before hart B enters OpenSBI. The hart B
> > eventually enters OpenSBI and follows the OpenSBI warm-boot path
> > so it will increment it's own entry_count before entering hsm wait
> > loop where it sees hsm device and stops itself. Later as part of
> > the Linux boot-up sequence, hart A issues SBI HSM start call to
> > bring-up hart B but OpenSBI sees entry_count != init_count for
> > hart B in sbi_hsm_hart_start() hence hsm_device_hart_start() is
> > not called for hart B resulting in hart B stuck forever in OpenSBI.
> >
> > To fix the above issue, revert entry_count before doing hsm stop
> > in hsm wait loop.
> >
> > Fixes: d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  include/sbi/sbi_init.h |  2 ++
> >  lib/sbi/sbi_hsm.c      |  4 +++-
> >  lib/sbi/sbi_init.c     | 13 +++++++++++++
> >  3 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/sbi/sbi_init.h b/include/sbi/sbi_init.h
> > index c9013ea4..ad068674 100644
> > --- a/include/sbi/sbi_init.h
> > +++ b/include/sbi/sbi_init.h
> > @@ -16,6 +16,8 @@ struct sbi_scratch;
> >
> >  void __noreturn sbi_init(struct sbi_scratch *scratch);
> >
> > +void sbi_revert_entry_count(struct sbi_scratch *scratch);
> > +
> >  unsigned long sbi_entry_count(u32 hartindex);
> >
> >  unsigned long sbi_init_count(u32 hartindex);
> > diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
> > index e8128a39..557ab131 100644
> > --- a/lib/sbi/sbi_hsm.c
> > +++ b/lib/sbi/sbi_hsm.c
> > @@ -176,8 +176,10 @@ static void sbi_hsm_hart_wait(struct sbi_scratch *scratch)
> >                  * If the hsm_dev is ready and it support the hotplug, we can
> >                  * use the hsm stop for more power saving
> >                  */
> > -               if (hsm_device_has_hart_hotplug())
> > +               if (hsm_device_has_hart_hotplug()) {
> > +                       sbi_revert_entry_count(scratch);
> >                         hsm_device_hart_stop();
>
> Hi Anup,
> Thanks for addressing the bug =)
>
> However, there may still be an issue in the following scenario:
> If the HSM state of core B is SBI_HSM_STATE_START_PENDING at this
> point, it won't enter the warm boot path. As a result, the
> `init_count` will end up being `entry_count` + 1 after the warm init
> path.
>
> Would it make sense to increment `entry_count` after the
> `sbi_hsm_init()` call instead?

This is not a bug rather expected behaviour.

When the hart B is in SBI_HSM_STATE_START_PENDING state
then it means some other hart A has issued HSM hart start and
has also provided entry details. The hart B increments entry_count
upon entry into OpenSBI and increments init_count after initialization
is done.

Regards,
Anup

>
> > +               }
> >
> >                 wfi();
> >         }
> > diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> > index 62c32682..84a63748 100644
> > --- a/lib/sbi/sbi_init.c
> > +++ b/lib/sbi/sbi_init.c
> > @@ -579,6 +579,19 @@ void __noreturn sbi_init(struct sbi_scratch *scratch)
> >                 init_warmboot(scratch, hartid);
> >  }
> >
> > +void sbi_revert_entry_count(struct sbi_scratch *scratch)
> > +{
> > +       unsigned long *entry_count, *init_count;
> > +
> > +       if (!entry_count_offset || !init_count_offset)
> > +               sbi_hart_hang();
> > +
> > +       entry_count = sbi_scratch_offset_ptr(scratch, entry_count_offset);
> > +       init_count = sbi_scratch_offset_ptr(scratch, init_count_offset);
> > +
> > +       *entry_count = *init_count;
> > +}
> > +
> >  unsigned long sbi_entry_count(u32 hartindex)
> >  {
> >         struct sbi_scratch *scratch;
> > --
> > 2.43.0
> >
>
> Best Regards,
> Nick

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH] lib: sbi: Revert entry_count before doing hsm stop in hsm wait loop

[Nick Hu]

On Wed, May 28, 2025 at 8:14 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Wed, May 28, 2025 at 3:56 PM Nick Hu <nick.hu@sifive.com> wrote:
> >
> > On Tue, May 27, 2025 at 8:48 PM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > Using hsm stop in hsm wait loop causes secondary harts to be stuck
> > > forever in OpenSBI on RISC-V platforms where HSM hart hotplug is
> > > available and all harts come-up at the same time during system
> > > power-on.
> > >
> > > For example, lets say we have two harts A and B on a RISC-V platform
> > > with HSM hart hotplug which come-up at the same time during system
> > > power-on. The hart A enters OpenSBI before hart B hence it becomes
> > > the primary (or cold-boot) hart whereas hart B becomes the secondary
> > > (or warm-boot) hart. The hart A follows the OpenSBI cold-boot path
> > > and registers hsm device before hart B enters OpenSBI. The hart B
> > > eventually enters OpenSBI and follows the OpenSBI warm-boot path
> > > so it will increment it's own entry_count before entering hsm wait
> > > loop where it sees hsm device and stops itself. Later as part of
> > > the Linux boot-up sequence, hart A issues SBI HSM start call to
> > > bring-up hart B but OpenSBI sees entry_count != init_count for
> > > hart B in sbi_hsm_hart_start() hence hsm_device_hart_start() is
> > > not called for hart B resulting in hart B stuck forever in OpenSBI.
> > >
> > > To fix the above issue, revert entry_count before doing hsm stop
> > > in hsm wait loop.
> > >
> > > Fixes: d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > ---
> > >  include/sbi/sbi_init.h |  2 ++
> > >  lib/sbi/sbi_hsm.c      |  4 +++-
> > >  lib/sbi/sbi_init.c     | 13 +++++++++++++
> > >  3 files changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/sbi/sbi_init.h b/include/sbi/sbi_init.h
> > > index c9013ea4..ad068674 100644
> > > --- a/include/sbi/sbi_init.h
> > > +++ b/include/sbi/sbi_init.h
> > > @@ -16,6 +16,8 @@ struct sbi_scratch;
> > >
> > >  void __noreturn sbi_init(struct sbi_scratch *scratch);
> > >
> > > +void sbi_revert_entry_count(struct sbi_scratch *scratch);
> > > +
> > >  unsigned long sbi_entry_count(u32 hartindex);
> > >
> > >  unsigned long sbi_init_count(u32 hartindex);
> > > diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
> > > index e8128a39..557ab131 100644
> > > --- a/lib/sbi/sbi_hsm.c
> > > +++ b/lib/sbi/sbi_hsm.c
> > > @@ -176,8 +176,10 @@ static void sbi_hsm_hart_wait(struct sbi_scratch *scratch)
> > >                  * If the hsm_dev is ready and it support the hotplug, we can
> > >                  * use the hsm stop for more power saving
> > >                  */
> > > -               if (hsm_device_has_hart_hotplug())
> > > +               if (hsm_device_has_hart_hotplug()) {
> > > +                       sbi_revert_entry_count(scratch);
> > >                         hsm_device_hart_stop();
> >
> > Hi Anup,
> > Thanks for addressing the bug =)
> >
> > However, there may still be an issue in the following scenario:
> > If the HSM state of core B is SBI_HSM_STATE_START_PENDING at this
> > point, it won't enter the warm boot path. As a result, the
> > `init_count` will end up being `entry_count` + 1 after the warm init
> > path.
> >
> > Would it make sense to increment `entry_count` after the
> > `sbi_hsm_init()` call instead?
>
> This is not a bug rather expected behaviour.
>
> When the hart B is in SBI_HSM_STATE_START_PENDING state
> then it means some other hart A has issued HSM hart start and
> has also provided entry details. The hart B increments entry_count
> upon entry into OpenSBI and increments init_count after initialization
> is done.
>
I'm not sure if I understand it correctly.
In the above case, the `init_count` != `entry_count` for hart B after
the initialization is done.
If the hart B enters the `hsm_dev->hart_stop()` via the
SBI_EXT_HSM_HART_STOP path, Doesn't that mean the hart A won’t be able
to call `hsm_dev->hart_start()` for the hart B?
The `hsm_dev->hart_stop()` function may transition the hart into a
platform-specific low-power state that requires
`hsm_dev->hart_start()` to wake it up.

Best Regards,
Nick

> Regards,
> Anup
>
> >
> > > +               }
> > >
> > >                 wfi();
> > >         }
> > > diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> > > index 62c32682..84a63748 100644
> > > --- a/lib/sbi/sbi_init.c
> > > +++ b/lib/sbi/sbi_init.c
> > > @@ -579,6 +579,19 @@ void __noreturn sbi_init(struct sbi_scratch *scratch)
> > >                 init_warmboot(scratch, hartid);
> > >  }
> > >
> > > +void sbi_revert_entry_count(struct sbi_scratch *scratch)
> > > +{
> > > +       unsigned long *entry_count, *init_count;
> > > +
> > > +       if (!entry_count_offset || !init_count_offset)
> > > +               sbi_hart_hang();
> > > +
> > > +       entry_count = sbi_scratch_offset_ptr(scratch, entry_count_offset);
> > > +       init_count = sbi_scratch_offset_ptr(scratch, init_count_offset);
> > > +
> > > +       *entry_count = *init_count;
> > > +}
> > > +
> > >  unsigned long sbi_entry_count(u32 hartindex)
> > >  {
> > >         struct sbi_scratch *scratch;
> > > --
> > > 2.43.0
> > >
> >
> > Best Regards,
> > Nick

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH] lib: sbi: Revert entry_count before doing hsm stop in hsm wait loop

[Nick Hu]

On Wed, May 28, 2025 at 10:34 PM Nick Hu <nick.hu@sifive.com> wrote:
>
> On Wed, May 28, 2025 at 8:14 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Wed, May 28, 2025 at 3:56 PM Nick Hu <nick.hu@sifive.com> wrote:
> > >
> > > On Tue, May 27, 2025 at 8:48 PM Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > Using hsm stop in hsm wait loop causes secondary harts to be stuck
> > > > forever in OpenSBI on RISC-V platforms where HSM hart hotplug is
> > > > available and all harts come-up at the same time during system
> > > > power-on.
> > > >
> > > > For example, lets say we have two harts A and B on a RISC-V platform
> > > > with HSM hart hotplug which come-up at the same time during system
> > > > power-on. The hart A enters OpenSBI before hart B hence it becomes
> > > > the primary (or cold-boot) hart whereas hart B becomes the secondary
> > > > (or warm-boot) hart. The hart A follows the OpenSBI cold-boot path
> > > > and registers hsm device before hart B enters OpenSBI. The hart B
> > > > eventually enters OpenSBI and follows the OpenSBI warm-boot path
> > > > so it will increment it's own entry_count before entering hsm wait
> > > > loop where it sees hsm device and stops itself. Later as part of
> > > > the Linux boot-up sequence, hart A issues SBI HSM start call to
> > > > bring-up hart B but OpenSBI sees entry_count != init_count for
> > > > hart B in sbi_hsm_hart_start() hence hsm_device_hart_start() is
> > > > not called for hart B resulting in hart B stuck forever in OpenSBI.
> > > >
> > > > To fix the above issue, revert entry_count before doing hsm stop
> > > > in hsm wait loop.
> > > >
> > > > Fixes: d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > > ---
> > > >  include/sbi/sbi_init.h |  2 ++
> > > >  lib/sbi/sbi_hsm.c      |  4 +++-
> > > >  lib/sbi/sbi_init.c     | 13 +++++++++++++
> > > >  3 files changed, 18 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/sbi/sbi_init.h b/include/sbi/sbi_init.h
> > > > index c9013ea4..ad068674 100644
> > > > --- a/include/sbi/sbi_init.h
> > > > +++ b/include/sbi/sbi_init.h
> > > > @@ -16,6 +16,8 @@ struct sbi_scratch;
> > > >
> > > >  void __noreturn sbi_init(struct sbi_scratch *scratch);
> > > >
> > > > +void sbi_revert_entry_count(struct sbi_scratch *scratch);
> > > > +
> > > >  unsigned long sbi_entry_count(u32 hartindex);
> > > >
> > > >  unsigned long sbi_init_count(u32 hartindex);
> > > > diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
> > > > index e8128a39..557ab131 100644
> > > > --- a/lib/sbi/sbi_hsm.c
> > > > +++ b/lib/sbi/sbi_hsm.c
> > > > @@ -176,8 +176,10 @@ static void sbi_hsm_hart_wait(struct sbi_scratch *scratch)
> > > >                  * If the hsm_dev is ready and it support the hotplug, we can
> > > >                  * use the hsm stop for more power saving
> > > >                  */
> > > > -               if (hsm_device_has_hart_hotplug())
> > > > +               if (hsm_device_has_hart_hotplug()) {
> > > > +                       sbi_revert_entry_count(scratch);
> > > >                         hsm_device_hart_stop();
> > >
> > > Hi Anup,
> > > Thanks for addressing the bug =)
> > >
> > > However, there may still be an issue in the following scenario:
> > > If the HSM state of core B is SBI_HSM_STATE_START_PENDING at this
> > > point, it won't enter the warm boot path. As a result, the
> > > `init_count` will end up being `entry_count` + 1 after the warm init
> > > path.
> > >
> > > Would it make sense to increment `entry_count` after the
> > > `sbi_hsm_init()` call instead?
> >
> > This is not a bug rather expected behaviour.
> >
> > When the hart B is in SBI_HSM_STATE_START_PENDING state
> > then it means some other hart A has issued HSM hart start and
> > has also provided entry details. The hart B increments entry_count
> > upon entry into OpenSBI and increments init_count after initialization
> > is done.
> >
> I'm not sure if I understand it correctly.
> In the above case, the `init_count` != `entry_count` for hart B after
> the initialization is done.
> If the hart B enters the `hsm_dev->hart_stop()` via the
> SBI_EXT_HSM_HART_STOP path, Doesn't that mean the hart A won’t be able
> to call `hsm_dev->hart_start()` for the hart B?
> The `hsm_dev->hart_stop()` function may transition the hart into a
> platform-specific low-power state that requires
> `hsm_dev->hart_start()` to wake it up.
>
Oh, I see. I had mistaken `hsm_device_hart_stop()` for `sbi_hsm_hart_stop()`.
Since hsm_device_hart_stop() doesn't check the HSM state, hart B will
proceed to hsm_dev->hart_stop() and, upon waking up, will enter the
warm boot path.
As a result, `init_count` will match `entry_count` after hart B
completes initialization.

Apologies for the confusion. The fix looks good to me.

Reviewed-by: Nick Hu <nick.hu@sifive.com>

> Best Regards,
> Nick
>
> > Regards,
> > Anup
> >
> > >
> > > > +               }
> > > >
> > > >                 wfi();
> > > >         }
> > > > diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> > > > index 62c32682..84a63748 100644
> > > > --- a/lib/sbi/sbi_init.c
> > > > +++ b/lib/sbi/sbi_init.c
> > > > @@ -579,6 +579,19 @@ void __noreturn sbi_init(struct sbi_scratch *scratch)
> > > >                 init_warmboot(scratch, hartid);
> > > >  }
> > > >
> > > > +void sbi_revert_entry_count(struct sbi_scratch *scratch)
> > > > +{
> > > > +       unsigned long *entry_count, *init_count;
> > > > +
> > > > +       if (!entry_count_offset || !init_count_offset)
> > > > +               sbi_hart_hang();
> > > > +
> > > > +       entry_count = sbi_scratch_offset_ptr(scratch, entry_count_offset);
> > > > +       init_count = sbi_scratch_offset_ptr(scratch, init_count_offset);
> > > > +
> > > > +       *entry_count = *init_count;
> > > > +}
> > > > +
> > > >  unsigned long sbi_entry_count(u32 hartindex)
> > > >  {
> > > >         struct sbi_scratch *scratch;
> > > > --
> > > > 2.43.0
> > > >
> > >
> > > Best Regards,
> > > Nick

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH] lib: sbi: Revert entry_count before doing hsm stop in hsm wait loop

[Anup Patel]

On Tue, May 27, 2025 at 6:18 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> Using hsm stop in hsm wait loop causes secondary harts to be stuck
> forever in OpenSBI on RISC-V platforms where HSM hart hotplug is
> available and all harts come-up at the same time during system
> power-on.
>
> For example, lets say we have two harts A and B on a RISC-V platform
> with HSM hart hotplug which come-up at the same time during system
> power-on. The hart A enters OpenSBI before hart B hence it becomes
> the primary (or cold-boot) hart whereas hart B becomes the secondary
> (or warm-boot) hart. The hart A follows the OpenSBI cold-boot path
> and registers hsm device before hart B enters OpenSBI. The hart B
> eventually enters OpenSBI and follows the OpenSBI warm-boot path
> so it will increment it's own entry_count before entering hsm wait
> loop where it sees hsm device and stops itself. Later as part of
> the Linux boot-up sequence, hart A issues SBI HSM start call to
> bring-up hart B but OpenSBI sees entry_count != init_count for
> hart B in sbi_hsm_hart_start() hence hsm_device_hart_start() is
> not called for hart B resulting in hart B stuck forever in OpenSBI.
>
> To fix the above issue, revert entry_count before doing hsm stop
> in hsm wait loop.
>
> Fixes: d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  include/sbi/sbi_init.h |  2 ++
>  lib/sbi/sbi_hsm.c      |  4 +++-
>  lib/sbi/sbi_init.c     | 13 +++++++++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/include/sbi/sbi_init.h b/include/sbi/sbi_init.h
> index c9013ea4..ad068674 100644
> --- a/include/sbi/sbi_init.h
> +++ b/include/sbi/sbi_init.h
> @@ -16,6 +16,8 @@ struct sbi_scratch;
>
>  void __noreturn sbi_init(struct sbi_scratch *scratch);
>
> +void sbi_revert_entry_count(struct sbi_scratch *scratch);
> +
>  unsigned long sbi_entry_count(u32 hartindex);
>
>  unsigned long sbi_init_count(u32 hartindex);
> diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
> index e8128a39..557ab131 100644
> --- a/lib/sbi/sbi_hsm.c
> +++ b/lib/sbi/sbi_hsm.c
> @@ -176,8 +176,10 @@ static void sbi_hsm_hart_wait(struct sbi_scratch *scratch)
>                  * If the hsm_dev is ready and it support the hotplug, we can
>                  * use the hsm stop for more power saving
>                  */
> -               if (hsm_device_has_hart_hotplug())
> +               if (hsm_device_has_hart_hotplug()) {
> +                       sbi_revert_entry_count(scratch);
>                         hsm_device_hart_stop();
> +               }
>
>                 wfi();
>         }
> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index 62c32682..84a63748 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -579,6 +579,19 @@ void __noreturn sbi_init(struct sbi_scratch *scratch)
>                 init_warmboot(scratch, hartid);
>  }
>
> +void sbi_revert_entry_count(struct sbi_scratch *scratch)
> +{
> +       unsigned long *entry_count, *init_count;
> +
> +       if (!entry_count_offset || !init_count_offset)
> +               sbi_hart_hang();
> +
> +       entry_count = sbi_scratch_offset_ptr(scratch, entry_count_offset);
> +       init_count = sbi_scratch_offset_ptr(scratch, init_count_offset);
> +
> +       *entry_count = *init_count;
> +}
> +
>  unsigned long sbi_entry_count(u32 hartindex)
>  {
>         struct sbi_scratch *scratch;
> --
> 2.43.0
>

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi