[PATCH v3 3/5] lib: sbi: sse: rework event availability
Clément Léger
cleger at rivosinc.com
Mon Dec 16 01:25:04 PST 2024
On 15/12/2024 14:11, Anup Patel wrote:
> On Fri, Dec 13, 2024 at 2:04 AM Clément Léger <cleger at rivosinc.com> wrote:
>>
>> In order to detect events that are available and supported, add a new
>> is_supported_cb() callback and move cb_ops in event_info.
>>
>> The cb_ops are now global for a specific event_id and can be filled by
>> drivers before calling sse init. sse_init() will then the callback to
>> determine which events are available. Global events are checked only
>> once and local_events are checked per hart. Thanks to that, we can now
>> allocate only the supported global and local events and defer sse
>> probing. All events are assigned a default callback operations that
>> reports true for the is_supported_cb() to keep the existing behavior.
>> Since the callback operations can now be registered once for all
>> harts, move the pmu sse_set_cb_ops() callback to the cold_boot 'if'.
>>
>> Checking the new additional is_supported_cb() is only done when we
>> fail from getting an event so that there is no performance cost.
>>
>> Signed-off-by: Clément Léger <cleger at rivosinc.com>
>>
>> lib: sbi: sse: add a is_supported_cb() callback
>>
>> This callback will be used to expose the availability of events based on
>> the presence of the dependencies (PMU Overflow IRQ for the PMU Overflow
>> SSE event for instance). Drivers that want to handle an SSE event will
>> need to register callbacks for the SSE event with at least the
>> is_supported_cb() callback implemented. Now that this callback is
>> implemented, set all non software events as not supported by default.
>>
>> Signed-off-by: Clément Léger <cleger at rivosinc.com>
>> ---
>> include/sbi/sbi_sse.h | 5 ++
>> lib/sbi/sbi_init.c | 21 ++---
>> lib/sbi/sbi_pmu.c | 4 +-
>> lib/sbi/sbi_sse.c | 182 ++++++++++++++++++++++++++++++++----------
>> 4 files changed, 157 insertions(+), 55 deletions(-)
>>
>> diff --git a/include/sbi/sbi_sse.h b/include/sbi/sbi_sse.h
>> index 7419698a..1eab4f8b 100644
>> --- a/include/sbi/sbi_sse.h
>> +++ b/include/sbi/sbi_sse.h
>> @@ -52,6 +52,11 @@ struct sbi_sse_cb_ops {
>> * Called when the SBI_EXT_SSE_DISABLE is invoked on the event.
>> */
>> void (*disable_cb)(uint32_t event_id);
>> +
>> + /**
>> + * Called at init to check if an event is supported.
>> + */
>> + bool (*is_supported_cb)(uint32_t event_id);
>> };
>>
>> /* Set the callback operations for an event
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index 0736345d..b2627e60 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -262,12 +262,6 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
>> if (rc)
>> sbi_hart_hang();
>>
>> - rc = sbi_sse_init(scratch, true);
>> - if (rc) {
>> - sbi_printf("%s: sse init failed (error %d)\n", __func__, rc);
>> - sbi_hart_hang();
>> - }
>> -
>> rc = sbi_pmu_init(scratch, true);
>> if (rc) {
>> sbi_printf("%s: pmu init failed (error %d)\n",
>> @@ -317,6 +311,13 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
>> sbi_printf("%s: mpxy init failed (error %d)\n", __func__, rc);
>> sbi_hart_hang();
>> }
>> +
>> + rc = sbi_sse_init(scratch, true);
>> + if (rc) {
>> + sbi_printf("%s: sse init failed (error %d)\n", __func__, rc);
>> + sbi_hart_hang();
>> + }
>> +
>> /*
>> * Note: Finalize domains after HSM initialization so that we
>> * can startup non-root domains.
>> @@ -404,10 +405,6 @@ static void __noreturn init_warm_startup(struct sbi_scratch *scratch,
>> if (rc)
>> sbi_hart_hang();
>>
>> - rc = sbi_sse_init(scratch, false);
>> - if (rc)
>> - sbi_hart_hang();
>> -
>> rc = sbi_pmu_init(scratch, false);
>> if (rc)
>> sbi_hart_hang();
>> @@ -440,6 +437,10 @@ static void __noreturn init_warm_startup(struct sbi_scratch *scratch,
>> if (rc)
>> sbi_hart_hang();
>>
>> + rc = sbi_sse_init(scratch, false);
>> + if (rc)
>> + sbi_hart_hang();
>> +
>
> This re-ordering of sbi_sse_init() is not going to fly because
> in the future we might have SSE events being registered by
> drivers from sbi_platform_final_init().
>
>> /*
>> * Configure PMP at last because if SMEPMP is detected,
>> * M-mode access to the S/U space will be rescinded.
>> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
>> index 1eab0629..182d8db9 100644
>> --- a/lib/sbi/sbi_pmu.c
>> +++ b/lib/sbi/sbi_pmu.c
>> @@ -1165,9 +1165,9 @@ int sbi_pmu_init(struct sbi_scratch *scratch, bool cold_boot)
>> return SBI_EINVAL;
>>
>> total_ctrs = num_hw_ctrs + SBI_PMU_FW_CTR_MAX;
>> - }
>>
>> - sbi_sse_set_cb_ops(SBI_SSE_EVENT_LOCAL_PMU, &pmu_sse_cb_ops);
>> + sbi_sse_set_cb_ops(SBI_SSE_EVENT_LOCAL_PMU, &pmu_sse_cb_ops);
>> + }
>>
>> phs = pmu_get_hart_state_ptr(scratch);
>> if (!phs) {
>> diff --git a/lib/sbi/sbi_sse.c b/lib/sbi/sbi_sse.c
>> index f710338e..f2a9d791 100644
>> --- a/lib/sbi/sbi_sse.c
>> +++ b/lib/sbi/sbi_sse.c
>> @@ -39,21 +39,33 @@
>>
>> #define EVENT_IS_GLOBAL(__event_id) ((__event_id) & SBI_SSE_EVENT_GLOBAL_BIT)
>>
>> -static const uint32_t supported_events[] = {
>> - SBI_SSE_EVENT_LOCAL_RAS,
>> - SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP,
>> - SBI_SSE_EVENT_GLOBAL_RAS,
>> - SBI_SSE_EVENT_LOCAL_PMU,
>> - SBI_SSE_EVENT_LOCAL_SOFTWARE,
>> - SBI_SSE_EVENT_GLOBAL_SOFTWARE,
>> +struct sse_event_info {
>> + uint32_t event_id;
>> + const struct sbi_sse_cb_ops *cb_ops;
>> };
>>
>> -#define EVENT_COUNT array_size(supported_events)
>> +static bool event_supported_true(uint32_t event_id);
>> +
>> +static const struct sbi_sse_cb_ops supported_event_ops = {
>> + .is_supported_cb = &event_supported_true
>> +};
>> +
>> +static struct sse_event_info valid_events[] = {
>> + {.event_id = SBI_SSE_EVENT_LOCAL_RAS, .cb_ops = &supported_event_ops},
>> + {.event_id = SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, .cb_ops = &supported_event_ops},
>> + {.event_id = SBI_SSE_EVENT_GLOBAL_RAS, .cb_ops = &supported_event_ops},
>> + {.event_id = SBI_SSE_EVENT_LOCAL_PMU, .cb_ops = &supported_event_ops},
>> + {.event_id = SBI_SSE_EVENT_LOCAL_SOFTWARE, .cb_ops = &supported_event_ops},
>> + {.event_id = SBI_SSE_EVENT_GLOBAL_SOFTWARE, .cb_ops = &supported_event_ops},
>> +};
>> +
>
> This static table approach is not scalable and does not allow
> platform specific SSE events to be registered from drivers.
Agreed.
>
> Initially you can start with a global linked list of supported SSE
> events which any one can register in the cold boot path.
>
> The sbi_sse_init() should be simplified to not depend on
> supported SSE events to be pre-populated.
IMHO, all the events that will be handled by drivers should be available
by the time sbi_sse_init() is called or the complexity will be deferred
to some other functions as well. This call can even be moved after
sbi_platform_final_init(). Except if you have another reason not to do
so but that would be the best way to go.
To clarify, I'll modify the current patch to use a linked list of events
populated only with the GLOBAL_SOFTWARE_EVENT and LOCAL_SOFTWARE_EVENT
at compile time. This list can be extended at boot time by registering
cb_ops for events with sbi_sse_set_cb_ops() from drivers. sbi_sse_init()
will now allocate space for all events that are in the linked list of
events (no matter if they are supported or not). Event availability will
be then tested using the is_supported() callback and stored in the event
struct itself. Finally, sbi_sse_init() will be moved after all other
driver init (ie after sbi_platform_final_init()). If you envision
something else, please detail it.
Thanks,
Clément
>
>> +#define EVENT_COUNT array_size(valid_events)
>>
>> #define sse_event_invoke_cb(_event, _cb, ...) \
>> { \
>> - if (_event->cb_ops && _event->cb_ops->_cb) \
>> - _event->cb_ops->_cb(_event->event_id, ##__VA_ARGS__); \
>> + const struct sbi_sse_cb_ops *__ops = _event->info->cb_ops; \
>> + if (__ops && __ops->_cb) \
>> + __ops->_cb(_event->event_id, ##__VA_ARGS__); \
>> }
>>
>> struct sse_entry_state {
>> @@ -110,7 +122,7 @@ struct sbi_sse_event {
>> struct sbi_sse_event_attrs attrs;
>> uint32_t event_id;
>> u32 hartindex;
>> - const struct sbi_sse_cb_ops *cb_ops;
>> + struct sse_event_info *info;
>> struct sbi_dlist node;
>> };
>>
>> @@ -144,6 +156,11 @@ struct sse_hart_state {
>> */
>> struct sbi_sse_event *local_events;
>>
>> + /**
>> + * Number of events in the local_events array.
>> + */
>> + unsigned int local_event_count;
>> +
>> /**
>> * State to track if the hart is ready to take sse events.
>> * One hart cannot modify this state of another hart.
>> @@ -167,7 +184,6 @@ struct sse_global_event {
>> spinlock_t lock;
>> };
>>
>> -static unsigned int local_event_count;
>> static unsigned int global_event_count;
>> static struct sse_global_event *global_events;
>>
>> @@ -180,6 +196,11 @@ static u32 sse_ipi_inject_event = SBI_IPI_EVENT_MAX;
>>
>> static int sse_ipi_inject_send(unsigned long hartid, uint32_t event_id);
>>
>> +static bool event_supported_true(uint32_t event_id)
>> +{
>> + return true;
>> +}
>> +
>> static unsigned long sse_event_state(struct sbi_sse_event *e)
>> {
>> return e->attrs.status & SBI_SSE_ATTR_STATUS_STATE_MASK;
>> @@ -244,8 +265,42 @@ static void sse_event_set_state(struct sbi_sse_event *e,
>> e->attrs.status |= new_state;
>> }
>>
>> +static struct sse_event_info *sse_event_info_get(uint32_t event_id)
>> +{
>> + struct sse_event_info *info;
>> + int i;
>> +
>> + for (i = 0; i < EVENT_COUNT; i++) {
>> + info = &valid_events[i];
>> + if (info->event_id == event_id)
>> + return info;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static int sse_event_info_is_supported(struct sse_event_info *info)
>> +{
>> + if (!info)
>> + return SBI_EINVAL;
>> +
>> + if (!info->cb_ops || !info->cb_ops->is_supported_cb)
>> + return SBI_ENOTSUPP;
>> +
>> + return info->cb_ops->is_supported_cb(info->event_id) ? SBI_SUCCESS :
>> + SBI_ENOTSUPP;
>> +}
>> +
>> +static int sse_event_is_supported(uint32_t event_id)
>> +{
>> + struct sse_event_info *info = sse_event_info_get(event_id);
>> +
>> + return sse_event_info_is_supported(info);
>> +}
>> +
>> static int sse_event_get(uint32_t event_id, struct sbi_sse_event **eret)
>> {
>> + int ret;
>> unsigned int i;
>> struct sbi_sse_event *e;
>> struct sse_hart_state *shs;
>> @@ -264,7 +319,7 @@ static int sse_event_get(uint32_t event_id, struct sbi_sse_event **eret)
>> }
>> } else {
>> shs = sse_thishart_state_ptr();
>> - for (i = 0; i < local_event_count; i++) {
>> + for (i = 0; i < shs->local_event_count; i++) {
>> e = &shs->local_events[i];
>> if (e->event_id == event_id) {
>> *eret = e;
>> @@ -273,7 +328,16 @@ static int sse_event_get(uint32_t event_id, struct sbi_sse_event **eret)
>> }
>> }
>>
>> - return SBI_EINVAL;
>> + ret = sse_event_is_supported(event_id);
>> + /*
>> + * This should never happen since if an event is supported it should
>> + * have been handled by the previous chunk of code. But better be safe
>> + * than sorry and return EFAIL.
>> + */
>> + if (ret == SBI_SUCCESS)
>> + return SBI_EFAIL;
>> +
>> + return ret;
>> }
>>
>> static void sse_event_put(struct sbi_sse_event *e)
>> @@ -869,18 +933,16 @@ int sbi_sse_inject_event(uint32_t event_id)
>>
>> int sbi_sse_set_cb_ops(uint32_t event_id, const struct sbi_sse_cb_ops *cb_ops)
>> {
>> - int ret;
>> - struct sbi_sse_event *e;
>> + struct sse_event_info *info;
>>
>> - if (cb_ops->set_hartid_cb && !EVENT_IS_GLOBAL(event_id))
>> + info = sse_event_info_get(event_id);
>> + if (!info)
>> return SBI_EINVAL;
>>
>> - ret = sse_event_get(event_id, &e);
>> - if (ret)
>> - return ret;
>> + if (cb_ops->set_hartid_cb && !EVENT_IS_GLOBAL(event_id))
>> + return SBI_EINVAL;
>>
>> - e->cb_ops = cb_ops;
>> - sse_event_put(e);
>> + info->cb_ops = cb_ops;
>>
>> return SBI_OK;
>> }
>> @@ -1063,42 +1125,47 @@ int sbi_sse_unregister(uint32_t event_id)
>> return ret;
>> }
>>
>> -static void sse_event_init(struct sbi_sse_event *e, uint32_t event_id)
>> +static void sse_event_init(struct sbi_sse_event *e, struct sse_event_info *info)
>> {
>> - e->event_id = event_id;
>> + e->event_id = info->event_id;
>> + e->info = info;
>> e->hartindex = current_hartindex();
>> e->attrs.hartid = current_hartid();
>> /* Declare all events as injectable */
>> e->attrs.status |= BIT(SBI_SSE_ATTR_STATUS_INJECT_OFFSET);
>> }
>>
>> -static void sse_event_count_init()
>> +static int sse_global_init(void)
>> {
>> - unsigned int i;
>> + struct sse_event_info *info;
>> + struct sbi_sse_event *e;
>> + unsigned int i, ev = 0;
>>
>> for (i = 0; i < EVENT_COUNT; i++) {
>> - if (EVENT_IS_GLOBAL(supported_events[i]))
>> - global_event_count++;
>> - else
>> - local_event_count++;
>> + info = &valid_events[i];
>> +
>> + if (!EVENT_IS_GLOBAL(info->event_id) ||
>> + sse_event_is_supported(info->event_id) != SBI_SUCCESS)
>> + continue;
>> +
>> + global_event_count++;
>> }
>> -}
>>
>> -static int sse_global_init()
>> -{
>> - struct sbi_sse_event *e;
>> - unsigned int i, ev = 0;
>> + if (!global_event_count)
>> + return 0;
>>
>> global_events = sbi_zalloc(sizeof(*global_events) * global_event_count);
>> if (!global_events)
>> return SBI_ENOMEM;
>>
>> for (i = 0; i < EVENT_COUNT; i++) {
>> - if (!EVENT_IS_GLOBAL(supported_events[i]))
>> + info = &valid_events[i];
>> + if (!EVENT_IS_GLOBAL(info->event_id) ||
>> + sse_event_is_supported(info->event_id) != SBI_SUCCESS)
>> continue;
>>
>> e = &global_events[ev].event;
>> - sse_event_init(e, supported_events[i]);
>> + sse_event_init(e, info);
>> SPIN_LOCK_INIT(global_events[ev].lock);
>>
>> ev++;
>> @@ -1107,18 +1174,40 @@ static int sse_global_init()
>> return 0;
>> }
>>
>> +static unsigned int sse_get_local_event_count(void)
>> +{
>> + int i;
>> + struct sse_event_info *info;
>> + unsigned int local_event_count = 0;
>> +
>> + for (i = 0; i < EVENT_COUNT; i++) {
>> + info = &valid_events[i];
>> +
>> + if (EVENT_IS_GLOBAL(info->event_id) ||
>> + sse_event_is_supported(info->event_id) != SBI_SUCCESS)
>> + continue;
>> +
>> + local_event_count++;
>> + }
>> +
>> + return local_event_count;
>> +}
>> +
>> static void sse_local_init(struct sse_hart_state *shs)
>> {
>> + struct sse_event_info *info;
>> unsigned int i, ev = 0;
>>
>> SBI_INIT_LIST_HEAD(&shs->enabled_event_list);
>> SPIN_LOCK_INIT(shs->enabled_event_lock);
>>
>> for (i = 0; i < EVENT_COUNT; i++) {
>> - if (EVENT_IS_GLOBAL(supported_events[i]))
>> + info = &valid_events[i];
>> + if (EVENT_IS_GLOBAL(info->event_id) ||
>> + sse_event_is_supported(info->event_id) != SBI_SUCCESS)
>> continue;
>>
>> - sse_event_init(&shs->local_events[ev++], supported_events[i]);
>> + sse_event_init(&shs->local_events[ev++], info);
>> }
>> }
>>
>> @@ -1128,10 +1217,9 @@ int sbi_sse_init(struct sbi_scratch *scratch, bool cold_boot)
>> void *sse_inject_mem;
>> struct sse_hart_state *shs;
>> struct sbi_fifo *sse_inject_q;
>> + unsigned int local_event_count = 0;
>>
>> if (cold_boot) {
>> - sse_event_count_init();
>> -
>> ret = sse_global_init();
>> if (ret)
>> return ret;
>> @@ -1165,13 +1253,16 @@ int sbi_sse_init(struct sbi_scratch *scratch, bool cold_boot)
>>
>> shs = sse_get_hart_state_ptr(scratch);
>> if (!shs) {
>> + local_event_count = sse_get_local_event_count();
>> /* Allocate per hart state and local events at once */
>> shs = sbi_zalloc(sizeof(*shs) + sizeof(struct sbi_sse_event) *
>> local_event_count);
>> if (!shs)
>> return SBI_ENOMEM;
>>
>> - shs->local_events = (struct sbi_sse_event *)(shs + 1);
>> + shs->local_event_count = local_event_count;
>> + if (local_event_count)
>> + shs->local_events = (struct sbi_sse_event *)(shs + 1);
>>
>> /* SSE events are masked until hart unmasks them */
>> shs->masked = true;
>> @@ -1195,9 +1286,14 @@ void sbi_sse_exit(struct sbi_scratch *scratch)
>> {
>> int i;
>> struct sbi_sse_event *e;
>> + struct sse_event_info *info;
>>
>> for (i = 0; i < EVENT_COUNT; i++) {
>> - if (sse_event_get(supported_events[i], &e))
>> + info = &valid_events[i];
>> + if (sse_event_info_is_supported(info) != SBI_SUCCESS)
>> + continue;
>> +
>> + if (sse_event_get(info->event_id, &e))
>> continue;
>>
>> if (e->attrs.hartid != current_hartid())
>> --
>> 2.45.2
>>
>>
>> --
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
>
> This patch needs to be heavily re-worked.
>
> Regards,
> Anup
More information about the opensbi
mailing list