[PATCH 1/3] lib: sbi: sse: Return a value from the register callback

[Clément Léger]

On 09/12/2024 21:04, Samuel Holland wrote:
> Hi Clément,
> 
> On 2024-12-06 2:59 PM, Clément Léger wrote:
>> Some events might not be available to registration due to missing
>> dependencies (ISA extensions for instance). Return an 'int' from the
>> register_cb() callback so that this can be handled.
>>
>> Signed-off-by: Clément Léger <cleger at rivosinc.com>
>> ---
>>  include/sbi/sbi_sse.h |  2 +-
>>  lib/sbi/sbi_sse.c     | 16 ++++++++++++++--
>>  2 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/sbi/sbi_sse.h b/include/sbi/sbi_sse.h
>> index 7419698a..58bd2ae0 100644
>> --- a/include/sbi/sbi_sse.h
>> +++ b/include/sbi/sbi_sse.h
>> @@ -36,7 +36,7 @@ struct sbi_sse_cb_ops {
>>  	/**
>>  	 * Called when the SBI_EXT_SSE_REGISTER is invoked on the event.
>>  	 */
>> -	void (*register_cb)(uint32_t event_id);
>> +	int (*register_cb)(uint32_t event_id);
>>  
>>  	/**
>>  	 * Called when the SBI_EXT_SSE_UNREGISTER is invoked on the event.
>> diff --git a/lib/sbi/sbi_sse.c b/lib/sbi/sbi_sse.c
>> index bf5620e8..5679c26f 100644
>> --- a/lib/sbi/sbi_sse.c
>> +++ b/lib/sbi/sbi_sse.c
>> @@ -50,6 +50,15 @@ static const uint32_t supported_events[] = {
>>  
>>  #define EVENT_COUNT array_size(supported_events)
>>  
>> +#define sse_event_invoke_cb_ret(_event, _cb, ...)                             \
>> +	({                                                                    \
>> +		int __ret = 0;                                                \
>> +		if (_event->cb_ops && _event->cb_ops->_cb)                    \
> 
> Shouldn't registration fail (except for the explicit "software injected" events)
> if no callback ops are registered (i.e. _event->cb_ops is NULL)? Right now
> OpenSBI has no support to generate RAS events, so those have no callback ops,
> but S-mode can still register and inject them.

If registration fails, this would make the testing the "blank" SSE
implementation more difficult to test (particularly priority handling)
since only the global and local software injectable events would be
available. But that is still possible. I mean, the spec does not say
anything about that so we might assume whatever we want.

> 
> Should we require callback ops to expose a SSE event? Should we remove an event
> ID from `supported_events` if the corresponding driver is not compiled in?
> 
>> +			__ret = _event->cb_ops->_cb(_event->event_id,         \
>> +						   ##__VA_ARGS__);            \
>> +		__ret;							      \
>> +	})
>> +
>>  #define sse_event_invoke_cb(_event, _cb, ...)                                 \
>>  	{                                                                     \
>>  		if (_event->cb_ops && _event->cb_ops->_cb)                    \
>> @@ -423,16 +432,19 @@ static int sse_event_register(struct sbi_sse_event *e,
>>  			      unsigned long handler_entry_pc,
>>  			      unsigned long handler_entry_arg)
>>  {
>> +	int ret;
> 
> minor: blank line needed here.

Yes sure, I'll add it.

Thanks,

Clément

> 
> Since the issue described above is an existing issue:
> 
> Reviewed-by: Samuel Holland <samuel.holland at sifive.com>
> 
> Regards,
> Samuel
> 
>>  	if (sse_event_state(e) != SBI_SSE_STATE_UNUSED)
>>  		return SBI_EINVALID_STATE;
>>  
>> +	ret = sse_event_invoke_cb_ret(e, register_cb);
>> +	if (ret)
>> +		return ret;
>> +
>>  	e->attrs.entry.pc = handler_entry_pc;
>>  	e->attrs.entry.arg = handler_entry_arg;
>>  
>>  	sse_event_set_state(e, SBI_SSE_STATE_REGISTERED);
>>  
>> -	sse_event_invoke_cb(e, register_cb);
>> -
>>  	return 0;
>>  }
>>  
>