linux-kernel - Re: [PATCH 05/15] irq: add generic_handle_arch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Oct 2021 10:02:28 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Pingfan Liu <kernelfans@...il.com>
Cc:     linux-kernel@...r.kernel.org, aou@...s.berkeley.edu,
        catalin.marinas@....com, deanbo422@...il.com, green.hu@...il.com,
        guoren@...nel.org, jonas@...thpole.se,
        linux-arm-kernel@...ts.infradead.org, linux@...linux.org.uk,
        maz@...nel.org, nickhu@...estech.com, palmer@...belt.com,
        paulmck@...nel.org, paul.walmsley@...ive.com, peterz@...radead.org,
        shorne@...il.com, stefan.kristiansson@...nalahti.fi,
        tglx@...utronix.de, torvalds@...ux-foundation.org,
        tsbogend@...ha.franken.de, vgupta@...nel.org, will@...nel.org
Subject: Re: [PATCH 05/15] irq: add generic_handle_arch_irq()

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@....com>
> > Cc: Marc Zyngier <maz@...nel.org>
> > Cc: Thomas Gleixner <tglx@...utronix.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.