[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[Xiang W]

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

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[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 ?

>                         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

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[Xiang W]

? 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

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[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 Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[Xiang W]

? 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

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[Anup Patel]

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

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[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.

Regards,
Samuel

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[Xiang W]

? 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

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[Anup Patel]

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

[PATCH] lib: sbi: Fix shift bug in sbi_system_reset

[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>

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