Re: [kvm-unit-tests PATCH v7 6/6] riscv: sbi: Add SSE extension tests

[Andrew Jones <andrew.jones@linux.dev>]

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