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.
...
> >> + /* 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.
Thanks,
drew
