| Message ID | 20211021180236.37428-1-mark.rutland@arm.com |
|---|---|
| Headers | show |
| Series | irq: remove handle_domain_{irq,nmi}() | expand |
On Thu, Oct 21, 2021 at 8:02 AM Mark Rutland <mark.rutland@arm.com> wrote: > > This series reworks the irq code to remove them, handling the necessary > entry work consistently in entry code (be it architectural or generic). Thanks for doing this. This is obviously a much bigger patch than the original, but this really feels like it's the proper fix, and maintainable going forwards. Obviously the big issue is testing this all, but I like the patches from scanning them. Linus
On Thu, Oct 21, 2021 at 07:02:31PM +0100, Mark Rutland wrote: > In preparation for removing HANDLE_DOMAIN_IRQ_IRQENTRY, have arch/arm64 > perform all the irqentry accounting in its entry code. > > As arch/arm64 already performs portions of the irqentry logic in > enter_from_kernel_mode() and exit_to_kernel_mode(), including > rcu_irq_{enter,exit}(), the only additional calls that need to be made > are to irq_{enter,exit}_rcu(). Removing the calls to > rcu_irq_{enter,exit}() from handle_domain_irq() ensures that we inform > RCU once per IRQ entry and will correctly identify quiescent periods. > > Since we should not call irq_{enter,exit}_rcu() when entering a > pseudo-NMI, el1_interrupt() is reworked to have separate __el1_irq() and > __el1_pnmi() paths for regular IRQ and psuedo-NMI entry, with > irq_{enter,exit}_irq() only called for the former. > > In preparation for removing HANDLE_DOMAIN_IRQ, the irq regs are managed > in do_interrupt_handler() for both regular IRQ and pseudo-NMI. This is > currently redundant, but not harmful. > > For clarity the preemption logic is moved into __el1_irq(). We should > never preempt within a pseudo-NMI, and arm64_enter_nmi() already > enforces this by incrementing the preempt_count, but it's clearer if we > never invoke the preemption logic when entering a pseudo-NMI. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Will Deacon <will@kernel.org> > --- > arch/arm64/Kconfig | 1 - > arch/arm64/kernel/entry-common.c | 52 ++++++++++++++++++++++++---------------- > 2 files changed, 31 insertions(+), 22 deletions(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 553239a5a5f7..5c7ae4c3954b 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -134,7 +134,6 @@ config ARM64 > select GENERIC_GETTIMEOFDAY > select GENERIC_VDSO_TIME_NS > select HANDLE_DOMAIN_IRQ > - select HANDLE_DOMAIN_IRQ_IRQENTRY > select HARDIRQS_SW_RESEND > select HAVE_MOVE_PMD > select HAVE_MOVE_PUD > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c > index 32f9796c4ffe..f7408edf8571 100644 > --- a/arch/arm64/kernel/entry-common.c > +++ b/arch/arm64/kernel/entry-common.c > @@ -17,6 +17,7 @@ > #include <asm/daifflags.h> > #include <asm/esr.h> > #include <asm/exception.h> > +#include <asm/irq_regs.h> > #include <asm/kprobes.h> > #include <asm/mmu.h> > #include <asm/processor.h> > @@ -219,22 +220,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs) > lockdep_hardirqs_on(CALLER_ADDR0); > } > > -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs) > -{ > - if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) > - arm64_enter_nmi(regs); > - else > - enter_from_kernel_mode(regs); > -} > - > -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs) > -{ > - if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) > - arm64_exit_nmi(regs); > - else > - exit_to_kernel_mode(regs); > -} > - > static void __sched arm64_preempt_schedule_irq(void) > { > lockdep_assert_irqs_disabled(); > @@ -263,10 +248,14 @@ static void __sched arm64_preempt_schedule_irq(void) > static void do_interrupt_handler(struct pt_regs *regs, > void (*handler)(struct pt_regs *)) > { > + struct pt_regs *old_regs = set_irq_regs(regs); > + > if (on_thread_stack()) > call_on_irq_stack(regs, handler); > else > handler(regs); > + > + set_irq_regs(old_regs); > } > > extern void (*handle_arch_irq)(struct pt_regs *); > @@ -432,13 +421,22 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs) > } > } > > -static void noinstr el1_interrupt(struct pt_regs *regs, > - void (*handler)(struct pt_regs *)) > +static __always_inline void __el1_pnmi(struct pt_regs *regs, > + void (*handler)(struct pt_regs *)) > { > - write_sysreg(DAIF_PROCCTX_NOIRQ, daif); > + arm64_enter_nmi(regs); > + do_interrupt_handler(regs, handler); > + arm64_exit_nmi(regs); > +} > + > +static __always_inline void __el1_irq(struct pt_regs *regs, > + void (*handler)(struct pt_regs *)) > +{ > + enter_from_kernel_mode(regs); > > - enter_el1_irq_or_nmi(regs); > + irq_enter_rcu(); > do_interrupt_handler(regs, handler); > + irq_exit_rcu(); > > /* > * Note: thread_info::preempt_count includes both thread_info::count > @@ -449,7 +447,17 @@ static void noinstr el1_interrupt(struct pt_regs *regs, > READ_ONCE(current_thread_info()->preempt_count) == 0) > arm64_preempt_schedule_irq(); > > - exit_el1_irq_or_nmi(regs); > + exit_to_kernel_mode(regs); > +} > +static void noinstr el1_interrupt(struct pt_regs *regs, > + void (*handler)(struct pt_regs *)) > +{ > + write_sysreg(DAIF_PROCCTX_NOIRQ, daif); > + > + if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) > + __el1_pnmi(regs, handler); > + else > + __el1_irq(regs, handler); > } > > asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs) > @@ -667,7 +675,9 @@ 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(); > > exit_to_user_mode(regs); > } Reviewed-by: Pingfan Liu <kernelfans@gmail.com>
Reviewed-by: Guo Ren <guoren@kernel.org> On Fri, Oct 22, 2021 at 2:03 AM Mark Rutland <mark.rutland@arm.com> wrote: > > In preparation for removing HANDLE_DOMAIN_IRQ_IRQENTRY, have arch/riscv > perform all the irqentry accounting in its entry code. As arch/riscv > uses GENERIC_IRQ_MULTI_HANDLER, we can use generic_handle_arch_irq() to > do so. > > Since generic_handle_arch_irq() handles the irq entry and setting the > irq regs, and happens before the irqchip code calls handle_IPI(), we can > remove the redundant irq entry and irq regs manipulation from > handle_IPI(). > > There should be no functional change as a result of this patch. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Albert Ou <aou@eecs.berkeley.edu> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Palmer Dabbelt <palmer@dabbelt.com> > Cc: Paul Walmsley <paul.walmsley@sifive.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > arch/riscv/Kconfig | 1 - > arch/riscv/kernel/entry.S | 3 +-- > arch/riscv/kernel/smp.c | 9 +-------- > 3 files changed, 2 insertions(+), 11 deletions(-) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 740653063a56..301a54233c7e 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -63,7 +63,6 @@ config RISCV > select GENERIC_SMP_IDLE_THREAD > select GENERIC_TIME_VSYSCALL if MMU && 64BIT > select HANDLE_DOMAIN_IRQ > - select HANDLE_DOMAIN_IRQ_IRQENTRY > select HAVE_ARCH_AUDITSYSCALL > select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL > select HAVE_ARCH_JUMP_LABEL_RELATIVE if !XIP_KERNEL > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > index 98f502654edd..64236f7efde5 100644 > --- a/arch/riscv/kernel/entry.S > +++ b/arch/riscv/kernel/entry.S > @@ -130,8 +130,7 @@ skip_context_tracking: > > /* Handle interrupts */ > move a0, sp /* pt_regs */ > - la a1, handle_arch_irq > - REG_L a1, (a1) > + la a1, generic_handle_arch_irq > jr a1 > 1: > /* > diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c > index 921d9d7df400..2f6da845c9ae 100644 > --- a/arch/riscv/kernel/smp.c > +++ b/arch/riscv/kernel/smp.c > @@ -140,12 +140,9 @@ void arch_irq_work_raise(void) > > void handle_IPI(struct pt_regs *regs) > { > - struct pt_regs *old_regs = set_irq_regs(regs); > unsigned long *pending_ipis = &ipi_data[smp_processor_id()].bits; > unsigned long *stats = ipi_data[smp_processor_id()].stats; > > - irq_enter(); > - > riscv_clear_ipi(); > > while (true) { > @@ -156,7 +153,7 @@ void handle_IPI(struct pt_regs *regs) > > ops = xchg(pending_ipis, 0); > if (ops == 0) > - goto done; > + return; > > if (ops & (1 << IPI_RESCHEDULE)) { > stats[IPI_RESCHEDULE]++; > @@ -189,10 +186,6 @@ void handle_IPI(struct pt_regs *regs) > /* Order data access and bit testing. */ > mb(); > } > - > -done: > - irq_exit(); > - set_irq_regs(old_regs); > } > > static const char * const ipi_names[] = { > -- > 2.11.0 >
On Thu, Oct 21, 2021 at 07:02:26PM +0100, Mark Rutland wrote: > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > handle_arch_irq() without performing any entry accounting. > > Add a generic wrapper to handle the commoon irqentry work when invoking > handle_arch_irq(). Where an architecture needs to perform some entry > accounting itself, it will need to invoke handle_arch_irq() itself. > > In subsequent patches it will become the responsibilty of the entry code > to set the irq regs when entering an IRQ (rather than deferring this to > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > regs now. This can be redundant in some cases, but is never harmful as > saving/restoring the old regs nests safely. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > kernel/irq/handle.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > index 221d80c31e94..27182003b879 100644 > --- a/kernel/irq/handle.c > +++ b/kernel/irq/handle.c > @@ -14,6 +14,8 @@ > #include <linux/interrupt.h> > #include <linux/kernel_stat.h> > > +#include <asm/irq_regs.h> > + > #include <trace/events/irq.h> > > #include "internals.h" > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > handle_arch_irq = handle_irq; > return 0; > } > + > +/** > + * generic_handle_arch_irq - root irq handler for architectures which do no > + * entry accounting themselves > + * @regs: Register file coming from the low-level handling code > + */ > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > +{ > + struct pt_regs *old_regs; > + > + irq_enter(); > + old_regs = set_irq_regs(regs); > + handle_arch_irq(regs); After all involved arches call generic_handle_arch_irq(), can handle_arch_irq be encapsulated by declaring as static? Two places need to be fixed for this purpose: -1. absorb the logic about handle_arch_irq in arm64/kernel/irq.c -2. In arm, setup_arch(), #ifdef CONFIG_GENERIC_IRQ_MULTI_HANDLER handle_arch_irq = mdesc->handle_irq; #endif Thanks, Pingfan
On Fri, Oct 22, 2021 at 2:03 AM Mark Rutland <mark.rutland@arm.com> wrote: > > In preparation for removing HANDLE_DOMAIN_IRQ_IRQENTRY, have arch/csky > perform all the irqentry accounting in its entry code. As arch/csky uses > GENERIC_IRQ_MULTI_HANDLER, we can use generic_handle_arch_irq() to do > so. > > There should be no functional change as a result of this patch. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Guo Ren <guoren@kernel.org> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > arch/csky/Kconfig | 1 - > arch/csky/kernel/entry.S | 2 +- > arch/csky/kernel/irq.c | 5 ----- > 3 files changed, 1 insertion(+), 7 deletions(-) > > diff --git a/arch/csky/Kconfig b/arch/csky/Kconfig > index 45f03f674a61..9d4d898df76b 100644 > --- a/arch/csky/Kconfig > +++ b/arch/csky/Kconfig > @@ -18,7 +18,6 @@ config CSKY > select DMA_DIRECT_REMAP > select IRQ_DOMAIN > select HANDLE_DOMAIN_IRQ > - select HANDLE_DOMAIN_IRQ_IRQENTRY > select DW_APB_TIMER_OF > select GENERIC_IOREMAP > select GENERIC_LIB_ASHLDI3 > diff --git a/arch/csky/kernel/entry.S b/arch/csky/kernel/entry.S > index 00e3c8ebf9b8..a4ababf25e24 100644 > --- a/arch/csky/kernel/entry.S > +++ b/arch/csky/kernel/entry.S > @@ -249,7 +249,7 @@ ENTRY(csky_irq) > > > mov a0, sp > - jbsr csky_do_IRQ > + jbsr generic_handle_arch_irq > > jmpi ret_from_exception > > diff --git a/arch/csky/kernel/irq.c b/arch/csky/kernel/irq.c > index 03a1930f1cbb..fcdaf3156286 100644 > --- a/arch/csky/kernel/irq.c > +++ b/arch/csky/kernel/irq.c > @@ -15,8 +15,3 @@ void __init init_IRQ(void) > setup_smp_ipi(); > #endif > } > - > -asmlinkage void __irq_entry csky_do_IRQ(struct pt_regs *regs) > -{ > - handle_arch_irq(regs); > -} Seems the previous code has lost old_regs save/restore? + struct pt_regs *old_regs; + + irq_enter(); + old_regs = set_irq_regs(regs); + handle_arch_irq(regs); + set_irq_regs(old_regs); + irq_exit(); > -- > 2.11.0 >
On Fri, Oct 22, 2021 at 10:19 AM Guo Ren <guoren@kernel.org> wrote: > > On Fri, Oct 22, 2021 at 2:03 AM Mark Rutland <mark.rutland@arm.com> wrote: > > > > In preparation for removing HANDLE_DOMAIN_IRQ_IRQENTRY, have arch/csky > > perform all the irqentry accounting in its entry code. As arch/csky uses > > GENERIC_IRQ_MULTI_HANDLER, we can use generic_handle_arch_irq() to do > > so. > > > > There should be no functional change as a result of this patch. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Guo Ren <guoren@kernel.org> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > --- > > arch/csky/Kconfig | 1 - > > arch/csky/kernel/entry.S | 2 +- > > arch/csky/kernel/irq.c | 5 ----- > > 3 files changed, 1 insertion(+), 7 deletions(-) > > > > diff --git a/arch/csky/Kconfig b/arch/csky/Kconfig > > index 45f03f674a61..9d4d898df76b 100644 > > --- a/arch/csky/Kconfig > > +++ b/arch/csky/Kconfig > > @@ -18,7 +18,6 @@ config CSKY > > select DMA_DIRECT_REMAP > > select IRQ_DOMAIN > > select HANDLE_DOMAIN_IRQ > > - select HANDLE_DOMAIN_IRQ_IRQENTRY > > select DW_APB_TIMER_OF > > select GENERIC_IOREMAP > > select GENERIC_LIB_ASHLDI3 > > diff --git a/arch/csky/kernel/entry.S b/arch/csky/kernel/entry.S > > index 00e3c8ebf9b8..a4ababf25e24 100644 > > --- a/arch/csky/kernel/entry.S > > +++ b/arch/csky/kernel/entry.S > > @@ -249,7 +249,7 @@ ENTRY(csky_irq) > > > > > > mov a0, sp > > - jbsr csky_do_IRQ > > + jbsr generic_handle_arch_irq > > > > jmpi ret_from_exception > > > > diff --git a/arch/csky/kernel/irq.c b/arch/csky/kernel/irq.c > > index 03a1930f1cbb..fcdaf3156286 100644 > > --- a/arch/csky/kernel/irq.c > > +++ b/arch/csky/kernel/irq.c > > @@ -15,8 +15,3 @@ void __init init_IRQ(void) > > setup_smp_ipi(); > > #endif > > } > > - > > -asmlinkage void __irq_entry csky_do_IRQ(struct pt_regs *regs) > > -{ > > - handle_arch_irq(regs); > > -} > > Seems the previous code has lost old_regs save/restore? > > + struct pt_regs *old_regs; > + > + irq_enter(); > + old_regs = set_irq_regs(regs); > + handle_arch_irq(regs); > + set_irq_regs(old_regs); > + irq_exit(); Sorry, handle_domain_irq has done it, and C-SKY's IPI is in the handle_domain_irq. Reviewed-by: Guo Ren <guoren@kernel.org> > > > -- > > 2.11.0 > > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/
On Fri, Oct 22, 2021 at 2:03 AM Mark Rutland <mark.rutland@arm.com> wrote: > > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > handle_arch_irq() without performing any entry accounting. > > Add a generic wrapper to handle the commoon irqentry work when invoking > handle_arch_irq(). Where an architecture needs to perform some entry > accounting itself, it will need to invoke handle_arch_irq() itself. > > In subsequent patches it will become the responsibilty of the entry code > to set the irq regs when entering an IRQ (rather than deferring this to > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > regs now. This can be redundant in some cases, but is never harmful as > saving/restoring the old regs nests safely. Shall we remove old_regs save/restore in handle_domain_irq? It's duplicated. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > kernel/irq/handle.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > index 221d80c31e94..27182003b879 100644 > --- a/kernel/irq/handle.c > +++ b/kernel/irq/handle.c > @@ -14,6 +14,8 @@ > #include <linux/interrupt.h> > #include <linux/kernel_stat.h> > > +#include <asm/irq_regs.h> > + > #include <trace/events/irq.h> > > #include "internals.h" > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > handle_arch_irq = handle_irq; > return 0; > } > + > +/** > + * generic_handle_arch_irq - root irq handler for architectures which do no > + * entry accounting themselves > + * @regs: Register file coming from the low-level handling code > + */ > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > +{ > + struct pt_regs *old_regs; > + > + irq_enter(); > + old_regs = set_irq_regs(regs); > + handle_arch_irq(regs); > + set_irq_regs(old_regs); > + irq_exit(); > +} > #endif > -- > 2.11.0 >
Mark Rutland <mark.rutland@arm.com> 於 2021年10月22日 週五 上午2:03寫道： > > In preparation for removing HANDLE_DOMAIN_IRQ, have arch/nds32 perform > all the necessary IRQ entry accounting in its entry code. > > Currently arch/nds32 is tightly coupled with the ativic32 irqchip, and > while the entry code should logically live under arch/nds32/, moving the > entry logic there makes things more convoluted. So for now, place the > entry logic in the ativic32 irqchip, but separated into a separate > function to make the split of responsibility clear. > > In future this should probably use GENERIC_IRQ_MULTI_HANDLER to cleanly > decouple this. > > There should be no functional change as a result of this patch. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Greentime Hu <green.hu@gmail.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Nick Hu <nickhu@andestech.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Vincent Chen <deanbo422@gmail.com> > --- > arch/nds32/Kconfig | 1 - > drivers/irqchip/irq-ativic32.c | 22 ++++++++++++++++++++-- > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/arch/nds32/Kconfig b/arch/nds32/Kconfig > index aea26e739543..4d1421b18734 100644 > --- a/arch/nds32/Kconfig > +++ b/arch/nds32/Kconfig > @@ -27,7 +27,6 @@ config NDS32 > select GENERIC_LIB_MULDI3 > select GENERIC_LIB_UCMPDI2 > select GENERIC_TIME_VSYSCALL > - select HANDLE_DOMAIN_IRQ > select HAVE_ARCH_TRACEHOOK > select HAVE_DEBUG_KMEMLEAK > select HAVE_EXIT_THREAD > diff --git a/drivers/irqchip/irq-ativic32.c b/drivers/irqchip/irq-ativic32.c > index 476d6024aaf2..223dd2f97d28 100644 > --- a/drivers/irqchip/irq-ativic32.c > +++ b/drivers/irqchip/irq-ativic32.c > @@ -5,11 +5,14 @@ > #include <linux/of.h> > #include <linux/of_irq.h> > #include <linux/of_address.h> > +#include <linux/hardirq.h> > #include <linux/interrupt.h> > #include <linux/irqdomain.h> > #include <linux/irqchip.h> > #include <nds32_intrinsic.h> > > +#include <asm/irq_regs.h> > + > unsigned long wake_mask; > > static void ativic32_ack_irq(struct irq_data *data) > @@ -103,10 +106,25 @@ static irq_hw_number_t get_intr_src(void) > - NDS32_VECTOR_offINTERRUPT; > } > > -asmlinkage void asm_do_IRQ(struct pt_regs *regs) > +static void ativic32_handle_irq(struct pt_regs *regs) > { > irq_hw_number_t hwirq = get_intr_src(); > - handle_domain_irq(root_domain, hwirq, regs); > + generic_handle_domain_irq(root_domain, hwirq); > +} > + > +/* > + * TODO: convert nds32 to GENERIC_IRQ_MULTI_HANDLER so that this entry logic > + * can live in arch code. > + */ > +asmlinkage void asm_do_IRQ(struct pt_regs *regs) > +{ > + struct pt_regs *old_regs; > + > + irq_enter(); > + old_regs = set_irq_regs(regs); > + ativic32_handle_irq(regs); > + set_irq_regs(old_regs); > + irq_exit(); > } > > int __init ativic32_init_irq(struct device_node *node, struct device_node *parent) > -- > 2.11.0 > Loop in Alan and KC.
On Fri, Oct 22, 2021 at 10:33:53AM +0800, Guo Ren wrote: > On Fri, Oct 22, 2021 at 2:03 AM Mark Rutland <mark.rutland@arm.com> wrote: > > > > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > > handle_arch_irq() without performing any entry accounting. > > > > Add a generic wrapper to handle the commoon irqentry work when invoking Ah, I'd typo'd 'common' there; I'll go fix that... > > handle_arch_irq(). Where an architecture needs to perform some entry > > accounting itself, it will need to invoke handle_arch_irq() itself. > > > > In subsequent patches it will become the responsibilty of the entry code > > to set the irq regs when entering an IRQ (rather than deferring this to > > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > > regs now. This can be redundant in some cases, but is never harmful as > > saving/restoring the old regs nests safely. > Shall we remove old_regs save/restore in handle_domain_irq? It's duplicated. We do, in patch 15 when handle_domain_irq() is removed entirely. :) Removing it immediately in this patch would require more ifdeffery and/or another copy of handle_domain_irq(), and since the duplication doesn't cause a functional problem, I think it's better to live with the temporary duplication for the next few patches than to try to optimize the intermediate steps at the cost of legibility. Thanks, Mark. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > --- > > kernel/irq/handle.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > > index 221d80c31e94..27182003b879 100644 > > --- a/kernel/irq/handle.c > > +++ b/kernel/irq/handle.c > > @@ -14,6 +14,8 @@ > > #include <linux/interrupt.h> > > #include <linux/kernel_stat.h> > > > > +#include <asm/irq_regs.h> > > + > > #include <trace/events/irq.h> > > > > #include "internals.h" > > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > > handle_arch_irq = handle_irq; > > return 0; > > } > > + > > +/** > > + * generic_handle_arch_irq - root irq handler for architectures which do no > > + * entry accounting themselves > > + * @regs: Register file coming from the low-level handling code > > + */ > > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > > +{ > > + struct pt_regs *old_regs; > > + > > + irq_enter(); > > + old_regs = set_irq_regs(regs); > > + handle_arch_irq(regs); > > + set_irq_regs(old_regs); > > + irq_exit(); > > +} > > #endif > > -- > > 2.11.0 > > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/
On Fri, Oct 22, 2021 at 10:10:26AM +0800, Pingfan Liu wrote: > On Thu, Oct 21, 2021 at 07:02:26PM +0100, Mark Rutland wrote: > > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > > handle_arch_irq() without performing any entry accounting. > > > > Add a generic wrapper to handle the commoon irqentry work when invoking > > handle_arch_irq(). Where an architecture needs to perform some entry > > accounting itself, it will need to invoke handle_arch_irq() itself. > > > > In subsequent patches it will become the responsibilty of the entry code > > to set the irq regs when entering an IRQ (rather than deferring this to > > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > > regs now. This can be redundant in some cases, but is never harmful as > > saving/restoring the old regs nests safely. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > --- > > kernel/irq/handle.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > > index 221d80c31e94..27182003b879 100644 > > --- a/kernel/irq/handle.c > > +++ b/kernel/irq/handle.c > > @@ -14,6 +14,8 @@ > > #include <linux/interrupt.h> > > #include <linux/kernel_stat.h> > > > > +#include <asm/irq_regs.h> > > + > > #include <trace/events/irq.h> > > > > #include "internals.h" > > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > > handle_arch_irq = handle_irq; > > return 0; > > } > > + > > +/** > > + * generic_handle_arch_irq - root irq handler for architectures which do no > > + * entry accounting themselves > > + * @regs: Register file coming from the low-level handling code > > + */ > > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > > +{ > > + struct pt_regs *old_regs; > > + > > + irq_enter(); > > + old_regs = set_irq_regs(regs); > > + handle_arch_irq(regs); > > After all involved arches call generic_handle_arch_irq(), can > handle_arch_irq be encapsulated by declaring as static? > > Two places need to be fixed for this purpose: > -1. absorb the logic about handle_arch_irq in arm64/kernel/irq.c > -2. In arm, setup_arch(), > #ifdef CONFIG_GENERIC_IRQ_MULTI_HANDLER > handle_arch_irq = mdesc->handle_irq; > #endif That arm bit would need to be set_handle_irq(mdesc->handle_irq); anywhere it uses handle_arch_irq it's depending on the CONFIG_GENERIC_IRQ_MULTI_HANDLER definition. While I agree it would seem nice to encapsulate this, in future we want architectures to move to the more correct entry sequencing described in the cover letter, and when that happens they should invoke handle_arch_irq() directly, so I think this is best to leave as-is. We have custom logic on arm64 because we want to handle IRQ and FIQ consistently, and there wasn't a neat way to bodge that into the generic code, but that issue doesn't apply to the other users of CONFIG_GENERIC_IRQ_MULTI_HANDLER. Thanks, Mark.
On Thu, 21 Oct 2021 19:02:25 +0100, Mark Rutland <mark.rutland@arm.com> wrote: > > There's no need for handle_domain_{irq,nmi}() to open-code the NULL > check performed by handle_irq_desc(), nor the resolution of the desc > performed by generic_handle_domain_irq(). > > Use generic_handle_domain_irq() directly, as this is functioanlly > equivalent and clearer. At the same time, delete the stale comments, > which are no longer helpful. > > There should be no functional change as a result of this patch. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > kernel/irq/irqdesc.c | 24 ++++-------------------- > 1 file changed, 4 insertions(+), 20 deletions(-) > > diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c > index 4e3c29bb603c..b07d0e1552bc 100644 > --- a/kernel/irq/irqdesc.c > +++ b/kernel/irq/irqdesc.c > @@ -690,17 +690,11 @@ int handle_domain_irq(struct irq_domain *domain, > unsigned int hwirq, struct pt_regs *regs) > { > struct pt_regs *old_regs = set_irq_regs(regs); > - struct irq_desc *desc; > - int ret = 0; > + int ret; > > irq_enter(); > > - /* The irqdomain code provides boundary checks */ > - desc = irq_resolve_mapping(domain, hwirq); > - if (likely(desc)) > - handle_irq_desc(desc); > - else > - ret = -EINVAL; > + ret = generic_handle_domain_irq(domain, hwirq); > > irq_exit(); > set_irq_regs(old_regs); > @@ -721,24 +715,14 @@ int handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq, > struct pt_regs *regs) > { > struct pt_regs *old_regs = set_irq_regs(regs); > - struct irq_desc *desc; > - int ret = 0; > + int ret; > > /* > * NMI context needs to be setup earlier in order to deal with tracing. > */ > WARN_ON(!in_nmi()); > > - desc = irq_resolve_mapping(domain, hwirq); > - > - /* > - * ack_bad_irq is not NMI-safe, just report > - * an invalid interrupt. > - */ > - if (likely(desc)) > - handle_irq_desc(desc); > - else > - ret = -EINVAL; > + ret = generic_handle_domain_irq(domain, hwirq); > > set_irq_regs(old_regs); > return ret; Yup, that's really neat. I somehow missed that when I moved some of the legacy stuff to be ARM specific. On a vaguely related note, I think you can drop the EXPORT_SYMBOL_GPL on handle_irq_desc() now. M.
Hi Mark, On Thu, 21 Oct 2021 19:02:21 +0100, Mark Rutland <mark.rutland@arm.com> wrote: > > The handle_domain_{irq,nmi}() functions were oringally intended as a > convenience, but recent rework to entry code across the kernel tree has > demonstrated that they cause more pain than they're worth and prevent > architectures from being able to write robust entry code. > > This series reworks the irq code to remove them, handling the necessary > entry work consistently in entry code (be it architectural or generic). [...] Thanks for going through the pain of putting this together. The couple of nits I mentioned notwithstanding: Reviewed-by: Marc Zyngier <maz@kernel.org> It'd be good to work out a merging strategy once this has seen a bit of testing. M.
On Fri, Oct 22, 2021 at 11:38:23AM +0100, Marc Zyngier wrote: > On Thu, 21 Oct 2021 19:02:22 +0100, > Mark Rutland <mark.rutland@arm.com> wrote: > > > > As bcm6345_l1_irq_handle() is a chained irqchip handler, it will be > > invoked within the context of the root irqchip handler, which must have > > entered IRQ context already. > > > > When bcm6345_l1_irq_handle() calls arch/mips's do_IRQ() , this will nest > > another call to irq_enter(), and the resulting nested increment to > > `rcu_data.dynticks_nmi_nesting` will cause rcu_is_cpu_rrupt_from_idle() > > to fail to identify wakeups from idle, resulting in failure to preempt, > > and RCU stalls. > > > > Chained irqchip handlers must invoke IRQ handlers by way of thee core > > irqchip code, i.e. generic_handle_irq() or generic_handle_domain_irq() > > and should not call do_IRQ(), which is intended only for root irqchip > > handlers. > > > > Fix bcm6345_l1_irq_handle() by calling generic_handle_irq() directly. > > > > Fixes: c7c42ec2baa1de7a ("irqchips/bmips: Add bcm6345-l1 interrupt controller") > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > --- > > drivers/irqchip/irq-bcm6345-l1.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c > > index e3483789f4df..1bd0621c4ce2 100644 > > --- a/drivers/irqchip/irq-bcm6345-l1.c > > +++ b/drivers/irqchip/irq-bcm6345-l1.c > > @@ -140,7 +140,7 @@ static void bcm6345_l1_irq_handle(struct irq_desc *desc) > > for_each_set_bit(hwirq, &pending, IRQS_PER_WORD) { > > irq = irq_linear_revmap(intc->domain, base + hwirq); > > if (irq) > > - do_IRQ(irq); > > + generic_handle_irq(irq); > > else > > spurious_interrupt(); > > } > > A marginally better fix would be to have: > > diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c > index 1bd0621c4ce2..fd079215c17f 100644 > --- a/drivers/irqchip/irq-bcm6345-l1.c > +++ b/drivers/irqchip/irq-bcm6345-l1.c > @@ -132,16 +132,12 @@ static void bcm6345_l1_irq_handle(struct irq_desc *desc) > int base = idx * IRQS_PER_WORD; > unsigned long pending; > irq_hw_number_t hwirq; > - unsigned int irq; > > pending = __raw_readl(cpu->map_base + reg_status(intc, idx)); > pending &= __raw_readl(cpu->map_base + reg_enable(intc, idx)); > > for_each_set_bit(hwirq, &pending, IRQS_PER_WORD) { > - irq = irq_linear_revmap(intc->domain, base + hwirq); > - if (irq) > - generic_handle_irq(irq); > - else > + if (generic_handle_domain_irq(intc->domain, base + hwirq)) > spurious_interrupt(); > } > } > > but we can also tackle that separately if you'd rather keep the change > minimal. I'll add that to the series immediately after this patch, to keep this change minimal for backporting. Thanks, Mark.
On Fri, Oct 22, 2021 at 11:52:28AM +0100, Marc Zyngier wrote: > On Thu, 21 Oct 2021 19:02:25 +0100, > Mark Rutland <mark.rutland@arm.com> wrote: > > > > There's no need for handle_domain_{irq,nmi}() to open-code the NULL > > check performed by handle_irq_desc(), nor the resolution of the desc > > performed by generic_handle_domain_irq(). > > > > Use generic_handle_domain_irq() directly, as this is functioanlly > > equivalent and clearer. At the same time, delete the stale comments, > > which are no longer helpful. > > > > There should be no functional change as a result of this patch. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > --- > > kernel/irq/irqdesc.c | 24 ++++-------------------- > > 1 file changed, 4 insertions(+), 20 deletions(-) > Yup, that's really neat. I somehow missed that when I moved some of > the legacy stuff to be ARM specific. > > On a vaguely related note, I think you can drop the EXPORT_SYMBOL_GPL > on handle_irq_desc() now. Seems so: [mark@lakrids:~/src/linux]% git grep -w handle_irq_desc arch/arm/kernel/irq.c: handle_irq_desc(desc); include/linux/irqdesc.h:int handle_irq_desc(struct irq_desc *desc); kernel/irq/irqdesc.c:int handle_irq_desc(struct irq_desc *desc) kernel/irq/irqdesc.c: return handle_irq_desc(irq_to_desc(irq)); kernel/irq/irqdesc.c: return handle_irq_desc(irq_resolve_mapping(domain, hwirq)); I'll add a patch to clean that up. Thanks, Mark
On Fri, Oct 22, 2021 at 12:20:31PM +0100, Marc Zyngier wrote: > Hi Mark, > > On Thu, 21 Oct 2021 19:02:21 +0100, > Mark Rutland <mark.rutland@arm.com> wrote: > > > > The handle_domain_{irq,nmi}() functions were oringally intended as a > > convenience, but recent rework to entry code across the kernel tree has > > demonstrated that they cause more pain than they're worth and prevent > > architectures from being able to write robust entry code. > > > > This series reworks the irq code to remove them, handling the necessary > > entry work consistently in entry code (be it architectural or generic). > > [...] > > Thanks for going through the pain of putting this together. The > couple of nits I mentioned notwithstanding: > > Reviewed-by: Marc Zyngier <maz@kernel.org> Thanks! I've pushed out an updated version to my irq/handle-domain-irq branch on kernel.org: git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git That has two new patches you suggested: * irq: mips: simplify bcm6345_l1_irq_handle() * irq: unexport handle_irq_desc() ... which I did not add your Reviewed-by to in case the commit messages are garbage or something like that. > It'd be good to work out a merging strategy once this has seen a bit > of testing. Conflict-wise, this merges near perfectly against next-20212022 aside from a trivial conflict against arch/riscv/Kconfig: | [mark@lakrids:~/src/linux]% git merge irq/handle-domain-irq | Auto-merging arch/riscv/kernel/entry.S | Auto-merging arch/riscv/Kconfig | CONFLICT (content): Merge conflict in arch/riscv/Kconfig | Auto-merging arch/nds32/Kconfig | Auto-merging arch/mips/Kconfig | Auto-merging arch/csky/Kconfig | Auto-merging arch/arm64/Kconfig | Auto-merging arch/arm/mach-s3c/irq-s3c24xx.c | Auto-merging arch/arm/kernel/entry-armv.S | Auto-merging arch/arm/Kconfig | Auto-merging arch/arc/Kconfig | Automatic merge failed; fix conflicts and then commit the result. | [mark@lakrids:~/src/linux]% git diff | diff --cc arch/riscv/Kconfig | index 77a088d0a7e9,353e28f5f849..000000000000 | --- a/arch/riscv/Kconfig | +++ b/arch/riscv/Kconfig | @@@ -62,8 -62,6 +62,11 @@@ config RISC | select GENERIC_SCHED_CLOCK | select GENERIC_SMP_IDLE_THREAD | select GENERIC_TIME_VSYSCALL if MMU && 64BIT | ++<<<<<<< HEAD | + select GENERIC_VDSO_TIME_NS if HAVE_GENERIC_VDSO | + select HANDLE_DOMAIN_IRQ | ++======= | ++>>>>>>> irq/handle-domain-irq | select HAVE_ARCH_AUDITSYSCALL | select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL | select HAVE_ARCH_JUMP_LABEL_RELATIVE if !XIP_KERNEL ... where the resolution is: | diff --cc arch/riscv/Kconfig | index 77a088d0a7e9,353e28f5f849..000000000000 | --- a/arch/riscv/Kconfig | +++ b/arch/riscv/Kconfig | @@@ -62,8 -62,6 +62,7 @@@ config RISC | select GENERIC_SCHED_CLOCK | select GENERIC_SMP_IDLE_THREAD | select GENERIC_TIME_VSYSCALL if MMU && 64BIT | + select GENERIC_VDSO_TIME_NS if HAVE_GENERIC_VDSO | - select HANDLE_DOMAIN_IRQ | select HAVE_ARCH_AUDITSYSCALL | select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL | select HAVE_ARCH_JUMP_LABEL_RELATIVE if !XIP_KERNEL ... so I reckon we're not set for major pain there unless something new appears in arch code in the next few days. If we can get this onto a branch for linux-next ASAP, and if Linus is happy with this having come together a little late, maybe we could queue this in tip for v5.16, perhaps after -rc1 to let this soak, or waiting to apply the final patch to make it easier to revert the arch changes if needed? I'd like to avoid sitting on this for an entire cycle if possible. Thanks, Mark.
Hi Mark, On 10/21/21 7:02 PM, Mark Rutland wrote: > +/* > + * TODO: restructure the ARMv7M entry logic so that this entry logic can live > + * in arch code. > + */ > +asmlinkage void __exception_irq_entry > +static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) I'm seeing build time failure... drivers/irqchip/irq-nvic.c:50:8: error: two or more data types in declaration specifiers static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) ^~~~ drivers/irqchip/irq-nvic.c:50:13: warning: 'nvic_handle_irq' defined but not used [-Wunused-function] static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) I've fixed that locally and planing to give it a go... Cheers Vladimir
On Fri, Oct 22, 2021 at 04:18:18PM +0100, Vladimir Murzin wrote: > Hi Mark, > > On 10/21/21 7:02 PM, Mark Rutland wrote: > > +/* > > + * TODO: restructure the ARMv7M entry logic so that this entry logic can live > > + * in arch code. > > + */ > > +asmlinkage void __exception_irq_entry > > +static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > > I'm seeing build time failure... > > drivers/irqchip/irq-nvic.c:50:8: error: two or more data types in declaration specifiers > static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > ^~~~ > drivers/irqchip/irq-nvic.c:50:13: warning: 'nvic_handle_irq' defined but not used [-Wunused-function] > static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > > I've fixed that locally and planing to give it a go... Ah, whoops. I've removed the extraneous `static void` from nvic_handle_irq() and build tested that as part of stm32_defconfig. The updated version is in my irq/handle-domain-irq branch at: git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git Thanks, Mark.
On Fri, Oct 22, 2021 at 05:34:20PM +0100, Vladimir Murzin wrote: > On 10/22/21 4:36 PM, Mark Rutland wrote: > > On Fri, Oct 22, 2021 at 04:18:18PM +0100, Vladimir Murzin wrote: > >> Hi Mark, > >> > >> On 10/21/21 7:02 PM, Mark Rutland wrote: > >>> +/* > >>> + * TODO: restructure the ARMv7M entry logic so that this entry logic can live > >>> + * in arch code. > >>> + */ > >>> +asmlinkage void __exception_irq_entry > >>> +static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > >> > >> I'm seeing build time failure... > >> > >> drivers/irqchip/irq-nvic.c:50:8: error: two or more data types in declaration specifiers > >> static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > >> ^~~~ > >> drivers/irqchip/irq-nvic.c:50:13: warning: 'nvic_handle_irq' defined but not used [-Wunused-function] > >> static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > >> > >> I've fixed that locally and planing to give it a go... > > > > Ah, whoops. I've removed the extraneous `static void` from > > nvic_handle_irq() and build tested that as part of stm32_defconfig. > > > > The updated version is in my irq/handle-domain-irq branch at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git > > > > $ cat /proc/interrupts > CPU0 > 16: 24 nvic_irq 4 Edge mps2-clkevt > 17: 0 nvic_irq 32 Edge mps2-uart-rx > 18: 6 nvic_irq 33 Edge mps2-uart-tx > 19: 0 nvic_irq 47 Edge mps2-uart-overrun > Err: 0 > > So if it helps feel free to add my > > Tested-by: Vladimir Murzin <vladimir.murzin@arm.com> # ARMv7M Thanks! I've folded that in and uppdated the branch. > As for TODO, is [1] look something you have been thinking of? IIUC, > the show stopper is that hwirq is being passed from exception entry > which retrieved via xPSR (IPSR to be precise). OTOH hwirq also available > via Interrupt Controller Status Register (ICSR) thus can be used in > driver itself... I gave [1] a go and it runs fine, yet I admit I might > be missing something... I hadn't thought about it in much detail, but that looks good! I was wondering if we needed something like a handle_arch_vectored_irq(), but if we can rely on the ICSR that seems simpler overall. I'm not at all familiar with M-class, so I'm not sure if there are pitfalls in this area. Thanks, Mark. > > [1] > > --- > arch/arm/include/asm/v7m.h | 3 ++- > arch/arm/kernel/entry-v7m.S | 10 +++------- > drivers/irqchip/Kconfig | 1 + > drivers/irqchip/irq-nvic.c | 21 +++++---------------- > 4 files changed, 11 insertions(+), 24 deletions(-) > > diff --git a/arch/arm/include/asm/v7m.h b/arch/arm/include/asm/v7m.h > index b1bad30b15d2..f047629887e7 100644 > --- a/arch/arm/include/asm/v7m.h > +++ b/arch/arm/include/asm/v7m.h > @@ -13,6 +13,7 @@ > #define V7M_SCB_ICSR_PENDSVSET (1 << 28) > #define V7M_SCB_ICSR_PENDSVCLR (1 << 27) > #define V7M_SCB_ICSR_RETTOBASE (1 << 11) > +#define V7M_SCB_ICSR_VECTACTIVE 0x000001ff > > #define V7M_SCB_VTOR 0x08 > > @@ -38,7 +39,7 @@ > #define V7M_SCB_SHCSR_MEMFAULTENA (1 << 16) > > #define V7M_xPSR_FRAMEPTRALIGN 0x00000200 > -#define V7M_xPSR_EXCEPTIONNO 0x000001ff > +#define V7M_xPSR_EXCEPTIONNO V7M_SCB_ICSR_VECTACTIVE > > /* > * When branching to an address that has bits [31:28] == 0xf an exception return > diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S > index 2e872a248e31..901c7cd1b1ce 100644 > --- a/arch/arm/kernel/entry-v7m.S > +++ b/arch/arm/kernel/entry-v7m.S > @@ -72,14 +72,10 @@ __irq_entry: > @ > @ Invoke the IRQ handler > @ > - mrs r0, ipsr > - ldr r1, =V7M_xPSR_EXCEPTIONNO > - and r0, r1 > - sub r0, #16 > - mov r1, sp > + mov r0, sp > stmdb sp!, {lr} > - @ routine called with r0 = irq number, r1 = struct pt_regs * > - bl nvic_handle_irq > + @ routine called with r0 = struct pt_regs * > + bl generic_handle_arch_irq > > pop {lr} > @ > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index aca7b595c4c7..b59a0bc0cd80 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -58,6 +58,7 @@ config ARM_NVIC > bool > select IRQ_DOMAIN_HIERARCHY > select GENERIC_IRQ_CHIP > + select GENERIC_IRQ_MULTI_HANDLER > > config ARM_VIC > bool > diff --git a/drivers/irqchip/irq-nvic.c b/drivers/irqchip/irq-nvic.c > index 63bac3f78863..52ff0ed19f2f 100644 > --- a/drivers/irqchip/irq-nvic.c > +++ b/drivers/irqchip/irq-nvic.c > @@ -37,25 +37,13 @@ > > static struct irq_domain *nvic_irq_domain; > > -static void __nvic_handle_irq(irq_hw_number_t hwirq) > +static void __irq_entry nvic_handle_irq(struct pt_regs *regs) > { > - generic_handle_domain_irq(nvic_irq_domain, hwirq); > -} > + unsigned long icsr = readl_relaxed(BASEADDR_V7M_SCB + V7M_SCB_ICSR); > + irq_hw_number_t hwirq = (icsr & V7M_SCB_ICSR_VECTACTIVE) - 16; > > -/* > - * TODO: restructure the ARMv7M entry logic so that this entry logic can live > - * in arch code. > - */ > -asmlinkage void __exception_irq_entry > -nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > -{ > - struct pt_regs *old_regs; > > - irq_enter(); > - old_regs = set_irq_regs(regs); > - __nvic_handle_irq(hwirq); > - set_irq_regs(old_regs); > - irq_exit(); > + generic_handle_domain_irq(nvic_irq_domain, hwirq); > } > > static int nvic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > @@ -141,6 +129,7 @@ static int __init nvic_of_init(struct device_node *node, > for (i = 0; i < irqs; i += 4) > writel_relaxed(0, nvic_base + NVIC_IPR + i); > > + set_handle_irq(nvic_handle_irq); > return 0; > } > IRQCHIP_DECLARE(armv7m_nvic, "arm,armv7m-nvic", nvic_of_init); > > > Thanks, > > Mark. > > >
On Fri, 22 Oct 2021 18:58:54 +0100, Mark Rutland <mark.rutland@arm.com> wrote: > > On Fri, Oct 22, 2021 at 05:34:20PM +0100, Vladimir Murzin wrote: > > On 10/22/21 4:36 PM, Mark Rutland wrote: > > > On Fri, Oct 22, 2021 at 04:18:18PM +0100, Vladimir Murzin wrote: > > >> Hi Mark, > > >> > > >> On 10/21/21 7:02 PM, Mark Rutland wrote: > > >>> +/* > > >>> + * TODO: restructure the ARMv7M entry logic so that this entry logic can live > > >>> + * in arch code. > > >>> + */ > > >>> +asmlinkage void __exception_irq_entry > > >>> +static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > > >> > > >> I'm seeing build time failure... > > >> > > >> drivers/irqchip/irq-nvic.c:50:8: error: two or more data types in declaration specifiers > > >> static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > > >> ^~~~ > > >> drivers/irqchip/irq-nvic.c:50:13: warning: 'nvic_handle_irq' defined but not used [-Wunused-function] > > >> static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) > > >> > > >> I've fixed that locally and planing to give it a go... > > > > > > Ah, whoops. I've removed the extraneous `static void` from > > > nvic_handle_irq() and build tested that as part of stm32_defconfig. > > > > > > The updated version is in my irq/handle-domain-irq branch at: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git > > > > > > > $ cat /proc/interrupts > > CPU0 > > 16: 24 nvic_irq 4 Edge mps2-clkevt > > 17: 0 nvic_irq 32 Edge mps2-uart-rx > > 18: 6 nvic_irq 33 Edge mps2-uart-tx > > 19: 0 nvic_irq 47 Edge mps2-uart-overrun > > Err: 0 > > > > So if it helps feel free to add my > > > > Tested-by: Vladimir Murzin <vladimir.murzin@arm.com> # ARMv7M > > Thanks! > > I've folded that in and uppdated the branch. > > > As for TODO, is [1] look something you have been thinking of? IIUC, > > the show stopper is that hwirq is being passed from exception entry > > which retrieved via xPSR (IPSR to be precise). OTOH hwirq also available > > via Interrupt Controller Status Register (ICSR) thus can be used in > > driver itself... I gave [1] a go and it runs fine, yet I admit I might > > be missing something... > > I hadn't thought about it in much detail, but that looks good! > > I was wondering if we needed something like a > handle_arch_vectored_irq(), but if we can rely on the ICSR that seems > simpler overall. I'm not at all familiar with M-class, so I'm not sure > if there are pitfalls in this area. Why can't we just use IPSR instead from the C code? It has the potential of being of lower latency then a MMIO read (though I have no idea whether it makes a material difference on M-class) and from what I can see in the arch spec, they are strictly equivalent. Thanks, M.
On Thu, Oct 21, 2021 at 07:02:33PM +0100, Mark Rutland wrote: > In preparation for removing HANDLE_DOMAIN_IRQ_IRQENTRY, have > arch/openrisc perform all the irqentry accounting in its entry code. As > arch/openrisc uses GENERIC_IRQ_MULTI_HANDLER, we can use > generic_handle_arch_irq() to do so. > > There should be no functional change as a result of this patch. Reviewed-by: Stafford Horne <shorne@gmail.com>
On 10/22/21 7:43 PM, Marc Zyngier wrote: > On Fri, 22 Oct 2021 18:58:54 +0100, > Mark Rutland <mark.rutland@arm.com> wrote: >> >> On Fri, Oct 22, 2021 at 05:34:20PM +0100, Vladimir Murzin wrote: >>> On 10/22/21 4:36 PM, Mark Rutland wrote: >>>> On Fri, Oct 22, 2021 at 04:18:18PM +0100, Vladimir Murzin wrote: >>>>> Hi Mark, >>>>> >>>>> On 10/21/21 7:02 PM, Mark Rutland wrote: >>>>>> +/* >>>>>> + * TODO: restructure the ARMv7M entry logic so that this entry logic can live >>>>>> + * in arch code. >>>>>> + */ >>>>>> +asmlinkage void __exception_irq_entry >>>>>> +static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) >>>>> >>>>> I'm seeing build time failure... >>>>> >>>>> drivers/irqchip/irq-nvic.c:50:8: error: two or more data types in declaration specifiers >>>>> static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) >>>>> ^~~~ >>>>> drivers/irqchip/irq-nvic.c:50:13: warning: 'nvic_handle_irq' defined but not used [-Wunused-function] >>>>> static void nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs) >>>>> >>>>> I've fixed that locally and planing to give it a go... >>>> >>>> Ah, whoops. I've removed the extraneous `static void` from >>>> nvic_handle_irq() and build tested that as part of stm32_defconfig. >>>> >>>> The updated version is in my irq/handle-domain-irq branch at: >>>> >>>> git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git >>>> >>> >>> $ cat /proc/interrupts >>> CPU0 >>> 16: 24 nvic_irq 4 Edge mps2-clkevt >>> 17: 0 nvic_irq 32 Edge mps2-uart-rx >>> 18: 6 nvic_irq 33 Edge mps2-uart-tx >>> 19: 0 nvic_irq 47 Edge mps2-uart-overrun >>> Err: 0 >>> >>> So if it helps feel free to add my >>> >>> Tested-by: Vladimir Murzin <vladimir.murzin@arm.com> # ARMv7M >> >> Thanks! >> >> I've folded that in and uppdated the branch. >> >>> As for TODO, is [1] look something you have been thinking of? IIUC, >>> the show stopper is that hwirq is being passed from exception entry >>> which retrieved via xPSR (IPSR to be precise). OTOH hwirq also available >>> via Interrupt Controller Status Register (ICSR) thus can be used in >>> driver itself... I gave [1] a go and it runs fine, yet I admit I might >>> be missing something... >> >> I hadn't thought about it in much detail, but that looks good! >> >> I was wondering if we needed something like a >> handle_arch_vectored_irq(), but if we can rely on the ICSR that seems >> simpler overall. I'm not at all familiar with M-class, so I'm not sure >> if there are pitfalls in this area. > > Why can't we just use IPSR instead from the C code? It has the > potential of being of lower latency then a MMIO read (though I have no > idea whether it makes a material difference on M-class) and from what > I can see in the arch spec, they are strictly equivalent. Hmmm, less arch specific asm(s) in driver code, no? Cheers Vladimir > > Thanks, > > M. >
On Sat, 23 Oct 2021 13:06:25 +0100, Vladimir Murzin <vladimir.murzin@arm.com> wrote: > > On 10/22/21 7:43 PM, Marc Zyngier wrote: > > On Fri, 22 Oct 2021 18:58:54 +0100, > > Mark Rutland <mark.rutland@arm.com> wrote: > >> > >> On Fri, Oct 22, 2021 at 05:34:20PM +0100, Vladimir Murzin wrote: [...] > >>> As for TODO, is [1] look something you have been thinking of? IIUC, > >>> the show stopper is that hwirq is being passed from exception entry > >>> which retrieved via xPSR (IPSR to be precise). OTOH hwirq also available > >>> via Interrupt Controller Status Register (ICSR) thus can be used in > >>> driver itself... I gave [1] a go and it runs fine, yet I admit I might > >>> be missing something... > >> > >> I hadn't thought about it in much detail, but that looks good! > >> > >> I was wondering if we needed something like a > >> handle_arch_vectored_irq(), but if we can rely on the ICSR that seems > >> simpler overall. I'm not at all familiar with M-class, so I'm not sure > >> if there are pitfalls in this area. > > > > Why can't we just use IPSR instead from the C code? It has the > > potential of being of lower latency then a MMIO read (though I have no > > idea whether it makes a material difference on M-class) and from what > > I can see in the arch spec, they are strictly equivalent. > > Hmmm, less arch specific asm(s) in driver code, no? Well, it isn't like this driver is going to be useful on anything else, is it? If there is no overhead in reading from MMIO compared to the architected register, then I agree that ICSR is the way to go. Is there any chance you could measure it on a HW platform? Or maybe in emulation? Thanks, M.
On 10/23/21 2:18 PM, Marc Zyngier wrote: > On Sat, 23 Oct 2021 13:06:25 +0100, > Vladimir Murzin <vladimir.murzin@arm.com> wrote: >> >> On 10/22/21 7:43 PM, Marc Zyngier wrote: >>> On Fri, 22 Oct 2021 18:58:54 +0100, >>> Mark Rutland <mark.rutland@arm.com> wrote: >>>> >>>> On Fri, Oct 22, 2021 at 05:34:20PM +0100, Vladimir Murzin wrote: > > [...] > >>>>> As for TODO, is [1] look something you have been thinking of? IIUC, >>>>> the show stopper is that hwirq is being passed from exception entry >>>>> which retrieved via xPSR (IPSR to be precise). OTOH hwirq also available >>>>> via Interrupt Controller Status Register (ICSR) thus can be used in >>>>> driver itself... I gave [1] a go and it runs fine, yet I admit I might >>>>> be missing something... >>>> >>>> I hadn't thought about it in much detail, but that looks good! >>>> >>>> I was wondering if we needed something like a >>>> handle_arch_vectored_irq(), but if we can rely on the ICSR that seems >>>> simpler overall. I'm not at all familiar with M-class, so I'm not sure >>>> if there are pitfalls in this area. >>> >>> Why can't we just use IPSR instead from the C code? It has the >>> potential of being of lower latency then a MMIO read (though I have no >>> idea whether it makes a material difference on M-class) and from what >>> I can see in the arch spec, they are strictly equivalent. >> >> Hmmm, less arch specific asm(s) in driver code, no? > > Well, it isn't like this driver is going to be useful on anything > else, is it? > Well, with some work to unwire it from arch/arm it can be COMPILE_TEST :) > If there is no overhead in reading from MMIO compared to the > architected register, then I agree that ICSR is the way to > go. Is there any chance you could measure it on a HW platform? Or > maybe in emulation? My MPS{2,3} boards left in office and I'm on holiday next week... OTOH, I have no strong opinion on ICSR vs IPSR, I just wanted to check how much work it'd be to close TODO per my (quite limited) understanding :) Cheers Vladimir > > Thanks, > > M. >
On Fri, 22 Oct 2021 16:10:07 +0100, Mark Rutland <mark.rutland@arm.com> wrote: > > On Fri, Oct 22, 2021 at 12:20:31PM +0100, Marc Zyngier wrote: > > Hi Mark, > > > > On Thu, 21 Oct 2021 19:02:21 +0100, > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > > > The handle_domain_{irq,nmi}() functions were oringally intended as a > > > convenience, but recent rework to entry code across the kernel tree has > > > demonstrated that they cause more pain than they're worth and prevent > > > architectures from being able to write robust entry code. > > > > > > This series reworks the irq code to remove them, handling the necessary > > > entry work consistently in entry code (be it architectural or generic). > > > > [...] > > > > Thanks for going through the pain of putting this together. The > > couple of nits I mentioned notwithstanding: > > > > Reviewed-by: Marc Zyngier <maz@kernel.org> > > Thanks! > > I've pushed out an updated version to my irq/handle-domain-irq branch > on kernel.org: > > git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git > > That has two new patches you suggested: > > * irq: mips: simplify bcm6345_l1_irq_handle() > * irq: unexport handle_irq_desc() > > ... which I did not add your Reviewed-by to in case the commit messages > are garbage or something like that. I quickly eyeballed the patches, and they look OK to me. Feel free to add my RB tag to them. > > > It'd be good to work out a merging strategy once this has seen a bit > > of testing. > > Conflict-wise, this merges near perfectly against next-20212022 aside > from a trivial conflict against arch/riscv/Kconfig: > > | [mark@lakrids:~/src/linux]% git merge irq/handle-domain-irq > | Auto-merging arch/riscv/kernel/entry.S > | Auto-merging arch/riscv/Kconfig > | CONFLICT (content): Merge conflict in arch/riscv/Kconfig > | Auto-merging arch/nds32/Kconfig > | Auto-merging arch/mips/Kconfig > | Auto-merging arch/csky/Kconfig > | Auto-merging arch/arm64/Kconfig > | Auto-merging arch/arm/mach-s3c/irq-s3c24xx.c > | Auto-merging arch/arm/kernel/entry-armv.S > | Auto-merging arch/arm/Kconfig > | Auto-merging arch/arc/Kconfig > | Automatic merge failed; fix conflicts and then commit the result. > | [mark@lakrids:~/src/linux]% git diff > | diff --cc arch/riscv/Kconfig > | index 77a088d0a7e9,353e28f5f849..000000000000 > | --- a/arch/riscv/Kconfig > | +++ b/arch/riscv/Kconfig > | @@@ -62,8 -62,6 +62,11 @@@ config RISC > | select GENERIC_SCHED_CLOCK > | select GENERIC_SMP_IDLE_THREAD > | select GENERIC_TIME_VSYSCALL if MMU && 64BIT > | ++<<<<<<< HEAD > | + select GENERIC_VDSO_TIME_NS if HAVE_GENERIC_VDSO > | + select HANDLE_DOMAIN_IRQ > | ++======= > | ++>>>>>>> irq/handle-domain-irq > | select HAVE_ARCH_AUDITSYSCALL > | select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL > | select HAVE_ARCH_JUMP_LABEL_RELATIVE if !XIP_KERNEL > > ... where the resolution is: > > | diff --cc arch/riscv/Kconfig > | index 77a088d0a7e9,353e28f5f849..000000000000 > | --- a/arch/riscv/Kconfig > | +++ b/arch/riscv/Kconfig > | @@@ -62,8 -62,6 +62,7 @@@ config RISC > | select GENERIC_SCHED_CLOCK > | select GENERIC_SMP_IDLE_THREAD > | select GENERIC_TIME_VSYSCALL if MMU && 64BIT > | + select GENERIC_VDSO_TIME_NS if HAVE_GENERIC_VDSO > | - select HANDLE_DOMAIN_IRQ > | select HAVE_ARCH_AUDITSYSCALL > | select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL > | select HAVE_ARCH_JUMP_LABEL_RELATIVE if !XIP_KERNEL > > ... so I reckon we're not set for major pain there unless something new > appears in arch code in the next few days. > > If we can get this onto a branch for linux-next ASAP, and if Linus is > happy with this having come together a little late, maybe we could queue > this in tip for v5.16, perhaps after -rc1 to let this soak, or waiting > to apply the final patch to make it easier to revert the arch changes if > needed? I'm happy to route it via the irqchip tree (and ultimately tip) if nobody objects (which also means getting Acks from the arch maintainers). The branch would thus see a bit of -next before being sent to Linus. > I'd like to avoid sitting on this for an entire cycle if possible. +1. M.
On Fri, Oct 22, 2021 at 4:52 PM Mark Rutland <mark.rutland@arm.com> wrote: > > On Fri, Oct 22, 2021 at 10:33:53AM +0800, Guo Ren wrote: > > On Fri, Oct 22, 2021 at 2:03 AM Mark Rutland <mark.rutland@arm.com> wrote: > > > > > > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > > > handle_arch_irq() without performing any entry accounting. > > > > > > Add a generic wrapper to handle the commoon irqentry work when invoking > > Ah, I'd typo'd 'common' there; I'll go fix that... > > > > handle_arch_irq(). Where an architecture needs to perform some entry > > > accounting itself, it will need to invoke handle_arch_irq() itself. > > > > > > In subsequent patches it will become the responsibilty of the entry code > > > to set the irq regs when entering an IRQ (rather than deferring this to > > > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > > > regs now. This can be redundant in some cases, but is never harmful as > > > saving/restoring the old regs nests safely. > > Shall we remove old_regs save/restore in handle_domain_irq? It's duplicated. > > We do, in patch 15 when handle_domain_irq() is removed entirely. :) I miss that patch. Yes, in generic_handle_domain_nmi. > > Removing it immediately in this patch would require more ifdeffery and/or > another copy of handle_domain_irq(), and since the duplication doesn't cause a > functional problem, I think it's better to live with the temporary duplication > for the next few patches than to try to optimize the intermediate steps at the > cost of legibility. Thx for explaining. Reviewed-by: Guo Ren <guoren@kernel.org> > > Thanks, > Mark. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > Cc: Marc Zyngier <maz@kernel.org> > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > --- > > > kernel/irq/handle.c | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > > > index 221d80c31e94..27182003b879 100644 > > > --- a/kernel/irq/handle.c > > > +++ b/kernel/irq/handle.c > > > @@ -14,6 +14,8 @@ > > > #include <linux/interrupt.h> > > > #include <linux/kernel_stat.h> > > > > > > +#include <asm/irq_regs.h> > > > + > > > #include <trace/events/irq.h> > > > > > > #include "internals.h" > > > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > > > handle_arch_irq = handle_irq; > > > return 0; > > > } > > > + > > > +/** > > > + * generic_handle_arch_irq - root irq handler for architectures which do no > > > + * entry accounting themselves > > > + * @regs: Register file coming from the low-level handling code > > > + */ > > > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > > > +{ > > > + struct pt_regs *old_regs; > > > + > > > + irq_enter(); > > > + old_regs = set_irq_regs(regs); > > > + handle_arch_irq(regs); > > > + set_irq_regs(old_regs); > > > + irq_exit(); > > > +} > > > #endif > > > -- > > > 2.11.0 > > > > > > > > > -- > > Best Regards > > Guo Ren > > > > ML: https://lore.kernel.org/linux-csky/
On Thu, Oct 21, 2021 at 07:02:23PM +0100, Mark Rutland wrote: > On MIPS, the only user of handle_domain_irq() is octeon_irq_ciu3_ip2(), > which is called from the platform-specific plat_irq_dispatch() function > invoked from the early assembly code. > > No other irqchip relevant to arch/mips uses handle_domain_irq(): > > * No other plat_irq_dispatch() function transitively calls > handle_domain_irq(). > > * No other vectored IRQ dispatch function registered with > set_vi_handler() calls handle_domain_irq(). > > * No chained irqchip handlers call handle_domain_irq(), which makes > sense as this is meant to only be used by root irqchip handlers. > > Currently octeon_irq_ciu3_ip2() passes NULL as the `regs` argument to > handle_domain_irq(), and as handle_domain_irq() will pass this to > set_irq_regs(), any invoked IRQ handlers will erroneously see a NULL > pt_regs if they call get_pt_regs(). > > Fix this by calling generic_handle_domain_irq() directly, and performing > the necessary irq_{enter,exit}() logic directly in > octeon_irq_ciu3_ip2(). At the same time, deselect HANDLE_DOMAIN_IRQ, > which subsequent patches will remove. > > Other than the corrected behaviour of get_pt_regs(), there should be no > functional change as a result of this patch. > > Fixes: ce210d35bb93c2c5 ("MIPS: OCTEON: Add support for OCTEON III interrupt controller.") > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > arch/mips/Kconfig | 1 - > arch/mips/cavium-octeon/octeon-irq.c | 5 ++++- > 2 files changed, 4 insertions(+), 2 deletions(-) Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
On Thu, Oct 21, 2021 at 07:02:24PM +0100, Mark Rutland wrote: > There's no need fpr arch/mips's do_domain_IRQ() to open-code the NULL > check performed by handle_irq_desc(), nor the resolution of the desc > performed by generic_handle_domain_irq(). > > Use generic_handle_domain_irq() directly, as this is functioanlly > equivalent and clearer. > > There should be no functional change as a result of this patch. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > arch/mips/kernel/irq.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c > index d20e002b3246..1fee96ef8059 100644 > --- a/arch/mips/kernel/irq.c > +++ b/arch/mips/kernel/irq.c > @@ -115,11 +115,7 @@ void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq) > > irq_enter(); > check_stack_overflow(); > - > - desc = irq_resolve_mapping(domain, hwirq); > - if (likely(desc)) > - handle_irq_desc(desc); > - > + generic_handle_domain_irq(domain, hwirq); > irq_exit(); > } > #endif > -- > 2.11.0 Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
On Thu, Oct 21, 2021 at 07:02:22PM +0100, Mark Rutland wrote: > As bcm6345_l1_irq_handle() is a chained irqchip handler, it will be > invoked within the context of the root irqchip handler, which must have > entered IRQ context already. > > When bcm6345_l1_irq_handle() calls arch/mips's do_IRQ() , this will nest > another call to irq_enter(), and the resulting nested increment to > `rcu_data.dynticks_nmi_nesting` will cause rcu_is_cpu_rrupt_from_idle() > to fail to identify wakeups from idle, resulting in failure to preempt, > and RCU stalls. > > Chained irqchip handlers must invoke IRQ handlers by way of thee core > irqchip code, i.e. generic_handle_irq() or generic_handle_domain_irq() > and should not call do_IRQ(), which is intended only for root irqchip > handlers. > > Fix bcm6345_l1_irq_handle() by calling generic_handle_irq() directly. > > Fixes: c7c42ec2baa1de7a ("irqchips/bmips: Add bcm6345-l1 interrupt controller") > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > drivers/irqchip/irq-bcm6345-l1.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c > index e3483789f4df..1bd0621c4ce2 100644 > --- a/drivers/irqchip/irq-bcm6345-l1.c > +++ b/drivers/irqchip/irq-bcm6345-l1.c > @@ -140,7 +140,7 @@ static void bcm6345_l1_irq_handle(struct irq_desc *desc) > for_each_set_bit(hwirq, &pending, IRQS_PER_WORD) { > irq = irq_linear_revmap(intc->domain, base + hwirq); > if (irq) > - do_IRQ(irq); > + generic_handle_irq(irq); > else > spurious_interrupt(); > } > -- > 2.11.0 Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
On Thu, Oct 21, 2021 at 07:02:31PM +0100, Mark Rutland wrote: > In preparation for removing HANDLE_DOMAIN_IRQ_IRQENTRY, have arch/arm64 > perform all the irqentry accounting in its entry code. > > As arch/arm64 already performs portions of the irqentry logic in > enter_from_kernel_mode() and exit_to_kernel_mode(), including > rcu_irq_{enter,exit}(), the only additional calls that need to be made > are to irq_{enter,exit}_rcu(). Removing the calls to > rcu_irq_{enter,exit}() from handle_domain_irq() ensures that we inform > RCU once per IRQ entry and will correctly identify quiescent periods. > > Since we should not call irq_{enter,exit}_rcu() when entering a > pseudo-NMI, el1_interrupt() is reworked to have separate __el1_irq() and > __el1_pnmi() paths for regular IRQ and psuedo-NMI entry, with > irq_{enter,exit}_irq() only called for the former. > > In preparation for removing HANDLE_DOMAIN_IRQ, the irq regs are managed > in do_interrupt_handler() for both regular IRQ and pseudo-NMI. This is > currently redundant, but not harmful. > > For clarity the preemption logic is moved into __el1_irq(). We should > never preempt within a pseudo-NMI, and arm64_enter_nmi() already > enforces this by incrementing the preempt_count, but it's clearer if we > never invoke the preemption logic when entering a pseudo-NMI. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Will Deacon <will@kernel.org> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
On Thu, Oct 21, 2021 at 07:02:24PM +0100, Mark Rutland wrote: > There's no need fpr arch/mips's do_domain_IRQ() to open-code the NULL > check performed by handle_irq_desc(), nor the resolution of the desc > performed by generic_handle_domain_irq(). > > Use generic_handle_domain_irq() directly, as this is functioanlly > equivalent and clearer. > > There should be no functional change as a result of this patch. > Except for this compile error: arch/mips/kernel/irq.c: In function 'do_domain_IRQ': arch/mips/kernel/irq.c:114:26: error: unused variable 'desc' [-Werror=unused-variable] 114 | struct irq_desc *desc; Guenter > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > arch/mips/kernel/irq.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c > index d20e002b3246..1fee96ef8059 100644 > --- a/arch/mips/kernel/irq.c > +++ b/arch/mips/kernel/irq.c > @@ -115,11 +115,7 @@ void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq) > > irq_enter(); > check_stack_overflow(); > - > - desc = irq_resolve_mapping(domain, hwirq); > - if (likely(desc)) > - handle_irq_desc(desc); > - > + generic_handle_domain_irq(domain, hwirq); > irq_exit(); > } > #endif > -- > 2.11.0 >
On Thu, Oct 28, 2021 at 10:07:32AM -0700, Guenter Roeck wrote: > On Thu, Oct 21, 2021 at 07:02:24PM +0100, Mark Rutland wrote: > > There's no need fpr arch/mips's do_domain_IRQ() to open-code the NULL > > check performed by handle_irq_desc(), nor the resolution of the desc > > performed by generic_handle_domain_irq(). > > > > Use generic_handle_domain_irq() directly, as this is functioanlly > > equivalent and clearer. > > > > There should be no functional change as a result of this patch. > > > > Except for this compile error: > > arch/mips/kernel/irq.c: In function 'do_domain_IRQ': > arch/mips/kernel/irq.c:114:26: error: unused variable 'desc' [-Werror=unused-variable] > 114 | struct irq_desc *desc; > > Guenter Sorry for that; this has been fixed by: https://lore.kernel.org/r/20211028095652.3503790-1-siyanteng@loongson.cn ... which is queued up: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/remove-handle-domain-irq-20211026&id=34fca8947b2743e6a3a9a8a3a44962e625993533 Thanks, Mark. > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > --- > > arch/mips/kernel/irq.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c > > index d20e002b3246..1fee96ef8059 100644 > > --- a/arch/mips/kernel/irq.c > > +++ b/arch/mips/kernel/irq.c > > @@ -115,11 +115,7 @@ void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq) > > > > irq_enter(); > > check_stack_overflow(); > > - > > - desc = irq_resolve_mapping(domain, hwirq); > > - if (likely(desc)) > > - handle_irq_desc(desc); > > - > > + generic_handle_domain_irq(domain, hwirq); > > irq_exit(); > > } > > #endif > > -- > > 2.11.0 > >
On 10/23/21 2:36 PM, Vladimir Murzin wrote: > On 10/23/21 2:18 PM, Marc Zyngier wrote: >> On Sat, 23 Oct 2021 13:06:25 +0100, >> Vladimir Murzin <vladimir.murzin@arm.com> wrote: >>> >>> On 10/22/21 7:43 PM, Marc Zyngier wrote: >>>> On Fri, 22 Oct 2021 18:58:54 +0100, >>>> Mark Rutland <mark.rutland@arm.com> wrote: >>>>> >>>>> On Fri, Oct 22, 2021 at 05:34:20PM +0100, Vladimir Murzin wrote: >> >> [...] >> >>>>>> As for TODO, is [1] look something you have been thinking of? IIUC, >>>>>> the show stopper is that hwirq is being passed from exception entry >>>>>> which retrieved via xPSR (IPSR to be precise). OTOH hwirq also available >>>>>> via Interrupt Controller Status Register (ICSR) thus can be used in >>>>>> driver itself... I gave [1] a go and it runs fine, yet I admit I might >>>>>> be missing something... >>>>> >>>>> I hadn't thought about it in much detail, but that looks good! >>>>> >>>>> I was wondering if we needed something like a >>>>> handle_arch_vectored_irq(), but if we can rely on the ICSR that seems >>>>> simpler overall. I'm not at all familiar with M-class, so I'm not sure >>>>> if there are pitfalls in this area. >>>> >>>> Why can't we just use IPSR instead from the C code? It has the >>>> potential of being of lower latency then a MMIO read (though I have no >>>> idea whether it makes a material difference on M-class) and from what >>>> I can see in the arch spec, they are strictly equivalent. >>> >>> Hmmm, less arch specific asm(s) in driver code, no? >> >> Well, it isn't like this driver is going to be useful on anything >> else, is it? >> > > Well, with some work to unwire it from arch/arm it can be COMPILE_TEST :) > >> If there is no overhead in reading from MMIO compared to the >> architected register, then I agree that ICSR is the way to >> go. Is there any chance you could measure it on a HW platform? Or >> maybe in emulation? > > My MPS{2,3} boards left in office and I'm on holiday next week... OTOH, I > have no strong opinion on ICSR vs IPSR, I just wanted to check how much > work it'd be to close TODO per my (quite limited) understanding :) One month and a week later... I observe that in terms of performance MRS r0, ipsr is equivalent to readl_relaxed(BASEADDR_V7M_SCB + V7M_SCB_ICSR) MOV.W r3, #3758153728 LDR.W r0, [r3, #3332] Old compilers can produce less performant sequence like LDR r3,0xbcc0 ADD.W r3,r3,#0xaf00 LDR r0,[r3,#0] So, what would be your preference? Cheers Vladimir > > Cheers > Vladimir > > >> >> Thanks, >> >> M. >> >
On 2021-11-30 08:49, Vladimir Murzin wrote: > One month and a week later... > > I observe that in terms of performance > > MRS r0, ipsr > > is equivalent to readl_relaxed(BASEADDR_V7M_SCB + V7M_SCB_ICSR) > > MOV.W r3, #3758153728 > LDR.W r0, [r3, #3332] > > Old compilers can produce less performant sequence like > > LDR r3,0xbcc0 > ADD.W r3,r3,#0xaf00 > LDR r0,[r3,#0] > > So, what would be your preference? If there is no significant overhead to reading the MMIO register and that you see a benefit in enabling COMPILE_TEST, then this probably is the way to go. Thanks, M.
The handle_domain_{irq,nmi}() functions were oringally intended as a convenience, but recent rework to entry code across the kernel tree has demonstrated that they cause more pain than they're worth and prevent architectures from being able to write robust entry code. This series reworks the irq code to remove them, handling the necessary entry work consistently in entry code (be it architectural or generic). The gory details are in the thread starting at: https://lore.kernel.org/r/20211011104729.GB1421@C02TD0UTHF1T.local ... which can be summarized as: * At entry time, entry code needs to poke lockdep, RCU, and irqflag tracing in a specific order due to interdependencies, before most other C code can run. Part of this involves calling rcu_irq_enter(). Currently, arm64 is the only architecture which uses handle_domain_irq() and performs the nominally correct sequence (which is aligned with the generic entry code we aim to move to in future). * RCU relies on rcu_irq_enter() being called precisely once per IRQ exception, or rcu_is_cpu_rrupt_from_idle() may fail to identify wakeups from idle, and RCU may not trigger a reschedule when necessary, leading to RCU stalls. * The handle_domain_irq() helper, which is called from irqchip code between the entry code and interrupt handlers, calls rcu_irq_enter(), consequently causing problems for RCU on arm64. In the thread Linus decreed: I really think that if the rule is "we can't do accounting in handle_domain_irq(), because it's too late for arm64", then the fix really should be to just not do that. Move the irq_enter()/irq_exit() to the callers - quite possibly far up the call chain to the root of it all, and just say "architecture code needs to do this in the low-level code before calling handle_arch_irq". This series does so, making entry code responsible for the IRQ entry work, and removing the handle_domain_{irq,nmi}() functions entirely so we're not tempted to reintroduce similar problems in future. The rework fixes a couple of latent bugs for MIPS, fixes the problem originally diagnosed for arm64, and should make it easier for the other architectures using handle_domain_irq() to move to nominally correct entry sequencing (e.g. by moving to the generic entry code). I've added a couple of checks to generic_handle_irq() and generic_handle_domain_irq() to verify that architectures are correctly performing the required entry work (which fingers crossed, shouldn't fire). Other than the above, there should be no functional change as result of these changes (except that previously erroneous usage of RCU in irqchip code will have become correct by virtue of RCU starting to watch earlier). I've given the series some light build and boot testing so far. Thanks, Mark. Mark Rutland (15): irq: mips: avoid nested irq_enter() irq: mips: stop (ab)using handle_domain_irq() irq: mips: simplify do_domain_IRQ() irq: simplify handle_domain_{irq,nmi}() irq: add generic_handle_arch_irq() irq: arc: avoid CONFIG_HANDLE_DOMAIN_IRQ irq: nds32: avoid CONFIG_HANDLE_DOMAIN_IRQ irq: add a (temporary) CONFIG_HANDLE_DOMAIN_IRQ_IRQENTRY irq: arm: perform irqentry in entry code irq: arm64: perform irqentry in entry code irq: csky: perform irqentry in entry code irq: openrisc: perform irqentry in entry code irq: riscv: perform irqentry in entry code irq: remove CONFIG_HANDLE_DOMAIN_IRQ_IRQENTRY irq: remove handle_domain_{irq,nmi}() Documentation/core-api/irq/irq-domain.rst | 3 -- arch/arc/Kconfig | 1 - arch/arc/kernel/irq.c | 10 +++- arch/arm/Kconfig | 1 - arch/arm/kernel/entry-armv.S | 5 +- arch/arm/kernel/irq.c | 14 +++--- arch/arm/mach-imx/avic.c | 2 +- arch/arm/mach-imx/tzic.c | 2 +- arch/arm/mach-omap1/irq.c | 2 +- arch/arm/mach-s3c/irq-s3c24xx.c | 2 +- arch/arm64/Kconfig | 1 - arch/arm64/kernel/entry-common.c | 52 ++++++++++++-------- arch/csky/Kconfig | 1 - arch/csky/kernel/entry.S | 2 +- arch/csky/kernel/irq.c | 5 -- arch/mips/Kconfig | 1 - arch/mips/cavium-octeon/octeon-irq.c | 5 +- arch/mips/kernel/irq.c | 6 +-- arch/nds32/Kconfig | 1 - arch/openrisc/Kconfig | 1 - arch/openrisc/kernel/entry.S | 4 +- arch/openrisc/kernel/irq.c | 5 -- arch/riscv/Kconfig | 1 - arch/riscv/kernel/entry.S | 3 +- arch/riscv/kernel/smp.c | 9 +--- drivers/irqchip/irq-apple-aic.c | 20 ++++---- drivers/irqchip/irq-armada-370-xp.c | 13 ++--- drivers/irqchip/irq-aspeed-vic.c | 2 +- drivers/irqchip/irq-ativic32.c | 22 ++++++++- drivers/irqchip/irq-atmel-aic.c | 2 +- drivers/irqchip/irq-atmel-aic5.c | 2 +- drivers/irqchip/irq-bcm2835.c | 2 +- drivers/irqchip/irq-bcm2836.c | 2 +- drivers/irqchip/irq-bcm6345-l1.c | 2 +- drivers/irqchip/irq-clps711x.c | 8 +-- drivers/irqchip/irq-csky-apb-intc.c | 2 +- drivers/irqchip/irq-csky-mpintc.c | 4 +- drivers/irqchip/irq-davinci-aintc.c | 2 +- drivers/irqchip/irq-davinci-cp-intc.c | 2 +- drivers/irqchip/irq-digicolor.c | 2 +- drivers/irqchip/irq-dw-apb-ictl.c | 2 +- drivers/irqchip/irq-ftintc010.c | 2 +- drivers/irqchip/irq-gic-v3.c | 4 +- drivers/irqchip/irq-gic.c | 2 +- drivers/irqchip/irq-hip04.c | 2 +- drivers/irqchip/irq-ixp4xx.c | 4 +- drivers/irqchip/irq-lpc32xx.c | 2 +- drivers/irqchip/irq-mmp.c | 4 +- drivers/irqchip/irq-mxs.c | 2 +- drivers/irqchip/irq-nvic.c | 19 +++++++- drivers/irqchip/irq-omap-intc.c | 2 +- drivers/irqchip/irq-or1k-pic.c | 2 +- drivers/irqchip/irq-orion.c | 4 +- drivers/irqchip/irq-rda-intc.c | 2 +- drivers/irqchip/irq-riscv-intc.c | 2 +- drivers/irqchip/irq-sa11x0.c | 4 +- drivers/irqchip/irq-sun4i.c | 2 +- drivers/irqchip/irq-versatile-fpga.c | 2 +- drivers/irqchip/irq-vic.c | 2 +- drivers/irqchip/irq-vt8500.c | 2 +- drivers/irqchip/irq-wpcm450-aic.c | 2 +- drivers/irqchip/irq-zevio.c | 2 +- include/linux/irqdesc.h | 9 +--- kernel/irq/Kconfig | 3 -- kernel/irq/handle.c | 18 +++++++ kernel/irq/irqdesc.c | 81 +++++++------------------------ 66 files changed, 193 insertions(+), 215 deletions(-)