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: <4415cf17-5078-1da8-c209-41936bf4dc0b@bytedance.com>
Date:   Tue, 19 Jul 2022 15:01:58 +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: [PATCH v1 2/2] arm64: support HAVE_IRQ_EXIT_ON_IRQ_STACK
From:   Qi Zheng <zhengqi.arch@bytedance.com>
To:     Will Deacon <will@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Mark Rutland <mark.rutland@arm.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20220708094950.41944-1-zhengqi.arch@bytedance.com>
 <20220708094950.41944-3-zhengqi.arch@bytedance.com>
 <CAK8P3a3rp6R1kWNHBxgDf5xrECj-F5AVn++qR9_Y7GiRrDBpDw@mail.gmail.com>
 <3afacff7-e559-b7b5-c65b-414ff988e696@bytedance.com>
Content-Language: en-US
In-Reply-To: <3afacff7-e559-b7b5-c65b-414ff988e696@bytedance.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2022/7/14 19:54, Qi Zheng wrote:
> 
> 
> On 2022/7/14 19:37, Arnd Bergmann wrote:
>> On Fri, Jul 8, 2022 at 11:49 AM Qi Zheng <zhengqi.arch@bytedance.com> 
>> wrote:
>>>
>>> Since softirqs are handled on the per-CPU IRQ stack,
>>> let's support HAVE_IRQ_EXIT_ON_IRQ_STACK which causes
>>> the core code to invoke __do_softirq() directly without
>>> going through do_softirq_own_stack().
>>>
>>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
>>
>> Adding Mark Rutland to Cc, he's the one that worked on this area the 
>> most in the
>> past and should probably review your patch. I still feel like there
>> should be a way
>> to improve readability of the IRQ entry path rather than just adding 
>> another
> 
> Got it. And looking forward to reviews and suggestions from Mark or
> anyone else.

Hi Will, Catalin and Mark,

I don't see any action items left for me. Any suggestions on this patch?

Thanks,
Qi

