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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <[email protected]> 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 <[email protected]>
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
On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <[email protected]> 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 <[email protected]>
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
On 2022/7/7 20:49, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <[email protected]> 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 <[email protected]>
>
> 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
On 2022/7/7 20:58, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <[email protected]> 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 <[email protected]>
>
> 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
On Thu, Jul 7, 2022 at 3:43 PM Qi Zheng <[email protected]> wrote:
> On 2022/7/7 20:58, Arnd Bergmann wrote:
> > On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <[email protected]> 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
On 2022/7/7 22:41, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 3:38 PM Qi Zheng <[email protected]> wrote:
>> On 2022/7/7 20:49, Arnd Bergmann wrote:
>>> On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <[email protected]> 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
On 2022/7/7 21:53, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 3:43 PM Qi Zheng <[email protected]> wrote:
>> On 2022/7/7 20:58, Arnd Bergmann wrote:
>>> On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <[email protected]> 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
On Thu, Jul 7, 2022 at 3:38 PM Qi Zheng <[email protected]> wrote:
> On 2022/7/7 20:49, Arnd Bergmann wrote:
> > On Thu, Jul 7, 2022 at 1:05 PM Qi Zheng <[email protected]> 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
On Thu, Jul 7, 2022 at 5:00 PM Qi Zheng <[email protected]> wrote:
> On 2022/7/7 22:41, Arnd Bergmann wrote:
> > On Thu, Jul 7, 2022 at 3:38 PM Qi Zheng <[email protected]> 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
On 2022/7/8 04:55, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 5:00 PM Qi Zheng <[email protected]> wrote:
>> On 2022/7/7 22:41, Arnd Bergmann wrote:
>>> On Thu, Jul 7, 2022 at 3:38 PM Qi Zheng <[email protected]> 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
On Fri, Jul 8, 2022 at 5:13 AM Qi Zheng <[email protected]> wrote:
> On 2022/7/8 04:55, Arnd Bergmann wrote:
> > On Thu, Jul 7, 2022 at 5:00 PM Qi Zheng <[email protected]> wrote:
> >> On 2022/7/7 22:41, Arnd Bergmann wrote:
> >>> On Thu, Jul 7, 2022 at 3:38 PM Qi Zheng <[email protected]> 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
On 2022/7/8 16:52, Arnd Bergmann wrote:
> On Fri, Jul 8, 2022 at 5:13 AM Qi Zheng <[email protected]> wrote:
>> On 2022/7/8 04:55, Arnd Bergmann wrote:
>>> On Thu, Jul 7, 2022 at 5:00 PM Qi Zheng <[email protected]> wrote:
>>>> On 2022/7/7 22:41, Arnd Bergmann wrote:
>>>>> On Thu, Jul 7, 2022 at 3:38 PM Qi Zheng <[email protected]> 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