Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230324071239.151677-1-guoren@kernel.org> <20230324071239.151677-2-guoren@kernel.org>
 <f170c68c-4975-4f71-ac50-979483cb5848@spud>
In-Reply-To: <f170c68c-4975-4f71-ac50-979483cb5848@spud>
From:   Guo Ren <guoren@kernel.org>
Date:   Mon, 27 Mar 2023 21:32:51 +0800
Message-ID: <CAJF2gTSwt1XkC=kisOAf0_aHmi6E6ty-EV0eSA110E1DzvWc2Q@mail.gmail.com>
Subject: Re: [PATCH -next V11 1/3] riscv: stack: Support HAVE_IRQ_EXIT_ON_IRQ_STACK
To:     Conor Dooley <conor.dooley@microchip.com>
Cc:     arnd@arndb.de, palmer@rivosinc.com, tglx@linutronix.de,
        peterz@infradead.org, luto@kernel.org, heiko@sntech.de,
        jszhang@kernel.org, lazyparser@gmail.com, falcon@tinylab.org,
        chenhuacai@kernel.org, apatel@ventanamicro.com,
        atishp@atishpatra.org, mark.rutland@arm.com, bjorn@kernel.org,
        linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-riscv@lists.infradead.org, Guo Ren <guoren@linux.alibaba.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Mar 27, 2023 at 7:30=E2=80=AFPM Conor Dooley <conor.dooley@microchi=
p.com> wrote:
>
> On Fri, Mar 24, 2023 at 03:12:37AM -0400, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Add independent irq stacks for percpu to prevent kernel stack overflows=
.
> > It is also compatible with VMAP_STACK by implementing
> > arch_alloc_vmap_stack.  Many architectures have supported
> > HAVE_IRQ_EXIT_ON_IRQ_STACK, riscv should follow up.
> >
> > Tested-by: Jisheng Zhang <jszhang@kernel.org>
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
>
> > --- a/arch/riscv/kernel/irq.c
> > +++ b/arch/riscv/kernel/irq.c
> > @@ -9,6 +9,37 @@
> >  #include <linux/irqchip.h>
> >  #include <linux/seq_file.h>
> >  #include <asm/smp.h>
> > +#include <asm/vmap_stack.h>
> > +
> > +#ifdef CONFIG_IRQ_STACKS
> > +DEFINE_PER_CPU(ulong *, irq_stack_ptr);
>
> btw, sparse is complaining about this variable:
> ../arch/riscv/kernel/irq.c:15:1: warning: symbol '__pcpu_scope_irq_stack_=
ptr' was not declared. Should it be static?
I declared it in traps.c, maybe I should put it in the vmap_stack.h.

>
> I'm not immediately sure why that is the case, but should be
> reproducible with gcc-12 allmodconfig.
>
> Thanks,
> Conor.
>
> > +
> > +#ifdef CONFIG_VMAP_STACK
> > +static void init_irq_stacks(void)
> > +{
> > +     int cpu;
> > +     ulong *p;
> > +
> > +     for_each_possible_cpu(cpu) {
> > +             p =3D arch_alloc_vmap_stack(IRQ_STACK_SIZE, cpu_to_node(c=
pu));
> > +             per_cpu(irq_stack_ptr, cpu) =3D p;
> > +     }
> > +}
> > +#else
> > +/* irq stack only needs to be 16 byte aligned - not IRQ_STACK_SIZE ali=
gned. */
> > +DEFINE_PER_CPU_ALIGNED(ulong [IRQ_STACK_SIZE/sizeof(ulong)], irq_stack=
);
> > +
> > +static void init_irq_stacks(void)
> > +{
> > +     int cpu;
> > +
> > +     for_each_possible_cpu(cpu)
> > +             per_cpu(irq_stack_ptr, cpu) =3D per_cpu(irq_stack, cpu);
> > +}
> > +#endif /* CONFIG_VMAP_STACK */
> > +#else
> > +static void init_irq_stacks(void) {}
> > +#endif /* CONFIG_IRQ_STACKS */
> >
> >  int arch_show_interrupts(struct seq_file *p, int prec)
> >  {
> > @@ -18,6 +49,7 @@ int arch_show_interrupts(struct seq_file *p, int prec=
)
> >
> >  void __init init_IRQ(void)
> >  {
> > +     init_irq_stacks();
> >       irqchip_init();
> >       if (!handle_arch_irq)
> >               panic("No interrupt controller found.");
> > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> > index 1f4e37be7eb3..b69933ab6bf8 100644
> > --- a/arch/riscv/kernel/traps.c
> > +++ b/arch/riscv/kernel/traps.c
> > @@ -305,16 +305,50 @@ asmlinkage __visible noinstr void do_page_fault(s=
truct pt_regs *regs)
> >  }
> >  #endif
> >
> > -asmlinkage __visible noinstr void do_irq(struct pt_regs *regs)
> > +static void noinstr handle_riscv_irq(struct pt_regs *regs)
> >  {
> >       struct pt_regs *old_regs;
> > -     irqentry_state_t state =3D irqentry_enter(regs);
> >
> >       irq_enter_rcu();
> >       old_regs =3D set_irq_regs(regs);
> >       handle_arch_irq(regs);
> >       set_irq_regs(old_regs);
> >       irq_exit_rcu();
> > +}
> > +
> > +#ifdef CONFIG_IRQ_STACKS
> > +DECLARE_PER_CPU(ulong *, irq_stack_ptr);
> > +#endif
I declared it here.

> > +
> > +asmlinkage void noinstr do_irq(struct pt_regs *regs)
> > +{
> > +     irqentry_state_t state =3D irqentry_enter(regs);
> > +#ifdef CONFIG_IRQ_STACKS
> > +     if (on_thread_stack()) {
> > +             ulong *sp =3D per_cpu(irq_stack_ptr, smp_processor_id())
> > +                                     + IRQ_STACK_SIZE/sizeof(ulong);
> > +             __asm__ __volatile(
> > +             "addi   sp, sp, -"RISCV_SZPTR  "\n"
> > +             REG_S"  ra, (sp)                \n"
> > +             "addi   sp, sp, -"RISCV_SZPTR  "\n"
> > +             REG_S"  s0, (sp)                \n"
> > +             "addi   s0, sp, 2*"RISCV_SZPTR "\n"
> > +             "move   sp, %[sp]               \n"
> > +             "move   a0, %[regs]             \n"
> > +             "call   handle_riscv_irq        \n"
> > +             "addi   sp, s0, -2*"RISCV_SZPTR"\n"
> > +             REG_L"  s0, (sp)                \n"
> > +             "addi   sp, sp, "RISCV_SZPTR   "\n"
> > +             REG_L"  ra, (sp)                \n"
> > +             "addi   sp, sp, "RISCV_SZPTR   "\n"
> > +             :
> > +             : [sp] "r" (sp), [regs] "r" (regs)
> > +             : "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7",
> > +               "t0", "t1", "t2", "t3", "t4", "t5", "t6",
> > +               "memory");
> > +     } else
> > +#endif
> > +             handle_riscv_irq(regs);
> >
> >       irqentry_exit(regs, state);
> >  }
> > --
> > 2.36.1
> >
> >


--=20
Best Regards
 Guo Ren