On 06/03/2025 16:15, Andrew Jones wrote:
> On Thu, Mar 06, 2025 at 03:32:39PM +0100, Clément Léger wrote:
>>
>>
>> On 28/02/2025 18:51, Andrew Jones wrote:
> ...
>>>> + attr = SBI_SSE_ATTR_INTERRUPTED_FLAGS;
>>>> + ret = sbi_sse_read_attrs(event_id, attr, 1, &prev_value);
>>>> + sbiret_report_error(&ret, SBI_SUCCESS, "Save interrupted flags no error");
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(interrupted_flags); i++) {
>>>> + flags = interrupted_flags[i];
>>>> + ret = sbi_sse_write_attrs(event_id, attr, 1, &flags);
>>>> + sbiret_report_error(&ret, SBI_SUCCESS,
>>>> + "Set interrupted flags bit 0x%lx value no error", flags);
>>>> + ret = sbi_sse_read_attrs(event_id, attr, 1, &value);
>>>> + sbiret_report_error(&ret, SBI_SUCCESS, "Get interrupted flags after set no error");
>>>> + report(value == flags, "interrupted flags modified value ok: 0x%lx", value);
>>>
>>> Do we also need to test with more than one flag set at a time?
>>
>> That is already done a few lines above (see /* Restore full saved state */).
>
> OK
>
>>
>>>
>>>> + }
>>>> +
>>>> + /* Write invalid bit in flag register */
>>>> + flags = SBI_SSE_ATTR_INTERRUPTED_FLAGS_SSTATUS_SDT << 1;
>>>> + ret = sbi_sse_write_attrs(event_id, attr, 1, &flags);
>>>> + sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "Set invalid flags bit 0x%lx value error",
>>>> + flags);
>>>> +#if __riscv_xlen > 32
>>>> + flags = BIT(32);
>>>> + ret = sbi_sse_write_attrs(event_id, attr, 1, &flags);
>>>> + sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "Set invalid flags bit 0x%lx value error",
>>>
>>> This should have a different report string than the test above.
>>
>> The bit value format does differentiate the printf though.
>
> OK
>
> ...
>>>> + ret = sbi_sse_unregister(event_id);
>>>> + if (!sbiret_report_error(&ret, SBI_SUCCESS, "SSE unregister no error"))
>>>> + goto done;
>>>> +
>>>> + sse_check_state(event_id, SBI_SSE_STATE_UNUSED);
>>>> +
>>>> +done:
>>>
>>> Is it ok to leave this function with an event registered/enabled? If not,
>>> then some of the goto's above should goto other labels which disable and
>>> unregister.
>>
>> No it's not but it's massive pain to keep everything coherent when it
>> fails ;)
>>
>
> asserts/aborts are fine if we can't recover easily, but then we should
> move the SSE tests out of the main SBI test into its own test so we
> don't short-circuit all other tests that may follow it.
Oh yes I did not though of that. I could as well short circuit the sse
event test themselves. Currently test function returns void but that
could be handled more gracefully so that we don't break all other tests
as well.
>
> ...
>>>> + /* Be sure global events are targeting the current hart */
>>>> + sse_global_event_set_current_hart(event_id);
>>>> +
>>>> + sbi_sse_register(event_id, event_arg);
>>>> + value = arg->prio;
>>>> + sbi_sse_write_attrs(event_id, SBI_SSE_ATTR_PRIORITY, 1, &value);
>>>> + sbi_sse_enable(event_id);
>>>
>>> No return code checks for these SSE calls? If we're 99% sure they should
>>> succeed, then I'd still check them with asserts.
>>
>> I was a bit lazy here. Since the goal is *not* to check the event state
>> themselve but rather the ordering, I didn't bother checking them. As
>> said before, habndling error and event state properly in case of error
>> seemed like a churn to me *just* for testing. I'll try something better
>> as well though.
>>
>
> We always want at least asserts() in order to catch the train when it
> first goes off the rails, rather than after it smashed through a village
> or two.
Yeah sure ;) I'll try to do what I said before: short circuit the
remaining SSE test for the event itself rather than aborting or
splitting the tests.
Thanks,
Clément
>
> Thanks,
> drew
