Re: [RFC PATCH 2/2] arm64: support HAVE_IRQ_EXIT_ON_IRQ_STACK

[Qi Zheng]

On 2022/7/7 22:41, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 3:38 PM Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx> wrote:
> > On 2022/7/7 20:49, Arnd Bergmann wrote:
> > > On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx> wrote:
> >            * Restore the SP from the FP, and restore the FP and LR from
> > the frame
> >            * record.
> >            */
> > -       mov     sp, x29
> > +999:   mov     sp, x29
> >           ldp     x29, x30, [sp], #16
> >    #ifdef CONFIG_SHADOW_CALL_STACK
> >           ldp     scs_sp, xzr, [sp], #16
> >
> > But this also requires a new parameter in do_interrupt_handler.
> >
> > I also considered implementing call_on_irq_stack() for nmi and irq
> > separately, but later think it's unnecessary.
>
> What I had in mind was something along the lines of
>
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 56cefd33eb8e..432042b91588 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -270,10 +270,7 @@ static void do_interrupt_handler(struct pt_regs *regs,
>   {
>          struct pt_regs *old_regs = set_irq_regs(regs);
>
> -       if (on_thread_stack())
> -               call_on_irq_stack(regs, handler);
> -       else
> -               handler(regs);
> +       handler(regs);
>
>          set_irq_regs(old_regs);
>   }
> @@ -473,16 +470,31 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
>                  __el1_irq(regs, handler);
>   }
>
> -asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
> +static void noinstr el1_irq(struct pt_regs *regs)
>   {
>          el1_interrupt(regs, handle_arch_irq);
>   }
>
> -asmlinkage void noinstr el1h_64_fiq_handler(struct pt_regs *regs)
> +asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
> +{
> +       if (on_thread_stack())
> +               call_on_irq_stack(regs, el1_irq);

IMO, this can't work. Because el1_interrupt() will invoke
arm64_preempt_schedule_irq(), which will cause scheduling on the
IRQ stack.

Thanks,
Qi

> +       else
> +               el1_irq(regs);
> +}
> +
> +static void noinstr el1_fiq(struct pt_regs *regs)
>   {
>          el1_interrupt(regs, handle_arch_fiq);
>   }
>
> +asmlinkage void noinstr el1h_64_fiq_handler(struct pt_regs *regs)
> +{
> +        if (on_thread_stack())
> +               call_on_irq_stack(regs, el1_fiq);
> +       else
> +               el1_fiq(regs);
> +}
> +
>   asmlinkage void noinstr el1h_64_error_handler(struct pt_regs *regs)
>   {
>          unsigned long esr = read_sysreg(esr_el1);
> @@ -713,7 +731,7 @@ static void noinstr
> __el0_irq_handler_common(struct pt_regs *regs)
>
>   asmlinkage void noinstr el0t_64_irq_handler(struct pt_regs *regs)
>   {
> -       __el0_irq_handler_common(regs);
> +       call_on_irq_stack(regs, __el0_irq_handler_common);
>   }
>
>   static void noinstr __el0_fiq_handler_common(struct pt_regs *regs)
> @@ -723,7 +741,7 @@ static void noinstr
> __el0_fiq_handler_common(struct pt_regs *regs)
>
>   asmlinkage void noinstr el0t_64_fiq_handler(struct pt_regs *regs)
>   {
> -       __el0_fiq_handler_common(regs);
> +       call_on_irq_stack(regs, __el0_fiq_handler_common);
>   }
>
>   static void noinstr __el0_error_handler_common(struct pt_regs *regs)
> @@ -807,12 +825,12 @@ asmlinkage void noinstr
> el0t_32_sync_handler(struct pt_regs *regs)
>
>   asmlinkage void noinstr el0t_32_irq_handler(struct pt_regs *regs)
>   {
> -       __el0_irq_handler_common(regs);
> +       call_on_irq_stack(regs, __el0_irq_handler_common);
>   }
>
>   asmlinkage void noinstr el0t_32_fiq_handler(struct pt_regs *regs)
>   {
> -       __el0_fiq_handler_common(regs);
> +       call_on_irq_stack(regs, __el0_fiq_handler_common);
>   }
>
>   asmlinkage void noinstr el0t_32_error_handler(struct pt_regs *regs)
>
> Not sure if that works.
>
>          Arnd

--
Thanks,
Qi