> 
>> level of indirection, but the ideas I had so far have not led to
>> anything useful.
>>
>> Overall I suppose your version is an improvement over the extra double 
>> stack
>> switch when entering softirq.
>>
>>          Arnd
> 
> Thanks,
> Qi
> 
>>
>>> ---
>>>   arch/arm64/Kconfig                 |  1 +
>>>   arch/arm64/include/asm/exception.h |  4 +++-
>>>   arch/arm64/kernel/entry-common.c   | 30 ++++++++++++++++++++----------
>>>   arch/arm64/kernel/entry.S          |  6 ++++--
>>>   arch/arm64/kernel/irq.c            |  5 +++--
>>>   5 files changed, 31 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index be0a9f0052ee..d2cc7daecce3 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -231,6 +231,7 @@ config ARM64
>>>          select TRACE_IRQFLAGS_SUPPORT
>>>          select TRACE_IRQFLAGS_NMI_SUPPORT
>>>          select HAVE_SOFTIRQ_ON_OWN_STACK
>>> +       select HAVE_IRQ_EXIT_ON_IRQ_STACK
>>>          help
>>>            ARM 64-bit (AArch64) Linux support.
>>>
>>> diff --git a/arch/arm64/include/asm/exception.h 
>>> b/arch/arm64/include/asm/exception.h
>>> index d94aecff9690..8bff0aa7ab50 100644
>>> --- a/arch/arm64/include/asm/exception.h
>>> +++ b/arch/arm64/include/asm/exception.h
>>> @@ -54,7 +54,9 @@ asmlinkage void el0t_32_fiq_handler(struct pt_regs 
>>> *regs);
>>>   asmlinkage void el0t_32_error_handler(struct pt_regs *regs);
>>>
>>>   asmlinkage void call_on_irq_stack(struct pt_regs *regs,
>>> -                                 void (*func)(struct pt_regs *));
>>> +                                 void (*func)(struct pt_regs *),
>>> +                                 void (*do_func)(struct pt_regs *,
>>> +                                                 void (*)(struct 
>>> pt_regs *)));
>>>   asmlinkage void asm_exit_to_user_mode(struct pt_regs *regs);
>>>
>>>   void do_mem_abort(unsigned long far, unsigned long esr, struct 
>>> pt_regs *regs);
>>> diff --git a/arch/arm64/kernel/entry-common.c 
>>> b/arch/arm64/kernel/entry-common.c
>>> index c75ca36b4a49..935d1ab150b5 100644
>>> --- a/arch/arm64/kernel/entry-common.c
>>> +++ b/arch/arm64/kernel/entry-common.c
>>> @@ -266,14 +266,16 @@ static void __sched 
>>> arm64_preempt_schedule_irq(void)
>>>   }
>>>
>>>   static void do_interrupt_handler(struct pt_regs *regs,
>>> -                                void (*handler)(struct pt_regs *))
>>> +                                void (*handler)(struct pt_regs *),
>>> +                                void (*do_handler)(struct pt_regs *,
>>> +                                                   void (*)(struct 
>>> pt_regs *)))
>>>   {
>>>          struct pt_regs *old_regs = set_irq_regs(regs);
>>>
>>>          if (on_thread_stack())
>>> -               call_on_irq_stack(regs, handler);
>>> +               call_on_irq_stack(regs, handler, do_handler);
>>>          else
>>> -               handler(regs);
>>> +               do_handler(regs, handler);
>>>
>>>          set_irq_regs(old_regs);
>>>   }
>>> @@ -441,22 +443,32 @@ asmlinkage void noinstr 
>>> el1h_64_sync_handler(struct pt_regs *regs)
>>>          }
>>>   }
>>>
>>> +static void nmi_handler(struct pt_regs *regs, void (*handler)(struct 
>>> pt_regs *))
>>> +{
>>> +       handler(regs);
>>> +}
>>> +
>>>   static __always_inline void __el1_pnmi(struct pt_regs *regs,
>>>                                         void (*handler)(struct 
>>> pt_regs *))
>>>   {
>>>          arm64_enter_nmi(regs);
>>> -       do_interrupt_handler(regs, handler);
>>> +       do_interrupt_handler(regs, handler, nmi_handler);
>>>          arm64_exit_nmi(regs);
>>>   }
>>>
>>> +static void irq_handler(struct pt_regs *regs, void (*handler)(struct 
>>> pt_regs *))
>>> +{
>>> +       irq_enter_rcu();
>>> +       handler(regs);
>>> +       irq_exit_rcu();
>>> +}
>>> +
>>>   static __always_inline void __el1_irq(struct pt_regs *regs,
>>>                                        void (*handler)(struct pt_regs 
>>> *))
>>>   {
>>>          enter_from_kernel_mode(regs);
>>>
>>> -       irq_enter_rcu();
>>> -       do_interrupt_handler(regs, handler);
>>> -       irq_exit_rcu();
>>> +       do_interrupt_handler(regs, handler, irq_handler);
>>>
>>>          arm64_preempt_schedule_irq();
>>>
>>> @@ -699,9 +711,7 @@ static void noinstr el0_interrupt(struct pt_regs 
>>> *regs,
>>>          if (regs->pc & BIT(55))
>>>                  arm64_apply_bp_hardening();
>>>
>>> -       irq_enter_rcu();
>>> -       do_interrupt_handler(regs, handler);
>>> -       irq_exit_rcu();
>>> +       do_interrupt_handler(regs, handler, irq_handler);
>>>
>>>          exit_to_user_mode(regs);
>>>   }
>>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>>> index 254fe31c03a0..1c351391f6bd 100644
>>> --- a/arch/arm64/kernel/entry.S
>>> +++ b/arch/arm64/kernel/entry.S
>>> @@ -867,7 +867,9 @@ NOKPROBE(ret_from_fork)
>>>
>>>   /*
>>>    * void call_on_irq_stack(struct pt_regs *regs,
>>> - *                       void (*func)(struct pt_regs *));
>>> + *                       void (*func)(struct pt_regs *)
>>> + *                       void (*do_func)(struct pt_regs *,
>>> + *                                       void (*)(struct pt_regs *)));
>>>    *
>>>    * Calls func(regs) using this CPU's irq stack and shadow irq stack.
>>>    */
>>> @@ -886,7 +888,7 @@ SYM_FUNC_START(call_on_irq_stack)
>>>
>>>          /* Move to the new stack and call the function there */
>>>          mov     sp, x16
>>> -       blr     x1
>>> +       blr     x2
>>>
>>>          /*
>>>           * Restore the SP from the FP, and restore the FP and LR 
>>> from the frame
>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
>>> index c36ad20a52f3..003db605bc4f 100644
>>> --- a/arch/arm64/kernel/irq.c
>>> +++ b/arch/arm64/kernel/irq.c
>>> @@ -73,14 +73,15 @@ static void init_irq_stacks(void)
>>>   #endif
>>>
>>>   #ifndef CONFIG_PREEMPT_RT
>>> -static void ____do_softirq(struct pt_regs *regs)
>>> +static void ____do_softirq(struct pt_regs *regs,
>>> +                          void (*handler)(struct pt_regs *))
>>>   {
>>>          __do_softirq();
>>>   }
>>>
>>>   void do_softirq_own_stack(void)
>>>   {
>>> -       call_on_irq_stack(NULL, ____do_softirq);
>>> +       call_on_irq_stack(NULL, NULL, ____do_softirq);
>>>   }
>>>   #endif
>>>
>>> -- 
>>> 2.20.1
>>>
> 

-- 
Thanks,
Qi