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

[Clément Léger]

On 14/03/2025 15:11, Andrew Jones wrote:
> On Fri, Mar 14, 2025 at 12:10:29PM +0100, Clément Léger wrote:
> ...
>> +static void sse_test_inject_local(uint32_t event_id)
>> +{
>> +	int cpu;
>> +	uint64_t timeout;
>> +	struct sbiret ret;
>> +	struct sse_local_per_cpu *cpu_args, *cpu_arg;
>> +	struct sse_foreign_cpu_test_arg *handler_arg;
>> +
>> +	cpu_args = calloc(NR_CPUS, sizeof(struct sbi_sse_handler_arg));
>> +
>> +	report_prefix_push("local_dispatch");
>> +	for_each_online_cpu(cpu) {
>> +		cpu_arg = &cpu_args[cpu];
>> +		cpu_arg->handler_arg.event_id = event_id;
>> +		cpu_arg->args.stack = sse_alloc_stack();
>> +		cpu_arg->args.handler = sse_foreign_cpu_handler;
>> +		cpu_arg->args.handler_data = (void *)&cpu_arg->handler_arg;
>> +		cpu_arg->state = SBI_SSE_STATE_UNUSED;
>> +	}
>> +
>> +	on_cpus(sse_register_enable_local, cpu_args);
>> +	for_each_online_cpu(cpu) {
>> +		cpu_arg = &cpu_args[cpu];
>> +		ret = cpu_arg->ret;
>> +		if (ret.error) {
>> +			report_fail("CPU failed to register/enable event: %ld", ret.error);
>> +			goto cleanup;
>> +		}
>> +
>> +		handler_arg = &cpu_arg->handler_arg;
>> +		WRITE_ONCE(handler_arg->expected_cpu, cpu);
>> +		/* For handler_arg content to be visible for other CPUs */
>> +		smp_wmb();
>> +		ret = sbi_sse_inject(event_id, cpus[cpu].hartid);
>> +		if (ret.error) {
>> +			report_fail("CPU failed to inject event: %ld", ret.error);
>> +			goto cleanup;
>> +		}
>> +	}
>> +
>> +	for_each_online_cpu(cpu) {
>> +		handler_arg = &cpu_args[cpu].handler_arg;
>> +		smp_rmb();
>> +
>> +		timeout = sse_event_get_complete_timeout();
>> +		while (!READ_ONCE(handler_arg->done) || timer_get_cycles() < timeout) {
> 
> I pointed this out in the last review, the || should be a &&. We don't
> want to keep waiting until we reach the timeout if we get the done signal
> earlier.

Missed it. I'll resend a V10 if you don't have any other comments.

Thanks,

Clément

> 
>> +			/* For handler_arg update to be visible */
>> +			smp_rmb();
>> +			cpu_relax();
>> +		}
>> +		report(READ_ONCE(handler_arg->done), "Event handled");
>> +		WRITE_ONCE(handler_arg->done, false);
>> +	}
>> +
>> +cleanup:
>> +	on_cpus(sbi_sse_disable_unregister_local, cpu_args);
>> +	for_each_online_cpu(cpu) {
>> +		cpu_arg = &cpu_args[cpu];
>> +		ret = READ_ONCE(cpu_arg->ret);
>> +		if (ret.error)
>> +			report_fail("CPU failed to disable/unregister event: %ld", ret.error);
>> +	}
>> +
>> +	for_each_online_cpu(cpu) {
>> +		cpu_arg = &cpu_args[cpu];
>> +		sse_free_stack(cpu_arg->args.stack);
>> +	}
>> +
>> +	report_prefix_pop();
>> +}
>> +
>> +static void sse_test_inject_global_cpu(uint32_t event_id, unsigned int cpu,
>> +				       struct sse_foreign_cpu_test_arg *test_arg)
>> +{
>> +	unsigned long value;
>> +	struct sbiret ret;
>> +	uint64_t timeout;
>> +	enum sbi_sse_state state;
>> +
>> +	WRITE_ONCE(test_arg->expected_cpu, cpu);
>> +	/* For test_arg content to be visible for other CPUs */
>> +	smp_wmb();
>> +	value = cpu;
>> +	ret = sbi_sse_write_attrs(event_id, SBI_SSE_ATTR_PREFERRED_HART, 1, &value);
>> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "Set preferred hart"))
>> +		return;
>> +
>> +	ret = sbi_sse_enable(event_id);
>> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "Enable event"))
>> +		return;
>> +
>> +	ret = sbi_sse_inject(event_id, cpu);
>> +	if (!sbiret_report_error(&ret, SBI_SUCCESS, "Inject event"))
>> +		goto disable;
>> +
>> +	smp_rmb();
>> +	timeout = sse_event_get_complete_timeout();
>> +	while (!READ_ONCE(test_arg->done) || timer_get_cycles() < timeout) {
> 
> same comment
> 
>> +		/* For shared test_arg structure */
>> +		smp_rmb();
>> +		cpu_relax();
>> +	}
>> +
>> +	report(READ_ONCE(test_arg->done), "event handler called");
>> +	WRITE_ONCE(test_arg->done, false);
>> +
>> +	timeout = sse_event_get_complete_timeout();
>> +	/* Wait for event to be back in ENABLED state */
>> +	do {
>> +		ret = sse_event_get_state(event_id, &state);
>> +		if (ret.error)
>> +			goto disable;
>> +		cpu_relax();
>> +	} while (state != SBI_SSE_STATE_ENABLED || timer_get_cycles() < timeout);
> 
> same comment
> 
> Otherwise,
> 
> Reviewed-by: Andrew Jones <andrew.jones at linux.dev>
> 
> Thanks,
> drew