Re: [PATCH v3 2/4] riscv: add support for SBI Supervisor Software Events extension

[Andrew Jones <ajones@ventanamicro.com>]

On Thu, Mar 20, 2025 at 09:16:07AM +0100, Clément Léger wrote:
> 
> 
> On 19/03/2025 18:08, Andrew Jones wrote:
> > On Fri, Dec 06, 2024 at 05:30:58PM +0100, Clément Léger wrote:
> > ...
> >> +int arch_sse_init_event(struct sse_event_arch_data *arch_evt, u32 evt_id, int cpu)
> >> +{
> >> +	void *stack;
> >> +
> >> +	arch_evt->evt_id = evt_id;
> >> +	stack = sse_stack_alloc(cpu, SSE_STACK_SIZE);
> >> +	if (!stack)
> >> +		return -ENOMEM;
> >> +
> >> +	arch_evt->stack = stack + SSE_STACK_SIZE;
> >> +
> >> +	if (sse_init_scs(cpu, arch_evt))
> >> +		goto free_stack;
> >> +
> >> +	if (is_kernel_percpu_address((unsigned long)&arch_evt->interrupted)) {
> >> +		arch_evt->interrupted_state_phys =
> >> +				per_cpu_ptr_to_phys(&arch_evt->interrupted);
> >> +	} else {
> >> +		arch_evt->interrupted_state_phys =
> >> +				virt_to_phys(&arch_evt->interrupted);
> >> +	}
> >> +
> >> +	return 0;
> > 
> > Hi Clément,
> > 
> > Testing SSE support with tools/testing/selftests/kvm/riscv/sbi_pmu_test
> > led to an opensbi sbi_trap_error because the output_phys_lo address passed
> > to sbi_sse_read_attrs() wasn't a physical address. The reason is that
> > is_kernel_percpu_address() can only be used on static percpu addresses,
> > but local sse events get their percpu addresses with alloc_percpu(), so
> > is_kernel_percpu_address() was returning false even for local events. I
> > made the following changes to get things working.
> 
> Hi Andrew,
> 
> Did something changed recently ? Because I tested that when it was send
> (PMU + some kernel internal testsuite) and didn't saw that. Anyway, I'll
> respin it with your changes as well.

It depends on the kernel config. Configs that don't have many
alloc_percpu() calls prior to the one made by sse can work, because,
iiuc, alloc_percpu() will get its allocation from the percpu allocator's
first chunk until that chunck fills up. The first chunck is shared with
the static allocations.

Thanks,
drew

> 
> Thanks !
> 
> Clément
> 
> > 
> > Thanks,
> > drew
> > 
> > diff --git a/arch/riscv/kernel/sse.c b/arch/riscv/kernel/sse.c
> > index b48ae69dad8d..f46893946086 100644
> > --- a/arch/riscv/kernel/sse.c
> > +++ b/arch/riscv/kernel/sse.c
> > @@ -100,12 +100,12 @@ int arch_sse_init_event(struct sse_event_arch_data *arch_evt, u32 evt_id, int cp
> >         if (sse_init_scs(cpu, arch_evt))
> >                 goto free_stack;
> > 
> > -       if (is_kernel_percpu_address((unsigned long)&arch_evt->interrupted)) {
> > +       if (sse_event_is_global(evt_id)) {
> >                 arch_evt->interrupted_state_phys =
> > -                               per_cpu_ptr_to_phys(&arch_evt->interrupted);
> > +                               virt_to_phys(&arch_evt->interrupted);
> >         } else {
> >                 arch_evt->interrupted_state_phys =
> > -                               virt_to_phys(&arch_evt->interrupted);
> > +                               per_cpu_ptr_to_phys(&arch_evt->interrupted);
> >         }
> > 
> >         return 0;
> > diff --git a/drivers/firmware/riscv/riscv_sse.c b/drivers/firmware/riscv/riscv_sse.c
> > index 511db9ad7a9e..fef375046f75 100644
> > --- a/drivers/firmware/riscv/riscv_sse.c
> > +++ b/drivers/firmware/riscv/riscv_sse.c
> > @@ -62,11 +62,6 @@ void sse_handle_event(struct sse_event_arch_data *arch_event,
> >                         ret);
> >  }
> > 
> > -static bool sse_event_is_global(u32 evt)
> > -{
> > -       return !!(evt & SBI_SSE_EVENT_GLOBAL);
> > -}
> > -
> >  static
> >  struct sse_event *sse_event_get(u32 evt)
> >  {
> > diff --git a/include/linux/riscv_sse.h b/include/linux/riscv_sse.h
> > index 16700677f1e8..06b757b036b0 100644
> > --- a/include/linux/riscv_sse.h
> > +++ b/include/linux/riscv_sse.h
> > @@ -8,6 +8,7 @@
> > 
> >  #include <linux/types.h>
> >  #include <linux/linkage.h>
> > +#include <asm/sbi.h>
> > 
> >  struct sse_event;
> >  struct pt_regs;
> > @@ -16,6 +17,11 @@ struct ghes;
> > 
> >  typedef int (sse_event_handler)(u32 event_num, void *arg, struct pt_regs *regs);
> > 
> > +static inline bool sse_event_is_global(u32 evt)
> > +{
> > +       return !!(evt & SBI_SSE_EVENT_GLOBAL);
> > +}
> > +
> >  #ifdef CONFIG_RISCV_SSE
> > 
> >  struct sse_event *sse_event_register(u32 event_num, u32 priority,
>