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

[Clément Léger]

On 10/12/2024 10:29, Clément Léger wrote:
> 
> 
> 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?

Woops, forgot to answer that as well.

I guess you are actually right, exposing the event without being
actually able to handle them is probably wrong. This means some driver
would be able to request them but fails to receive any event. With the
addition of SBI_ERR_NOT_SUPPORTED return value, we'll be able to handle
that properly so I think we will go that way of refuse register for
events that do not have a register_cb() callback.

Thanks,

Clément


>>
>>> +			__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;
>>>  }
>>>  
>>
>