[RFC PATCH 0/2] arm64: run softirqs on the per-CPU IRQ stack

[RFC PATCH 0/2] arm64: run softirqs on the per-CPU IRQ stack

[Qi Zheng]

Hi all,

Currently arm64 supports per-CPU IRQ stack, but softirqs are
still handled in the task context.

Since any call to local_bh_enable() at any level in the task's
call stack may trigger a softirq processing run, which could
potentially cause a task stack overflow if the combined stack
footprints exceed the stack's size. And we did encounter this
situation in the real environment:

Call trace:
 dump_backtrace+0x0/0x1cc,
 show_stack+0x14/0x1c,
 dump_stack+0xc4/0xfc,
 panic+0x150/0x2c8,
 panic+0x0/0x2c8,
 handle_bad_stack+0x11c/0x130,
 __bad_stack+0x88/0x8c,
 vsnprintf+0x2c/0x524,
 vscnprintf+0x38/0x7c,
 scnprintf+0x6c/0x90,
 /* ... */
 __do_softirq+0x1e0/0x370,
 do_softirq+0x40/0x50,
 __local_bh_enable_ip+0x8c/0x90,
 _raw_spin_unlock_bh+0x1c/0x24,
 /* ... */
 process_one_work+0x1dc/0x3e4,
 worker_thread+0x260/0x360,
 kthread+0x118/0x128,
 ret_from_fork+0x10/0x18,

So let's run these softirqs on the IRQ stack as well.

This series is based on next-20220705.

Comments and suggestions are welcome.

Thanks,
Qi.

Qi Zheng (2):
  arm64: run softirqs on the per-CPU IRQ stack
  arm64: support HAVE_IRQ_EXIT_ON_IRQ_STACK

 arch/arm64/Kconfig                 |  2 ++
 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            | 12 ++++++++++++
 5 files changed, 41 insertions(+), 13 deletions(-)

-- 
2.20.1

[RFC PATCH 1/2] arm64: run softirqs on the per-CPU IRQ stack

[Qi Zheng]

Currently arm64 supports per-CPU IRQ stack, but softirqs
are still handled in the task context.

Since any call to local_bh_enable() at any level in the task's
call stack may trigger a softirq processing run, which could
potentially cause a task stack overflow if the combined stack
footprints exceed the stack's size, let's run these softirqs
on the IRQ stack as well.

Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
---
 arch/arm64/Kconfig      |  1 +
 arch/arm64/kernel/irq.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 3491d0d99891..402e16fec02a 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -230,6 +230,7 @@ config ARM64
 	select HAVE_ARCH_USERFAULTFD_MINOR if USERFAULTFD
 	select TRACE_IRQFLAGS_SUPPORT
 	select TRACE_IRQFLAGS_NMI_SUPPORT
+	select HAVE_SOFTIRQ_ON_OWN_STACK
 	help
 	  ARM 64-bit (AArch64) Linux support.
 
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index bda49430c9ea..e6aa37672fd4 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -22,6 +22,7 @@
 #include <linux/vmalloc.h>
 #include <asm/daifflags.h>
 #include <asm/vmap_stack.h>
+#include <asm/exception.h>
 
 /* Only access this in an NMI enter/exit */
 DEFINE_PER_CPU(struct nmi_ctx, nmi_contexts);
@@ -71,6 +72,16 @@ static void init_irq_stacks(void)
 }
 #endif
 
