[PATCH v2 2/4] lib: sbi: sse: rework event availability

Clément Léger cleger at rivosinc.com
Thu Dec 12 00:23:47 PST 2024


On 10/12/2024 19:42, Samuel Holland wrote:
> Hi Clément,
> 
> On 2024-12-10 11:05 AM, Clément Léger wrote:
>> In order to detect events that are available and supported, add a per
>> event 'supported' property.
>>
>> To make full use of that property, add a new struct sse_event_info which
>> contains the 'cb_ops' as well as a 'supported' boolean. The cb_ops are
>> now global for a specific event_id and can be filled by drivers before
>> calling sse init which will use the callback to determine which events
>> are available. Thanks to that, we can also allocate only the supported
>> events and defer sse probing. All events are declared as supported 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 'supported' property is only done when we
>> fail from getting an event so that there is no performance cost.
> 
> Looks like a good design. It does prevent supporting a local event on some harts
> but not others, but that's probably not worth considering until there's an
> actual need for it.

That's a good comment and I can actually modify that pretty easily to
have per hart local_event_count. I'll do that for the next version.

> 
>> Signed-off-by: Clément Léger <cleger at rivosinc.com>
>> ---
>>  lib/sbi/sbi_init.c |  20 ++++-----
>>  lib/sbi/sbi_pmu.c  |   4 +-
>>  lib/sbi/sbi_sse.c  | 104 ++++++++++++++++++++++++++++++++-------------
>>  3 files changed, 86 insertions(+), 42 deletions(-)
>>
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index 0736345d..dd8b990f 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,12 @@ 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();
>> +	}
> 
> Should have a blank line here.
> 
>>  	/*
>>  	 * Note: Finalize domains after HSM initialization so that we
>>  	 * can startup non-root domains.
>> @@ -404,10 +404,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 +436,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();
>> +
>>  	/*
>>  	 * 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 0696ab5e..6f192440 100644
>> --- a/lib/sbi/sbi_pmu.c
>> +++ b/lib/sbi/sbi_pmu.c
>> @@ -1153,9 +1153,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 ed07844a..4c186a22 100644
>> --- a/lib/sbi/sbi_sse.c
>> +++ b/lib/sbi/sbi_sse.c
>> @@ -39,21 +39,28 @@
>>  
>>  #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;
>> +	bool supported;
>> +	const struct sbi_sse_cb_ops *cb_ops;
>> +};
>> +
>> +static struct sse_event_info valid_events[] = {
>> +	{.event_id = SBI_SSE_EVENT_LOCAL_RAS, .supported = true},
>> +	{.event_id = SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, .supported = true},
>> +	{.event_id = SBI_SSE_EVENT_GLOBAL_RAS, .supported = true},
>> +	{.event_id = SBI_SSE_EVENT_LOCAL_PMU, .supported = true},
>> +	{.event_id = SBI_SSE_EVENT_LOCAL_SOFTWARE, .supported = true},
>> +	{.event_id = SBI_SSE_EVENT_GLOBAL_SOFTWARE, .supported = true},
>>  };
>>  
>> -#define EVENT_COUNT array_size(supported_events)
>> +#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 +117,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;
>>  };
>>  
>> @@ -244,6 +251,30 @@ 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_is_supported(uint32_t event_id)
>> +{
>> +	struct sse_event_info *info = sse_event_info_get(event_id);
>> +
>> +	if (!info)
>> +		return SBI_EINVAL;
>> +
>> +	return info->supported ? SBI_SUCCESS : SBI_ENOTSUPP;
>> +}
>> +
>>  static int sse_event_get(uint32_t event_id, struct sbi_sse_event **eret)
>>  {
>>  	unsigned int i;
>> @@ -270,7 +301,7 @@ static int sse_event_get(uint32_t event_id, struct sbi_sse_event **eret)
>>  		}
>>  	}
>>  
>> -	return SBI_EINVAL;
>> +	return sse_event_is_supported(event_id);
> 
> I don't like that sse_event_is_supported() can return SBI_SUCCESS while this
> function has not initialized eret. I don't think this can actually happen in
> practice -- all actually supported events will have a struct sbi_sse_event
> allocated -- but I think it at least deserves a comment.

Sure, let's be conservative and return an error if eret isn't set in all
cases.

Thanks,

Clément

> 
> Regards,
> Samuel
> 
>>  }
>>  
>>  static void sse_event_put(struct sbi_sse_event *e)
>> @@ -866,18 +897,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;
>>  }
>> @@ -1060,21 +1089,27 @@ 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 void sse_event_info_init(void)
>>  {
>>  	unsigned int i;
>> +	struct sse_event_info *info;
>>  
>>  	for (i = 0; i < EVENT_COUNT; i++) {
>> -		if (EVENT_IS_GLOBAL(supported_events[i]))
>> +		info = &valid_events[i];
>> +		if (!info->supported)
>> +			continue;
>> +
>> +		if (EVENT_IS_GLOBAL(info->event_id))
>>  			global_event_count++;
>>  		else
>>  			local_event_count++;
>> @@ -1083,6 +1118,7 @@ static void sse_event_count_init()
>>  
>>  static int sse_global_init()
>>  {
>> +	struct sse_event_info *info;
>>  	struct sbi_sse_event *e;
>>  	unsigned int i, ev = 0;
>>  
>> @@ -1091,11 +1127,12 @@ static int sse_global_init()
>>  		return SBI_ENOMEM;
>>  
>>  	for (i = 0; i < EVENT_COUNT; i++) {
>> -		if (!EVENT_IS_GLOBAL(supported_events[i]))
>> +		info = &valid_events[i];
>> +		if (!info->supported || !EVENT_IS_GLOBAL(info->event_id))
>>  			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++;
>> @@ -1106,16 +1143,18 @@ static int sse_global_init()
>>  
>>  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 (!info->supported || EVENT_IS_GLOBAL(info->event_id))
>>  			continue;
>>  
>> -		sse_event_init(&shs->local_events[ev++], supported_events[i]);
>> +		sse_event_init(&shs->local_events[ev++], info);
>>  	}
>>  }
>>  
>> @@ -1127,7 +1166,7 @@ int sbi_sse_init(struct sbi_scratch *scratch, bool cold_boot)
>>  	struct sbi_fifo *sse_inject_q;
>>  
>>  	if (cold_boot) {
>> -		sse_event_count_init();
>> +		sse_event_info_init();
>>  
>>  		ret = sse_global_init();
>>  		if (ret)
>> @@ -1192,9 +1231,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 (!info->supported)
>> +			continue;
>> +
>> +		if (sse_event_get(info->event_id, &e))
>>  			continue;
>>  
>>  		if (e->attrs.hartid != current_hartid())
> 




More information about the opensbi mailing list