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;
Message-ID: <33a63e76-fb71-2b9e-3b3c-cc6f7a675cf3@bytedance.com>
Date:   Thu, 7 Jul 2022 23:00:37 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.0
Subject: Re: [RFC PATCH 2/2] arm64: support HAVE_IRQ_EXIT_ON_IRQ_STACK
Content-Language: en-US
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20220707110511.52129-1-zhengqi.arch@bytedance.com>
 <20220707110511.52129-3-zhengqi.arch@bytedance.com>
 <CAK8P3a1-rL391dp33D5r+SgQ+L0u7Neta_-DZoZL=+c1zPtNQg@mail.gmail.com>
 <42bfd82c-b3fb-c942-d6f2-ae5adec90a26@bytedance.com>
 <CAK8P3a17A1t=qkyToQNVnuVfGPp-7VpFx5qJ-gmpyrkJ3yRXTw@mail.gmail.com>
From:   Qi Zheng <zhengqi.arch@bytedance.com>
In-Reply-To: <CAK8P3a17A1t=qkyToQNVnuVfGPp-7VpFx5qJ-gmpyrkJ3yRXTw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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