+static void ____do_softirq(struct pt_regs *regs)
+{
+	__do_softirq();
+}
+
+void do_softirq_own_stack(void)
+{
+	call_on_irq_stack(NULL, ____do_softirq);
+}
+
 static void default_handle_irq(struct pt_regs *regs)
 {
 	panic("IRQ taken without a root IRQ handler\n");
-- 
2.20.1

Re: [RFC PATCH 1/2] arm64: run softirqs on the per-CPU IRQ stack

[Arnd Bergmann]

On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>
> Currently arm64 supports per-CPU IRQ stack, but softirqs
> are still handled in the task context.
>
> Since any call to local_bh_enable() at any level in the task's
> call stack may trigger a softirq processing run, which could
> potentially cause a task stack overflow if the combined stack
> footprints exceed the stack's size, let's run these softirqs
> on the IRQ stack as well.
>
> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>

I think this is the correct approach, but your patch conflicts with another
patch I have queued up in the asm-generic tree, see
https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git/commit/?h=asm-generic&id=f2c5092190f21

Please adapt accordingly.

Are there any architectures left that use IRQ stacks but don't
set HAVE_SOFTIRQ_ON_OWN_STACK? If not, we could
also consider removing the Kconfig symbol and just requiring
it to be done this way (for non-PREEMPT_RT).

        Arnd

Re: [RFC PATCH 1/2] arm64: run softirqs on the per-CPU IRQ stack

[Qi Zheng]

On 2022/7/7 20:58, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>>
>> Currently arm64 supports per-CPU IRQ stack, but softirqs
>> are still handled in the task context.
>>
>> Since any call to local_bh_enable() at any level in the task's
>> call stack may trigger a softirq processing run, which could
>> potentially cause a task stack overflow if the combined stack
>> footprints exceed the stack's size, let's run these softirqs
>> on the IRQ stack as well.
>>
>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> 
> I think this is the correct approach, but your patch conflicts with another
> patch I have queued up in the asm-generic tree, see
> https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git/commit/?h=asm-generic&id=f2c5092190f21
> 
> Please adapt accordingly.

OK, will do in the next version.

> 
> Are there any architectures left that use IRQ stacks but don't
> set HAVE_SOFTIRQ_ON_OWN_STACK? If not, we could
> also consider removing the Kconfig symbol and just requiring
> it to be done this way (for non-PREEMPT_RT).

I haven't taken a close look at other architectures than x86 and arm,
but I think it's a good idea.

Thanks,
Qi

> 
>          Arnd

-- 
Thanks,
Qi

Re: [RFC PATCH 1/2] arm64: run softirqs on the per-CPU IRQ stack

[Arnd Bergmann]

On Thu, Jul 7, 2022 at 3:43 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
> On 2022/7/7 20:58, Arnd Bergmann wrote:
> > On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
> > Are there any architectures left that use IRQ stacks but don't
> > set HAVE_SOFTIRQ_ON_OWN_STACK? If not, we could
> > also consider removing the Kconfig symbol and just requiring
> > it to be done this way (for non-PREEMPT_RT).
>
> I haven't taken a close look at other architectures than x86 and arm,
> but I think it's a good idea.

I had another look in the meantime, and I think it's only mips and loongarch
now that don't use HAVE_SOFTIRQ_ON_OWN_STACK. Not sure about
arch/um/, which is a bit different from the rest.

       Arnd

Re: [RFC PATCH 1/2] arm64: run softirqs on the per-CPU IRQ stack

[Qi Zheng]

On 2022/7/7 21:53, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 3:43 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>> On 2022/7/7 20:58, Arnd Bergmann wrote:
>>> On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>>> Are there any architectures left that use IRQ stacks but don't
>>> set HAVE_SOFTIRQ_ON_OWN_STACK? If not, we could
>>> also consider removing the Kconfig symbol and just requiring
>>> it to be done this way (for non-PREEMPT_RT).
>>
>> I haven't taken a close look at other architectures than x86 and arm,
>> but I think it's a good idea.
> 
> I had another look in the meantime, and I think it's only mips and loongarch
> now that don't use HAVE_SOFTIRQ_ON_OWN_STACK. Not sure about
> arch/um/, which is a bit different from the rest.

I just glanced at arch/um/, and it's really different from the rest:

  * Unlike i386, UML doesn't receive IRQs on the normal kernel stack
  * and switch over to the IRQ stack after some preparation.

> 
>         Arnd

-- 
Thanks,
Qi

[RFC PATCH 2/2] arm64: support HAVE_IRQ_EXIT_ON_IRQ_STACK

[Qi Zheng]

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>
---
 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 402e16fec02a..89f6368b705e 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 e6aa37672fd4..54cd418d47ef 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -72,14 +72,15 @@ static void init_irq_stacks(void)
 }
 #endif
 
-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);
 }
 
 static void default_handle_irq(struct pt_regs *regs)
-- 
2.20.1

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

[Arnd Bergmann]

On Thu, Jul 7, 2022 at 1:05 PM 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>

I think the idea is right, but the extra function pointer adds more complexity
than necessary:

>  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);
>  }

Would it be possible to instead pull out the call_on_irq_stack() so these
two functions are instead called on the IRQ stack already?

        Arnd

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

