On 14/01/2025 12:59, Andrew Jones wrote:
>
> 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 ...
> }
Hey Andrew,
That not possible since these are global event so they can only be
injected on one CPU at a time. I did this modification for local event
as you suggested though.
>
> 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.
Indeed, I'll fix the error sequence
>
>> +
>> + 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.
I shouldn't have looked at OpenSBI code to implement that test /o\.
Thanks for catching it.
>
>> +
>> + 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()
Indeed.
>
>> +
>> + /*
>> + * 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.
Sure.
>
>> + 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
Thanks for the review.
Clément
