| Message ID | 20231221014432.393824-1-wxjstz@126.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lib: sbi: Fix shift bug in sbi_system_reset | expand |
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi Regards, Anup
在 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@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi > > Regards, > Anup
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.
在 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
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@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/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
在 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
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
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);
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(-)