| Message ID | 20231208212301.3649930-3-samuel.holland@sifive.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | sbi_ipi bug fixes and optimizations | expand |
On Sat, Dec 9, 2023 at 2:53 AM Samuel Holland <samuel.holland@sifive.com> wrote: > > An IPI sent to the local hart can be processed directly instead of > triggering the IPI device. This is more efficient, and it avoids a > deadlock when the .sync callback is defined. Since interrupts are > disabled while handling an ecall, the IPI would not get delivered > until the next mret, but sbi_ipi_sync() is called before then. > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > > lib/sbi/sbi_ipi.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/lib/sbi/sbi_ipi.c b/lib/sbi/sbi_ipi.c > index 1fd24228..ac31cd94 100644 > --- a/lib/sbi/sbi_ipi.c > +++ b/lib/sbi/sbi_ipi.c > @@ -60,6 +60,14 @@ static int sbi_ipi_send(struct sbi_scratch *scratch, u32 remote_hartindex, > remote_hartindex, data); > if (ret != SBI_IPI_UPDATE_SUCCESS) > return ret; > + } else if (scratch == remote_scratch) { > + /* > + * IPI events with an update() callback are expected to return > + * SBI_IPI_UPDATE_BREAK for self-IPIs. For other events, check > + * for self-IPI and execute the callback directly here. > + */ > + ipi_ops->process(scratch); > + return 0; > } > > /* > -- > 2.42.0 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
在 2023-12-08星期五的 13:22 -0800，Samuel Holland写道： > An IPI sent to the local hart can be processed directly instead of > triggering the IPI device. This is more efficient, and it avoids a > deadlock when the .sync callback is defined. Since interrupts are > disabled while handling an ecall, the IPI would not get delivered > until the next mret, but sbi_ipi_sync() is called before then. > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com> > --- > > lib/sbi/sbi_ipi.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/lib/sbi/sbi_ipi.c b/lib/sbi/sbi_ipi.c > index 1fd24228..ac31cd94 100644 > --- a/lib/sbi/sbi_ipi.c > +++ b/lib/sbi/sbi_ipi.c > @@ -60,6 +60,14 @@ static int sbi_ipi_send(struct sbi_scratch *scratch, u32 remote_hartindex, > remote_hartindex, data); > if (ret != SBI_IPI_UPDATE_SUCCESS) > return ret; > + } else if (scratch == remote_scratch) { > + /* > + * IPI events with an update() callback are expected to return > + * SBI_IPI_UPDATE_BREAK for self-IPIs. For other events, check > + * for self-IPI and execute the callback directly here. > + */ > + ipi_ops->process(scratch); > + return 0; This modification will affect sbi_tlb.c. tlb_update has already called tinfo->local_fn(tinfo) when returning SBI_IPI_UPDATE_BREAK. And the synchronization variable is only updated when SBI_IPI_UPDATE_SUCCESS is returned. If tlb_process is called, the synchronization variable will be affected. Regards, Xiang W > } > > /* > -- > 2.42.0 > >
在 2023-12-11星期一的 15:24 +0800，Xiang W写道： > 在 2023-12-08星期五的 13:22 -0800，Samuel Holland写道： > > An IPI sent to the local hart can be processed directly instead of > > triggering the IPI device. This is more efficient, and it avoids a > > deadlock when the .sync callback is defined. Since interrupts are > > disabled while handling an ecall, the IPI would not get delivered > > until the next mret, but sbi_ipi_sync() is called before then. > > > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com> > > --- > > > > lib/sbi/sbi_ipi.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/lib/sbi/sbi_ipi.c b/lib/sbi/sbi_ipi.c > > index 1fd24228..ac31cd94 100644 > > --- a/lib/sbi/sbi_ipi.c > > +++ b/lib/sbi/sbi_ipi.c > > @@ -60,6 +60,14 @@ static int sbi_ipi_send(struct sbi_scratch *scratch, u32 remote_hartindex, > > remote_hartindex, data); > > if (ret != SBI_IPI_UPDATE_SUCCESS) > > return ret; > > + } else if (scratch == remote_scratch) { > > + /* > > + * IPI events with an update() callback are expected to return > > + * SBI_IPI_UPDATE_BREAK for self-IPIs. For other events, check > > + * for self-IPI and execute the callback directly here. > > + */ > > + ipi_ops->process(scratch); > > + return 0; > This modification will affect sbi_tlb.c. tlb_update has already called > tinfo->local_fn(tinfo) when returning SBI_IPI_UPDATE_BREAK. And the > synchronization variable is only updated when SBI_IPI_UPDATE_SUCCESS is > returned. If tlb_process is called, the synchronization variable will be > affected. > > Regards, > Xiang W I made a mistake! I thought ipi_ops->process(scratch) was called when SBI_IPI_UPDATE_BREAK was returned. I ignored that there is no ipi_ops->update when it doesn't exist. Look good to me. Reviewed-by: Xiang W <wxjstz@126.com> > > > } > > > > /* > > -- > > 2.42.0 > > > > > >
On Sat, Dec 9, 2023 at 2:53 AM Samuel Holland <samuel.holland@sifive.com> wrote: > > An IPI sent to the local hart can be processed directly instead of > triggering the IPI device. This is more efficient, and it avoids a > deadlock when the .sync callback is defined. Since interrupts are > disabled while handling an ecall, the IPI would not get delivered > until the next mret, but sbi_ipi_sync() is called before then. > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com> Applied this patch to the riscv/opensbi repo. Thanks, Anup > --- > > lib/sbi/sbi_ipi.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/lib/sbi/sbi_ipi.c b/lib/sbi/sbi_ipi.c > index 1fd24228..ac31cd94 100644 > --- a/lib/sbi/sbi_ipi.c > +++ b/lib/sbi/sbi_ipi.c > @@ -60,6 +60,14 @@ static int sbi_ipi_send(struct sbi_scratch *scratch, u32 remote_hartindex, > remote_hartindex, data); > if (ret != SBI_IPI_UPDATE_SUCCESS) > return ret; > + } else if (scratch == remote_scratch) { > + /* > + * IPI events with an update() callback are expected to return > + * SBI_IPI_UPDATE_BREAK for self-IPIs. For other events, check > + * for self-IPI and execute the callback directly here. > + */ > + ipi_ops->process(scratch); > + return 0; > } > > /* > -- > 2.42.0 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
diff --git a/lib/sbi/sbi_ipi.c b/lib/sbi/sbi_ipi.c index 1fd24228..ac31cd94 100644 --- a/lib/sbi/sbi_ipi.c +++ b/lib/sbi/sbi_ipi.c @@ -60,6 +60,14 @@ static int sbi_ipi_send(struct sbi_scratch *scratch, u32 remote_hartindex, remote_hartindex, data); if (ret != SBI_IPI_UPDATE_SUCCESS) return ret; + } else if (scratch == remote_scratch) { + /* + * IPI events with an update() callback are expected to return + * SBI_IPI_UPDATE_BREAK for self-IPIs. For other events, check + * for self-IPI and execute the callback directly here. + */ + ipi_ops->process(scratch); + return 0; } /*
An IPI sent to the local hart can be processed directly instead of triggering the IPI device. This is more efficient, and it avoids a deadlock when the .sync callback is defined. Since interrupts are disabled while handling an ecall, the IPI would not get delivered until the next mret, but sbi_ipi_sync() is called before then. Signed-off-by: Samuel Holland <samuel.holland@sifive.com> --- lib/sbi/sbi_ipi.c | 8 ++++++++ 1 file changed, 8 insertions(+)