[Qi Zheng]

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:
>>
>> 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>
> 
> I think the idea is right, but the extra function pointer adds more complexity
> than necessary:
> 
>>   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);
>>   }
> 
> Would it be possible to instead pull out the call_on_irq_stack() so these
> two functions are instead called on the IRQ stack already?

Hi,

Do you mean to modify call_on_irq_stack()?

I have tried doing a conditional jump inside call_on_irq_stack() like
this:

--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -888,13 +888,22 @@ SYM_FUNC_START(call_on_irq_stack)

         /* Move to the new stack and call the function there */
         mov     sp, x16
-       blr     x1
+
+       cmp     x2, #1
+       b.eq    99f
+
+       blr     x1
+       b       999f
+
+99:    bl      irq_enter_rcu
+       blr     x1
+       bl      irq_exit_rcu

         /*
          * 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.

> 
>          Arnd

Thanks,
Qi

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

[Arnd Bergmann]

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);
+       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

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@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

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

[Arnd Bergmann]

On Thu, Jul 7, 2022 at 5:00 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
> 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:
> >
> > -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.

Ah, too bad. I spent some more time looking for a simpler approach,
but couldn't find one I'm happy with. One idea might be to have
callback functions for each combinations of irq/fiq with irq/pnmi
to avoid the nested callback pointers. Not sure if that helps.

       Arnd

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

[Qi Zheng]

On 2022/7/8 04:55, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 5:00 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>> 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:
>>>
>>> -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.
> 
> Ah, too bad. I spent some more time looking for a simpler approach,
> but couldn't find one I'm happy with. One idea might be to have
> callback functions for each combinations of irq/fiq with irq/pnmi
> to avoid the nested callback pointers. Not sure if that helps.

Maybe nested callback pointers are not always a wild beast. ;)
This method does not change much, and we can also conveniently stuff
all kinds of things in do_handler() that we want to run on the IRQ
stack in addition to the handler().

Thanks,
Qi

> 
>         Arnd

-- 
Thanks,
Qi

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

[Arnd Bergmann]

On Fri, Jul 8, 2022 at 5:13 AM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
> On 2022/7/8 04:55, Arnd Bergmann wrote:
> > On Thu, Jul 7, 2022 at 5:00 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
> >> 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:
> >>>
> >>> -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.
> >
> > Ah, too bad. I spent some more time looking for a simpler approach,
> > but couldn't find one I'm happy with. One idea might be to have
> > callback functions for each combinations of irq/fiq with irq/pnmi
> > to avoid the nested callback pointers. Not sure if that helps.
>
> Maybe nested callback pointers are not always a wild beast. ;)
> This method does not change much, and we can also conveniently stuff
> all kinds of things in do_handler() that we want to run on the IRQ
> stack in addition to the handler().

Right, your approach is probably the one that changes the existing
code the least. I see that x86 handles this by having call_on_irq_stack()
in an inline asm, but this in turn complicates the asm implementation,
which is also worth keeping simple.

         Arnd

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

[Qi Zheng]

On 2022/7/8 16:52, Arnd Bergmann wrote:
> On Fri, Jul 8, 2022 at 5:13 AM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>> On 2022/7/8 04:55, Arnd Bergmann wrote:
>>> On Thu, Jul 7, 2022 at 5:00 PM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>>>> 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:
>>>>>
>>>>> -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.
>>>
>>> Ah, too bad. I spent some more time looking for a simpler approach,
>>> but couldn't find one I'm happy with. One idea might be to have
>>> callback functions for each combinations of irq/fiq with irq/pnmi
>>> to avoid the nested callback pointers. Not sure if that helps.
>>
>> Maybe nested callback pointers are not always a wild beast. ;)
>> This method does not change much, and we can also conveniently stuff
>> all kinds of things in do_handler() that we want to run on the IRQ
>> stack in addition to the handler().
> 
> Right, your approach is probably the one that changes the existing
> code the least. I see that x86 handles this by having call_on_irq_stack()
> in an inline asm, but this in turn complicates the asm implementation,
> which is also worth keeping simple.

Yes, and I see that the commit f2c5092190f2 ("arch/*: Disable softirq
stacks on PREEMPT_RT.") has been merged into next-20220707, so I will
rebase to the next-20220707 and send the next version.

Thank you very much :)

> 
>           Arnd

-- 
Thanks,
Qi