* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
@ 2023-12-21 1:44 Xiang W
2023-12-25 9:24 ` Anup Patel
2023-12-27 5:46 ` Anup Patel
0 siblings, 2 replies; 10+ messages in thread
From: Xiang W @ 2023-12-21 1:44 UTC (permalink / raw)
To: opensbi
There is a problem with judging whether the current hart belongs to
hmask. If cur_hartid minus hbase is greater than BITS_PER_LONG, the
previous hmask will also have a bit cleared incorrectly, which will
cause some harts to lose ipi.
Signed-off-by: Xiang W <wxjstz@126.com>
---
lib/sbi/sbi_system.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/sbi/sbi_system.c b/lib/sbi/sbi_system.c
index d1fa349..e930272 100644
--- a/lib/sbi/sbi_system.c
+++ b/lib/sbi/sbi_system.c
@@ -72,7 +72,8 @@ void __noreturn sbi_system_reset(u32 reset_type, u32 reset_reason)
/* Send HALT IPI to every hart other than the current hart */
while (!sbi_hsm_hart_interruptible_mask(dom, hbase, &hmask)) {
- if (hbase <= cur_hartid)
+ if ((hbase <= cur_hartid)
+ && (cur_hartid < hbase + BITS_PER_LONG))
hmask &= ~(1UL << (cur_hartid - hbase));
if (hmask)
sbi_ipi_send_halt(hmask, hbase);
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
2023-12-21 1:44 [PATCH] lib: sbi: Fix shift bug in sbi_system_reset Xiang W
@ 2023-12-25 9:24 ` Anup Patel
2023-12-25 9:32 ` Xiang W
2023-12-25 9:48 ` Andreas Schwab
2023-12-27 5:46 ` Anup Patel
1 sibling, 2 replies; 10+ messages in thread
From: Anup Patel @ 2023-12-25 9:24 UTC (permalink / raw)
To: opensbi
On Thu, Dec 21, 2023 at 7:14?AM Xiang W <wxjstz@126.com> wrote:
>
> There is a problem with judging whether the current hart belongs to
> hmask. If cur_hartid minus hbase is greater than BITS_PER_LONG, the
> previous hmask will also have a bit cleared incorrectly, which will
> cause some harts to lose ipi.
>
> Signed-off-by: Xiang W <wxjstz@126.com>
> ---
> lib/sbi/sbi_system.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/sbi/sbi_system.c b/lib/sbi/sbi_system.c
> index d1fa349..e930272 100644
> --- a/lib/sbi/sbi_system.c
> +++ b/lib/sbi/sbi_system.c
> @@ -72,7 +72,8 @@ void __noreturn sbi_system_reset(u32 reset_type, u32 reset_reason)
>
> /* Send HALT IPI to every hart other than the current hart */
> while (!sbi_hsm_hart_interruptible_mask(dom, hbase, &hmask)) {
> - if (hbase <= cur_hartid)
> + if ((hbase <= cur_hartid)
> + && (cur_hartid < hbase + BITS_PER_LONG))
If "cur_hartid < hbase + BITS_PER_LONG" then
"1UL << (cur_hartid - hbase) == 0x0" so hmask
won't be impacted.
Do we still need this additional check ?
> hmask &= ~(1UL << (cur_hartid - hbase));
> if (hmask)
> sbi_ipi_send_halt(hmask, hbase);
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Regards,
Anup
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
2023-12-25 9:24 ` Anup Patel
@ 2023-12-25 9:32 ` Xiang W
2023-12-25 9:48 ` Andreas Schwab
1 sibling, 0 replies; 10+ messages in thread
From: Xiang W @ 2023-12-25 9:32 UTC (permalink / raw)
To: opensbi
? 2023-12-25???? 14:54 +0530?Anup Patel???
> On Thu, Dec 21, 2023 at 7:14?AM Xiang W <wxjstz@126.com> wrote:
> >
> > There is a problem with judging whether the current hart belongs to
> > hmask. If cur_hartid minus hbase is greater than BITS_PER_LONG, the
> > previous hmask will also have a bit cleared incorrectly, which will
> > cause some harts to lose ipi.
> >
> > Signed-off-by: Xiang W <wxjstz@126.com>
> > ---
> > ?lib/sbi/sbi_system.c | 3 ++-
> > ?1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/sbi/sbi_system.c b/lib/sbi/sbi_system.c
> > index d1fa349..e930272 100644
> > --- a/lib/sbi/sbi_system.c
> > +++ b/lib/sbi/sbi_system.c
> > @@ -72,7 +72,8 @@ void __noreturn sbi_system_reset(u32 reset_type, u32 reset_reason)
> >
> > ??????? /* Send HALT IPI to every hart other than the current hart */
> > ??????? while (!sbi_hsm_hart_interruptible_mask(dom, hbase, &hmask)) {
> > -?????????????? if (hbase <= cur_hartid)
> > +?????????????? if ((hbase <= cur_hartid)
> > +???????????????????????? && (cur_hartid < hbase + BITS_PER_LONG))
>
> If "cur_hartid < hbase + BITS_PER_LONG" then
> "1UL << (cur_hartid - hbase) == 0x0" so hmask
> won't be impacted.
>
> Do we still need this additional check ?
Yes, needed.
1UL << (cur_hartid - hbase) is equal to 1UL << ((cur_hartid - hbase) % XLEN)
is not equal to 0.
Regards,
Xiang W
>
> > ??????????????????????? hmask &= ~(1UL << (cur_hartid - hbase));
> > ??????????????? if (hmask)
> > ??????????????????????? sbi_ipi_send_halt(hmask, hbase);
> > --
> > 2.43.0
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
>
> Regards,
> Anup
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
2023-12-25 9:24 ` Anup Patel
2023-12-25 9:32 ` Xiang W
@ 2023-12-25 9:48 ` Andreas Schwab
2023-12-25 16:09 ` Xiang W
2023-12-26 15:51 ` Anup Patel
1 sibling, 2 replies; 10+ messages in thread
From: Andreas Schwab @ 2023-12-25 9:48 UTC (permalink / raw)
To: opensbi
On Dez 25 2023, Anup Patel wrote:
> If "cur_hartid < hbase + BITS_PER_LONG" then
> "1UL << (cur_hartid - hbase) == 0x0"
If cur_hartid - hbase < BITS_PER_LONG, then 1UL << (cur_hartid - hbase)
will not be 0. If cur_hartid - hbase >= BITS_PER_LONG, then 1UL <<
(cur_hartid - hbase) will overflow and be undefined.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
2023-12-25 9:48 ` Andreas Schwab
@ 2023-12-25 16:09 ` Xiang W
2023-12-26 15:51 ` Anup Patel
1 sibling, 0 replies; 10+ messages in thread
From: Xiang W @ 2023-12-25 16:09 UTC (permalink / raw)
To: opensbi
? 2023-12-25???? 10:48 +0100?Andreas Schwab???
> On Dez 25 2023, Anup Patel wrote:
>
> > If "cur_hartid < hbase + BITS_PER_LONG" then
> > "1UL << (cur_hartid - hbase) == 0x0"
>
> If cur_hartid - hbase < BITS_PER_LONG, then 1UL << (cur_hartid - hbase)
> will not be 0.? If cur_hartid - hbase >= BITS_PER_LONG, then 1UL <<
> (cur_hartid - hbase) will overflow and be undefined.
>
Andreas is right. C99 semantics require that the result of a shift overflow
is undefined. Under RISCV, 1<< n is equal to 1<<(n%XLAN).
Regards,
Xiang W
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
2023-12-25 9:48 ` Andreas Schwab
2023-12-25 16:09 ` Xiang W
@ 2023-12-26 15:51 ` Anup Patel
2023-12-26 18:03 ` Samuel Holland
1 sibling, 1 reply; 10+ messages in thread
From: Anup Patel @ 2023-12-26 15:51 UTC (permalink / raw)
To: opensbi
On Mon, Dec 25, 2023 at 3:19?PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Dez 25 2023, Anup Patel wrote:
>
> > If "cur_hartid < hbase + BITS_PER_LONG" then
> > "1UL << (cur_hartid - hbase) == 0x0"
>
> If cur_hartid - hbase < BITS_PER_LONG, then 1UL << (cur_hartid - hbase)
> will not be 0. If cur_hartid - hbase >= BITS_PER_LONG, then 1UL <<
> (cur_hartid - hbase) will overflow and be undefined.
I meant "cur_hartid >= hbase + BITS_PER_LONG" which is
the overflow case.
Since the overflow behavior is undefined, we have the
explicit check added by this patch.
Regards,
Anup
>
> --
> Andreas Schwab, schwab at linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
2023-12-26 15:51 ` Anup Patel
@ 2023-12-26 18:03 ` Samuel Holland
2023-12-27 1:54 ` Xiang W
0 siblings, 1 reply; 10+ messages in thread
From: Samuel Holland @ 2023-12-26 18:03 UTC (permalink / raw)
To: opensbi
On 2023-12-26 9:51 AM, Anup Patel wrote:
> On Mon, Dec 25, 2023 at 3:19?PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> On Dez 25 2023, Anup Patel wrote:
>>
>>> If "cur_hartid < hbase + BITS_PER_LONG" then
>>> "1UL << (cur_hartid - hbase) == 0x0"
>>
>> If cur_hartid - hbase < BITS_PER_LONG, then 1UL << (cur_hartid - hbase)
>> will not be 0. If cur_hartid - hbase >= BITS_PER_LONG, then 1UL <<
>> (cur_hartid - hbase) will overflow and be undefined.
>
> I meant "cur_hartid >= hbase + BITS_PER_LONG" which is
> the overflow case.
>
> Since the overflow behavior is undefined, we have the
> explicit check added by this patch.
As pointed out by Andreas, "cur_hartid - hbase < BITS_PER_LONG" is the only
check needed here. Unsigned wraparound will take care of the case where
cur_hartid < hbase.
Regards,
Samuel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
2023-12-26 18:03 ` Samuel Holland
@ 2023-12-27 1:54 ` Xiang W
2023-12-27 5:45 ` Anup Patel
0 siblings, 1 reply; 10+ messages in thread
From: Xiang W @ 2023-12-27 1:54 UTC (permalink / raw)
To: opensbi
? 2023-12-26???? 12:03 -0600?Samuel Holland???
> On 2023-12-26 9:51 AM, Anup Patel wrote:
> > On Mon, Dec 25, 2023 at 3:19?PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > >
> > > On Dez 25 2023, Anup Patel wrote:
> > >
> > > > If "cur_hartid < hbase + BITS_PER_LONG" then
> > > > "1UL << (cur_hartid - hbase) == 0x0"
> > >
> > > If cur_hartid - hbase < BITS_PER_LONG, then 1UL << (cur_hartid - hbase)
> > > will not be 0.? If cur_hartid - hbase >= BITS_PER_LONG, then 1UL <<
> > > (cur_hartid - hbase) will overflow and be undefined.
> >
> > I meant "cur_hartid >= hbase + BITS_PER_LONG" which is
> > the overflow case.
> >
> > Since the overflow behavior is undefined, we have the
> > explicit check added by this patch.
>
> As pointed out by Andreas, "cur_hartid - hbase < BITS_PER_LONG" is the only
> check needed here. Unsigned wraparound will take care of the case where
> cur_hartid < hbase.
cur_hartid < hbase is not always true.
For example, cur_hartid is 3 and 0-100 belongs to a domain. Here the loop
is executed twice, the first time with hbase 0 and the second time with
hbase 64. The second loop will have cur_hartid < hbase.
Regards,
Xiang W
>
> Regards,
> Samuel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
2023-12-27 1:54 ` Xiang W
@ 2023-12-27 5:45 ` Anup Patel
0 siblings, 0 replies; 10+ messages in thread
From: Anup Patel @ 2023-12-27 5:45 UTC (permalink / raw)
To: opensbi
On Wed, Dec 27, 2023 at 7:25?AM Xiang W <wxjstz@126.com> wrote:
>
> ? 2023-12-26???? 12:03 -0600?Samuel Holland???
> > On 2023-12-26 9:51 AM, Anup Patel wrote:
> > > On Mon, Dec 25, 2023 at 3:19?PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > > >
> > > > On Dez 25 2023, Anup Patel wrote:
> > > >
> > > > > If "cur_hartid < hbase + BITS_PER_LONG" then
> > > > > "1UL << (cur_hartid - hbase) == 0x0"
> > > >
> > > > If cur_hartid - hbase < BITS_PER_LONG, then 1UL << (cur_hartid - hbase)
> > > > will not be 0. If cur_hartid - hbase >= BITS_PER_LONG, then 1UL <<
> > > > (cur_hartid - hbase) will overflow and be undefined.
> > >
> > > I meant "cur_hartid >= hbase + BITS_PER_LONG" which is
> > > the overflow case.
> > >
> > > Since the overflow behavior is undefined, we have the
> > > explicit check added by this patch.
> >
> > As pointed out by Andreas, "cur_hartid - hbase < BITS_PER_LONG" is the only
> > check needed here. Unsigned wraparound will take care of the case where
> > cur_hartid < hbase.
> cur_hartid < hbase is not always true.
>
> For example, cur_hartid is 3 and 0-100 belongs to a domain. Here the loop
> is executed twice, the first time with hbase 0 and the second time with
> hbase 64. The second loop will have cur_hartid < hbase.
The real issue is that overflow behaviour of the left shift operator is
undefined. No need for further justification on this issue.
Regards,
Anup
>
> Regards,
> Xiang W
> >
> > Regards,
> > Samuel
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] lib: sbi: Fix shift bug in sbi_system_reset
2023-12-21 1:44 [PATCH] lib: sbi: Fix shift bug in sbi_system_reset Xiang W
2023-12-25 9:24 ` Anup Patel
@ 2023-12-27 5:46 ` Anup Patel
1 sibling, 0 replies; 10+ messages in thread
From: Anup Patel @ 2023-12-27 5:46 UTC (permalink / raw)
To: opensbi
On Thu, Dec 21, 2023 at 7:14?AM Xiang W <wxjstz@126.com> wrote:
>
> There is a problem with judging whether the current hart belongs to
> hmask. If cur_hartid minus hbase is greater than BITS_PER_LONG, the
> previous hmask will also have a bit cleared incorrectly, which will
> cause some harts to lose ipi.
>
> Signed-off-by: Xiang W <wxjstz@126.com>
Looks good to me.
Reviewed-by: Anup Patel <anup@brainfault.org>
Applied this patch to the riscv/opensbi repo.
Thanks,
Anup
> ---
> lib/sbi/sbi_system.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/sbi/sbi_system.c b/lib/sbi/sbi_system.c
> index d1fa349..e930272 100644
> --- a/lib/sbi/sbi_system.c
> +++ b/lib/sbi/sbi_system.c
> @@ -72,7 +72,8 @@ void __noreturn sbi_system_reset(u32 reset_type, u32 reset_reason)
>
> /* Send HALT IPI to every hart other than the current hart */
> while (!sbi_hsm_hart_interruptible_mask(dom, hbase, &hmask)) {
> - if (hbase <= cur_hartid)
> + if ((hbase <= cur_hartid)
> + && (cur_hartid < hbase + BITS_PER_LONG))
> hmask &= ~(1UL << (cur_hartid - hbase));
> if (hmask)
> sbi_ipi_send_halt(hmask, hbase);
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-12-27 5:46 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-21 1:44 [PATCH] lib: sbi: Fix shift bug in sbi_system_reset Xiang W
2023-12-25 9:24 ` Anup Patel
2023-12-25 9:32 ` Xiang W
2023-12-25 9:48 ` Andreas Schwab
2023-12-25 16:09 ` Xiang W
2023-12-26 15:51 ` Anup Patel
2023-12-26 18:03 ` Samuel Holland
2023-12-27 1:54 ` Xiang W
2023-12-27 5:45 ` Anup Patel
2023-12-27 5:46 ` Anup Patel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox