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

[Qi Zheng <zhengqi.arch@bytedance.com>]

On 2022/7/7 22:41, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 3:38 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>> On 2022/7/7 20:49, Arnd Bergmann wrote:
>>> On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <zhengqi.arch@bytedance.com> 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