[PATCH v2 1/3] lib: sbi: Introduce IPI device rating
Anup Patel
anup at brainfault.org
Wed Sep 3 08:08:44 PDT 2025
On Wed, Sep 3, 2025 at 8:26 PM Samuel Holland <samuel.holland at sifive.com> wrote:
>
> Hi Anup,
>
> On 2025-09-03 1:52 AM, Anup Patel wrote:
> > A platform can have multiple IPI devices (such as ACLINT MSWI,
> > AIA IMSIC, etc). Currently, OpenSBI rely on platform calling
> > the sbi_ipi_set_device() function in correct order and prefer
> > the first avaiable IPI device which is fragile.
> >
> > Instead of the above, introdcue IPI device rating and prefer
> > the highest rated IPI device. This further allows extending
> > the sbi_ipi_raw_clear() to clear all available IPI devices.
> >
> > Signed-off-by: Anup Patel <apatel at ventanamicro.com>
> > ---
> > include/sbi/sbi_ipi.h | 5 ++++-
> > lib/sbi/sbi_init.c | 2 +-
> > lib/sbi/sbi_ipi.c | 41 ++++++++++++++++++++++++++++++------
> > lib/utils/ipi/aclint_mswi.c | 1 +
> > lib/utils/ipi/andes_plicsw.c | 1 +
> > lib/utils/irqchip/imsic.c | 1 +
> > 6 files changed, 42 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/sbi/sbi_ipi.h b/include/sbi/sbi_ipi.h
> > index 62d61304..620a95cf 100644
> > --- a/include/sbi/sbi_ipi.h
> > +++ b/include/sbi/sbi_ipi.h
> > @@ -23,6 +23,9 @@ struct sbi_ipi_device {
> > /** Name of the IPI device */
> > char name[32];
> >
> > + /** Ratings of the IPI device (higher is better) */
> > + unsigned long rating;
> > +
> > /** Send IPI to a target HART index */
> > void (*ipi_send)(u32 hart_index);
> >
> > @@ -87,7 +90,7 @@ void sbi_ipi_process(void);
> >
> > int sbi_ipi_raw_send(u32 hartindex);
> >
> > -void sbi_ipi_raw_clear(void);
> > +void sbi_ipi_raw_clear(bool all_devices);
> >
> > const struct sbi_ipi_device *sbi_ipi_get_device(void);
> >
> > diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> > index 84a63748..663b486b 100644
> > --- a/lib/sbi/sbi_init.c
> > +++ b/lib/sbi/sbi_init.c
> > @@ -507,7 +507,7 @@ static void __noreturn init_warmboot(struct sbi_scratch *scratch, u32 hartid)
> > if (hstate == SBI_HSM_STATE_SUSPENDED) {
> > init_warm_resume(scratch, hartid);
> > } else {
> > - sbi_ipi_raw_clear();
> > + sbi_ipi_raw_clear(true);
> > init_warm_startup(scratch, hartid);
> > }
> > }
> > diff --git a/lib/sbi/sbi_ipi.c b/lib/sbi/sbi_ipi.c
> > index 2de459b0..d7ba1902 100644
> > --- a/lib/sbi/sbi_ipi.c
> > +++ b/lib/sbi/sbi_ipi.c
> > @@ -15,9 +15,11 @@
> > #include <sbi/sbi_domain.h>
> > #include <sbi/sbi_error.h>
> > #include <sbi/sbi_hart.h>
> > +#include <sbi/sbi_heap.h>
> > #include <sbi/sbi_hsm.h>
> > #include <sbi/sbi_init.h>
> > #include <sbi/sbi_ipi.h>
> > +#include <sbi/sbi_list.h>
> > #include <sbi/sbi_platform.h>
> > #include <sbi/sbi_pmu.h>
> > #include <sbi/sbi_string.h>
> > @@ -32,8 +34,14 @@ _Static_assert(
> > "type of sbi_ipi_data.ipi_type has changed, please redefine SBI_IPI_EVENT_MAX"
> > );
> >
> > +struct sbi_ipi_device_node {
> > + struct sbi_dlist head;
> > + const struct sbi_ipi_device *dev;
> > +};
> > +
> > static unsigned long ipi_data_off;
> > static const struct sbi_ipi_device *ipi_dev = NULL;
> > +static SBI_LIST_HEAD(ipi_dev_node_list);
> > static const struct sbi_ipi_event_ops *ipi_ops_array[SBI_IPI_EVENT_MAX];
> >
> > static int sbi_ipi_send(struct sbi_scratch *scratch, u32 remote_hartindex,
> > @@ -248,7 +256,7 @@ void sbi_ipi_process(void)
> > sbi_scratch_offset_ptr(scratch, ipi_data_off);
> >
> > sbi_pmu_ctr_incr_fw(SBI_PMU_FW_IPI_RECVD);
> > - sbi_ipi_raw_clear();
> > + sbi_ipi_raw_clear(false);
> >
> > ipi_type = atomic_raw_xchg_ulong(&ipi_data->ipi_type, 0);
> > ipi_event = 0;
> > @@ -283,10 +291,19 @@ int sbi_ipi_raw_send(u32 hartindex)
> > return 0;
> > }
> >
> > -void sbi_ipi_raw_clear(void)
> > +void sbi_ipi_raw_clear(bool all_devices)
> > {
> > - if (ipi_dev && ipi_dev->ipi_clear)
> > - ipi_dev->ipi_clear();
> > + struct sbi_ipi_device_node *entry;
> > +
> > + if (all_devices) {
> > + sbi_list_for_each_entry(entry, &ipi_dev_node_list, head) {
> > + if (entry->dev->ipi_clear)
> > + entry->dev->ipi_clear();
> > + }
> > + } else {
> > + if (ipi_dev && ipi_dev->ipi_clear)
> > + ipi_dev->ipi_clear();
> > + }
> >
> > /*
> > * Ensure that memory or MMIO writes after this
> > @@ -307,10 +324,20 @@ const struct sbi_ipi_device *sbi_ipi_get_device(void)
> >
> > void sbi_ipi_set_device(const struct sbi_ipi_device *dev)
>
> Maybe this should be renamed to sbi_ipi_add_device(), since we can now track
> more than one device.
I agree. I will update in the next revision.
>
> > {
> > - if (!dev || ipi_dev)
> > + struct sbi_ipi_device_node *entry;
> > +
> > + if (!dev)
> > + return;
> > +
> > + entry = sbi_zalloc(sizeof(*entry));
> > + if (!entry)
> > return;
> > + SBI_INIT_LIST_HEAD(&entry->head);
> > + entry->dev = dev;
> > + sbi_list_add_tail(&entry->head, &ipi_dev_node_list);
>
> Why not put the list node directly in `struct sbi_ipi_device`, instead of using
> a separate allocation? Or is there a reason why that would not work?
The parameter passed to sbi_ipi_set_device() is const and
we should keep it that way.
>
> Regards,
> Samuel
>
> >
> > - ipi_dev = dev;
> > + if (!ipi_dev || ipi_dev->rating < dev->rating)
> > + ipi_dev = dev;
> > }
> >
> > int sbi_ipi_init(struct sbi_scratch *scratch, bool cold_boot)
> > @@ -347,7 +374,7 @@ int sbi_ipi_init(struct sbi_scratch *scratch, bool cold_boot)
> > ipi_data->ipi_type = 0x00;
> >
> > /* Clear any pending IPIs for the current hart */
> > - sbi_ipi_raw_clear();
> > + sbi_ipi_raw_clear(true);
> >
> > /* Enable software interrupts */
> > csr_set(CSR_MIE, MIP_MSIP);
> > diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
> > index 9e55078a..23510551 100644
> > --- a/lib/utils/ipi/aclint_mswi.c
> > +++ b/lib/utils/ipi/aclint_mswi.c
> > @@ -62,6 +62,7 @@ static void mswi_ipi_clear(void)
> >
> > static struct sbi_ipi_device aclint_mswi = {
> > .name = "aclint-mswi",
> > + .rating = 100,
> > .ipi_send = mswi_ipi_send,
> > .ipi_clear = mswi_ipi_clear
> > };
> > diff --git a/lib/utils/ipi/andes_plicsw.c b/lib/utils/ipi/andes_plicsw.c
> > index 5d085d85..000fe9fd 100644
> > --- a/lib/utils/ipi/andes_plicsw.c
> > +++ b/lib/utils/ipi/andes_plicsw.c
> > @@ -61,6 +61,7 @@ static void plicsw_ipi_clear(void)
> >
> > static struct sbi_ipi_device plicsw_ipi = {
> > .name = "andes_plicsw",
> > + .rating = 200,
> > .ipi_send = plicsw_ipi_send,
> > .ipi_clear = plicsw_ipi_clear
> > };
> > diff --git a/lib/utils/irqchip/imsic.c b/lib/utils/irqchip/imsic.c
> > index 057b9fa7..3c150c37 100644
> > --- a/lib/utils/irqchip/imsic.c
> > +++ b/lib/utils/irqchip/imsic.c
> > @@ -199,6 +199,7 @@ static void imsic_ipi_send(u32 hart_index)
> >
> > static struct sbi_ipi_device imsic_ipi_device = {
> > .name = "aia-imsic",
> > + .rating = 300,
> > .ipi_send = imsic_ipi_send
> > };
> >
>
Regards
Anup
More information about the opensbi
mailing list