lib: sbi: Revert entry_count before doing hsm stop in hsm wait loop
PATCH 1/1 lib: sbi: Revert entry_count before doing hsm stop in hsm wait loop
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.
--- 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(-)
include/sbi/sbi_init.h +2 -0
@@ -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);
lib/sbi/sbi_hsm.c +3 -1
@@ -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 (); }
lib/sbi/sbi_init.c +13 -0
@@ -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;
Signature · 7 lines
2.43.0 -- opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
Quoted 49 lines · 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_countbefore 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 seesentry_count!=init_countfor hart B insbi_hsm_hart_start()hencehsm_device_hart_start()is not called for hart B resulting in hart B stuck forever in OpenSBI.To fix the above issue, revert
entry_countbefore doing hsm stop in hsm wait loop.Fixes d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")Signed-off-by Anup Patel <[email protected]>--- 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(-)include/sbi/sbi_init.h +2
-0@@ -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);lib/sbi/sbi_hsm.c +0
-0
lib/sbi/sbi_hsm.c static void sbi_hsm_hart_wait(struct sbi_scratch *scratch) · line 181
Quoted 7 lines
lib/sbi/sbi_hsm.c +2
-1@@ -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?
Quoted 29 lines
lib/sbi/sbi_hsm.c +1
-0+ }wfi (); }lib/sbi/sbi_init.c +13
-1@@ -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; --
Quoted 2 lines
2.43.0
Best Regards,
Nick
Signature · 3 lines
opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
Quoted 62 lines · Nick Hu
Quoted 49 lines · 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_countbefore 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 seesentry_count!=init_countfor hart B insbi_hsm_hart_start()hencehsm_device_hart_start()is not called for hart B resulting in hart B stuck forever in OpenSBI.To fix the above issue, revert
entry_countbefore doing hsm stop in hsm wait loop.Fixes d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")Signed-off-by Anup Patel <[email protected]>--- 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(-)include/sbi/sbi_init.h +2
-0@@ -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);lib/sbi/sbi_hsm.c +0
-0lib/sbi/sbi_hsm.c static void sbi_hsm_hart_wait(struct sbi_scratch *scratch) · line 181
Quoted 7 lines
lib/sbi/sbi_hsm.c +2
-1@@ -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 =)
Quoted 8 lines
However, there may still be an issue in the following scenario:
If the HSM state of core B isSBI_HSM_STATE_START_PENDINGat this point, it won't enter the warm boot path. As a result, theinit_countwill end up beingentry_count+ 1 after the warm init path.Would it make sense to increment
entry_countafter thesbi_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
Quoted 35 lines
Quoted 30 lines
lib/sbi/sbi_hsm.c +1
-0+ }wfi (); }lib/sbi/sbi_init.c +13
-1@@ -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; --Quoted 2 lines
2.43.0
Best Regards,
Nick
Signature · 3 lines
opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
Quoted 73 lines · Anup Patel
Quoted 62 lines · Nick Hu
Quoted 49 lines · 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_countbefore 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 seesentry_count!=init_countfor hart B insbi_hsm_hart_start()hencehsm_device_hart_start()is not called for hart B resulting in hart B stuck forever in OpenSBI.To fix the above issue, revert
entry_countbefore doing hsm stop in hsm wait loop.Fixes d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")Signed-off-by Anup Patel <[email protected]>--- 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(-)include/sbi/sbi_init.h +2
-0@@ -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);lib/sbi/sbi_hsm.c +0
-0lib/sbi/sbi_hsm.c static void sbi_hsm_hart_wait(struct sbi_scratch *scratch) · line 181
Quoted 7 lines
lib/sbi/sbi_hsm.c +2
-1@@ -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 =)Quoted 8 lines
However, there may still be an issue in the following scenario:
If the HSM state of core B isSBI_HSM_STATE_START_PENDINGat this point, it won't enter the warm boot path. As a result, theinit_countwill end up beingentry_count+ 1 after the warm init path.Would it make sense to increment
entry_countafter thesbi_hsm_init()call instead?
Quoted 8 lines
This is not a bug rather expected behaviour.
When the hart B is in
SBI_HSM_STATE_START_PENDINGstate then it means some other hart A has issued HSM hart start and has also provided entry details. The hart B incrementsentry_countupon entry into OpenSBI and incrementsinit_countafter 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
Quoted 38 lines
Regards,
AnupQuoted 35 lines
Quoted 30 lines
lib/sbi/sbi_hsm.c +1
-0+ }wfi (); }lib/sbi/sbi_init.c +13
-1@@ -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; --Quoted 2 lines
2.43.0
Best Regards,
Nick
Signature · 3 lines
opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
Nick Hu
Reviewed-byQuoted 83 lines · Nick Hu
Quoted 73 lines · Anup Patel
Quoted 62 lines · Nick Hu
Quoted 49 lines · 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_countbefore 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 seesentry_count!=init_countfor hart B insbi_hsm_hart_start()hencehsm_device_hart_start()is not called for hart B resulting in hart B stuck forever in OpenSBI.To fix the above issue, revert
entry_countbefore doing hsm stop in hsm wait loop.Fixes d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")Signed-off-by Anup Patel <[email protected]>--- 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(-)include/sbi/sbi_init.h +2
-0@@ -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);lib/sbi/sbi_hsm.c +0
-0lib/sbi/sbi_hsm.c static void sbi_hsm_hart_wait(struct sbi_scratch *scratch) · line 181
Quoted 7 lines
lib/sbi/sbi_hsm.c +2
-1@@ -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 =)Quoted 8 lines
However, there may still be an issue in the following scenario:
If the HSM state of core B isSBI_HSM_STATE_START_PENDINGat this point, it won't enter the warm boot path. As a result, theinit_countwill end up beingentry_count+ 1 after the warm init path.Would it make sense to increment
entry_countafter thesbi_hsm_init()call instead?Quoted 8 lines
This is not a bug rather expected behaviour.
When the hart B is in
SBI_HSM_STATE_START_PENDINGstate then it means some other hart A has issued HSM hart start and has also provided entry details. The hart B incrementsentry_countupon entry into OpenSBI and incrementsinit_countafter initialization is done.
Quoted 10 lines
I'm not sure if I understand it correctly.
In the above case, theinit_count!=entry_countfor hart B after the initialization is done.
If the hart B enters thehsm_dev->hart_stop()via theSBI_EXT_HSM_HART_STOPpath, Doesn't that mean the hart A won’t be able to callhsm_dev->hart_start()for the hart B?
Thehsm_dev->hart_stop()function may transition the hart into a platform-specific low-power state that requireshsm_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.
Quoted 41 lines
Best Regards,
NickQuoted 38 lines
Regards,
AnupQuoted 35 lines
Quoted 30 lines
lib/sbi/sbi_hsm.c +1
-0+ }wfi (); }lib/sbi/sbi_init.c +13
-1@@ -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; --Quoted 2 lines
2.43.0
Best Regards,
Nick
Signature · 3 lines
opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
Anup Patel
AppliedQuoted 20 lines · 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_countbefore 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 seesentry_count!=init_countfor hart B insbi_hsm_hart_start()hencehsm_device_hart_start()is not called for hart B resulting in hart B stuck forever in OpenSBI.
Quoted 5 lines
To fix the above issue, revert
entry_countbefore doing hsm stop in hsm wait loop.Fixes d844deadec94 ("lib: sbi: Use hsm stop for hsm wait")Signed-off-by Anup Patel <[email protected]>
Applied this patch to the riscv/opensbi repo.
Thanks,
Anup
Quoted 62 lines
--- 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(-)include/sbi/sbi_init.h +2
-0@@ -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);lib/sbi/sbi_hsm.c +3
-1@@ -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 (); }lib/sbi/sbi_init.c +13
-1@@ -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
Signature · 3 lines
opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi