Continuing review...
On Mon, Nov 25, 2024 at 05:21:54PM +0100, Clément Léger wrote:
...
> +static void sse_test_inject_global(unsigned long event_id)
> +{
> + unsigned long ret;
> + unsigned int cpu;
> + struct sse_handler_arg args;
> + volatile struct sse_foreign_cpu_test_arg test_arg = {.event_id = event_id};
> + enum sbi_sse_state state;
> +
> + args.handler = sse_foreign_cpu_handler;
> + args.handler_data = (void *)&test_arg;
> + args.stack = sse_alloc_stack();
> +
> + report_prefix_push("global_dispatch");
> +
> + ret = sse_event_register(event_id, &args);
> + if (ret)
print/report something here?
> + goto done;
> +
> + for_each_online_cpu(cpu) {
> + test_arg.expected_cpu = cpu;
> + /* For test_arg content to be visible for other CPUs */
> + smp_wmb();
> + ret = sse_event_set_attr(event_id, SBI_SSE_ATTR_PREFERRED_HART, cpu);
> + if (ret) {
> + report_fail("Failed to set preferred hart");
> + goto done;
> + }
> +
> + ret = sse_event_enable(event_id);
> + if (ret) {
> + report_fail("Failed to enable SSE event");
> + goto done;
> + }
> +
> + ret = sse_event_inject(event_id, cpu);
> + if (ret) {
> + report_fail("Failed to inject event");
> + goto done;
> + }
> +
> + while (!test_arg.done) {
> + /* For shared test_arg structure */
> + smp_rmb();
> + }
> +
> + test_arg.done = false;
> +
> + /* Wait for event to be in ENABLED state */
> + do {
> + ret = sse_get_state(event_id, &state);
> + if (ret) {
> + report_fail("Failed to get event state");
> + goto done;
> + }
> + } while (state != SBI_SSE_STATE_ENABLED);
> +
> + ret = sse_event_disable(event_id);
> + if (ret) {
> + report_fail("Failed to disable SSE event");
> + goto done;
> + }
> +
> + report_pass("Global event on CPU %d", cpu);
> + }
Breaking this into two parts
for_each_online_cpu() {
... inject event ...
}
... wait a bit ...
for_each_online_cpu() {
... check results ...
}
would allow testing that the SBI implementation can handle
simultaneous requests.
> +
> +done:
> + ret = sse_event_unregister(event_id);
> + if (ret)
> + report_fail("Failed to unregister event");
If we came here from the 'goto done' for sse_event_register(), then I'd
expect unregister to fail since we failed to register.
> +
> + sse_free_stack(args.stack);
> + report_prefix_pop();
> +}
> +
> +struct priority_test_arg {
> + unsigned long evt;
> + bool called;
> + u32 prio;
> + struct priority_test_arg *next_evt_arg;
> + void (*check_func)(struct priority_test_arg *arg);
> +};
> +
> +static void sse_hi_priority_test_handler(void *arg, struct pt_regs *regs,
> + unsigned int hartid)
> +{
> + struct priority_test_arg *targ = arg;
> + struct priority_test_arg *next = targ->next_evt_arg;
> +
> + targ->called = 1;
nit: = true
> + if (next) {
> + sse_event_inject(next->evt, current_thread_info()->hartid);
> + if (sse_event_pending(next->evt))
> + report_fail("Higher priority event is pending");
> + if (!next->called)
> + report_fail("Higher priority event was not handled");
Should change these if-report-fails to report()'s
> + }
> +}
> +
> +static void sse_low_priority_test_handler(void *arg, struct pt_regs *regs,
> + unsigned int hartid)
> +{
> + struct priority_test_arg *targ = arg;
> + struct priority_test_arg *next = targ->next_evt_arg;
> +
> + targ->called = 1;
nit: = true
> +
> + if (next) {
> + sse_event_inject(next->evt, current_thread_info()->hartid);
> +
> + if (!sse_event_pending(next->evt))
> + report_fail("Lower priority event is pending");
> +
> + if (next->called)
> + report_fail("Lower priority event %s was handle before %s",
> + sse_evt_name(next->evt), sse_evt_name(targ->evt));
Should change these if-report-fails to report()'s
> + }
> +}
> +
> +static void sse_test_injection_priority_arg(struct priority_test_arg *in_args,
> + unsigned int in_args_size,
> + sse_handler_fn handler,
> + const char *test_name)
> +{
> + unsigned int i;
> + int ret;
> + unsigned long event_id;
> + struct priority_test_arg *arg;
> + unsigned int args_size = 0;
> + struct sse_handler_arg event_args[in_args_size];
> + struct priority_test_arg *args[in_args_size];
> + void *stack;
> + struct sse_handler_arg *event_arg;
> +
> + report_prefix_push(test_name);
> +
> + for (i = 0; i < in_args_size; i++) {
> + arg = &in_args[i];
> + event_id = arg->evt;
> + if (!sse_evt_can_inject(event_id))
> + continue;
> +
> + args[args_size] = arg;
> + args_size++;
> + }
> +
> + if (!args_size) {
> + report_skip("No event injectable");
> + report_prefix_pop();
> + goto skip;
> + }
> +
> + for (i = 0; i < args_size; i++) {
> + arg = args[i];
> + event_id = arg->evt;
> + stack = sse_alloc_stack();
> +
> + event_arg = &event_args[i];
> + event_arg->handler = handler;
> + event_arg->handler_data = (void *)arg;
> + event_arg->stack = stack;
> +
> + if (i < (args_size - 1))
> + arg->next_evt_arg = args[i + 1];
> + else
> + arg->next_evt_arg = NULL;
> +
> + /* Be sure global events are targeting the current hart */
> + sse_global_event_set_current_hart(event_id);
> +
> + sse_event_register(event_id, event_arg);
> + sse_event_set_attr(event_id, SBI_SSE_ATTR_PRIORITY, arg->prio);
> + sse_event_enable(event_id);
> + }
> +
> + /* Inject first event */
> + ret = sse_event_inject(args[0]->evt, current_thread_info()->hartid);
> + report(ret == SBI_SUCCESS, "SSE injection no error");
> +
> + for (i = 0; i < args_size; i++) {
> + arg = args[i];
> + event_id = arg->evt;
> +
> + if (!arg->called)
> + report_fail("Event %s handler called", sse_evt_name(arg->evt));
report()
> +
> + sse_event_disable(event_id);
> + sse_event_unregister(event_id);
> +
> + event_arg = &event_args[i];
> + sse_free_stack(event_arg->stack);
> + }
> +
> +skip:
> + report_prefix_pop();
> +}
> +
> +static struct priority_test_arg hi_prio_args[] = {
> + {.evt = SBI_SSE_EVENT_GLOBAL_SOFTWARE},
> + {.evt = SBI_SSE_EVENT_LOCAL_SOFTWARE},
> + {.evt = SBI_SSE_EVENT_LOCAL_PMU},
> + {.evt = SBI_SSE_EVENT_GLOBAL_RAS},
> + {.evt = SBI_SSE_EVENT_LOCAL_RAS},
> +};
> +
> +static struct priority_test_arg low_prio_args[] = {
> + {.evt = SBI_SSE_EVENT_LOCAL_RAS},
> + {.evt = SBI_SSE_EVENT_GLOBAL_RAS},
> + {.evt = SBI_SSE_EVENT_LOCAL_PMU},
> + {.evt = SBI_SSE_EVENT_LOCAL_SOFTWARE},
> + {.evt = SBI_SSE_EVENT_GLOBAL_SOFTWARE},
> +};
> +
> +static struct priority_test_arg prio_args[] = {
> + {.evt = SBI_SSE_EVENT_GLOBAL_SOFTWARE, .prio = 5},
> + {.evt = SBI_SSE_EVENT_LOCAL_SOFTWARE, .prio = 10},
> + {.evt = SBI_SSE_EVENT_LOCAL_PMU, .prio = 15},
> + {.evt = SBI_SSE_EVENT_GLOBAL_RAS, .prio = 20},
> + {.evt = SBI_SSE_EVENT_LOCAL_RAS, .prio = 25},
> +};
> +
> +static struct priority_test_arg same_prio_args[] = {
> + {.evt = SBI_SSE_EVENT_LOCAL_PMU, .prio = 0},
> + {.evt = SBI_SSE_EVENT_LOCAL_RAS, .prio = 10},
> + {.evt = SBI_SSE_EVENT_LOCAL_SOFTWARE, .prio = 10},
> + {.evt = SBI_SSE_EVENT_GLOBAL_SOFTWARE, .prio = 10},
> + {.evt = SBI_SSE_EVENT_GLOBAL_RAS, .prio = 20},
> +};
> +
> +static void sse_test_injection_priority(void)
> +{
> + report_prefix_push("prio");
> +
> + sse_test_injection_priority_arg(hi_prio_args, ARRAY_SIZE(hi_prio_args),
> + sse_hi_priority_test_handler, "high");
> +
> + sse_test_injection_priority_arg(low_prio_args, ARRAY_SIZE(low_prio_args),
> + sse_low_priority_test_handler, "low");
> +
> + sse_test_injection_priority_arg(prio_args, ARRAY_SIZE(prio_args),
> + sse_low_priority_test_handler, "changed");
> +
> + sse_test_injection_priority_arg(same_prio_args, ARRAY_SIZE(same_prio_args),
> + sse_low_priority_test_handler, "same_prio_args");
> +
> + report_prefix_pop();
> +}
> +
> +static bool sse_can_inject(unsigned long event_id)
> +{
> + int ret;
> + unsigned long status;
> +
> + ret = sse_event_get_attr(event_id, SBI_SSE_ATTR_STATUS, &status);
> + report(ret == 0, "SSE get attr status no error");
I'm not sure we need this report()
> + if (ret)
> + return 0;
nit: return false
> +
> + return !!(status & BIT(SBI_SSE_ATTR_STATUS_INJECT_OFFSET));
> +}
> +
> +static void boot_secondary(void *data)
> +{
> + sse_hart_unmask();
> +}
> +
> +static void sse_check_mask(void)
> +{
> + int ret;
> +
> + /* Upon boot, event are masked, check that */
> + ret = sse_hart_mask();
> + report(ret == SBI_ERR_ALREADY_STARTED, "SSE hart mask at boot time ok");
The spec says that trying to mask twice should return
SBI_ERR_ALREADY_STOPPED. If this test is passing then we probably have it
backwards in opensbi too.
> +
> + ret = sse_hart_unmask();
> + report(ret == SBI_SUCCESS, "SSE hart no error ok");
> + ret = sse_hart_unmask();
> + report(ret == SBI_ERR_ALREADY_STOPPED, "SSE hart unmask twice error ok");
Should be SBI_ERR_ALREADY_STARTED
> +
> + ret = sse_hart_mask();
> + report(ret == SBI_SUCCESS, "SSE hart mask no error");
> + ret = sse_hart_mask();
> + report(ret == SBI_ERR_ALREADY_STARTED, "SSE hart mask twice ok");
SBI_ERR_ALREADY_STOPPED
> +}
> +
> +void check_sse(void)
> +{
> + unsigned long i, event;
> +
> + report_prefix_push("sse");
> + sse_check_mask();
I guess the mask check should come after the sbi_probe()
> +
> + /*
> + * Dummy wakeup of all processors since some of them will be targeted
> + * by global events without going through the wakeup call as well as
> + * unmasking all
> + */
> + on_cpus(boot_secondary, NULL);
> +
> + if (!sbi_probe(SBI_EXT_SSE)) {
> + report_skip("SSE extension not available");
> + report_prefix_pop();
> + return;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(sse_event_infos); i++) {
> + event = sse_event_infos[i].event_id;
> + report_prefix_push(sse_event_infos[i].name);
> + if (!sse_can_inject(event)) {
> + report_skip("Event does not support injection");
Let's output the event ID too.
> + report_prefix_pop();
> + continue;
> + } else {
> + sse_event_infos[i].can_inject = true;
> + }
> + sse_test_attr(event);
> + sse_test_register_error(event);
> + sse_test_inject_simple(event);
> + if (sse_event_is_global(event))
> + sse_test_inject_global(event);
> + else
> + sse_test_inject_local(event);
> +
> + report_prefix_pop();
> + }
> +
> + sse_test_injection_priority();
> +
> + report_prefix_pop();
> +}
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 6f4ddaf1..33d5e40d 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -32,6 +32,8 @@
>
> #define HIGH_ADDR_BOUNDARY ((phys_addr_t)1 << 32)
>
> +void check_sse(void);
> +
> static long __labs(long a)
> {
> return __builtin_labs(a);
> @@ -1451,6 +1453,7 @@ int main(int argc, char **argv)
> check_hsm();
> check_dbcn();
> check_susp();
> + check_sse();
>
> return report_summary();
> }
> --
> 2.45.2
>
Thanks,
drew
