[PATCH 0/8] arm64: Support FIQ controller registration

[PATCH 0/8] arm64: Support FIQ controller registration

[Mark Rutland]

Hector's M1 support series [1] shows that some platforms have critical
interrupts wired to FIQ, and to support these platforms we need to support
handling FIQ exceptions. Other contemporary platforms don't use FIQ (since e.g.
this is usually routed to EL3), and as we never expect to take an FIQ, we have
the FIQ vector cause a panic.

Since the use of FIQ is a platform integration detail (which can differ across
bare-metal and virtualized environments), we need be able to explicitly opt-in
to handling FIQs while retaining the existing behaviour otherwise. This series
adds a new set_handle_fiq() hook so that the FIQ controller can do so, and
where no controller is registered the default handler will panic(). For
consistency the set_handle_irq() code is made to do the same.

The first couple of patches are from Marc's irq/drop-generic_irq_multi_handler
branch [2] on kernel.org, and clean up CONFIG_GENERIC_IRQ_MULTI_HANDLER usage.
The next four patches move arm64 over to a local set_handle_irq()
implementation, which is written to share code with a set_handle_fiq() function
in the last two patches. The only functional difference here is that if an IRQ
is somehow taken prior to set_handle_irq() the default handler will directly
panic() rather than the vector branching to NULL.

The penultimate patch is cherry-picked from the v2 M1 series, and as per
discussion there [3] will need a few additional fixups. I've included it for
now as the DAIF.IF alignment is necessary for the FIQ exception handling added
in the final patch.

The final patch adds the low-level FIQ exception handling and registration
mechanism atop the prior rework.

I'm hoping that we can somehow queue the first 6 patches of this series as a
base for the M1 support. With that we can either cherry-pick a later version of
the DAIF.IF patch here, or the M1 support series can take the FIQ handling
patch. I've pushed the series out to my arm64/fiq branch [4] on kernel.org,
atop v5.11.

Thanks,
Mark.

[1] https://http://lore.kernel.org/r/20210215121713.57687-1-marcan@marcan.st
[2] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/drop-generic_irq_multi_handler
[3] https://lore.kernelo.org/r/20210215121713.57687-9-marcan@marcan.st
[4] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/fiq

Hector Martin (1):
  arm64: Always keep DAIF.[IF] in sync

Marc Zyngier (5):
  ARM: ep93xx: Select GENERIC_IRQ_MULTI_HANDLER directly
  irqchip: Do not blindly select CONFIG_GENERIC_IRQ_MULTI_HANDLER
  genirq: Allow architectures to override set_handle_irq() fallback
  arm64: don't use GENERIC_IRQ_MULTI_HANDLER
  arm64: entry: factor irq triage logic into macros

Mark Rutland (2):
  arm64: irq: add a default handle_irq panic function
  arm64: irq: allow FIQs to be handled

 arch/arm/Kconfig                   |   1 +
 arch/arm64/Kconfig                 |   1 -
 arch/arm64/include/asm/assembler.h |   6 +--
 arch/arm64/include/asm/daifflags.h |   4 +-
 arch/arm64/include/asm/irq.h       |   4 ++
 arch/arm64/include/asm/irqflags.h  |  19 ++++---
 arch/arm64/kernel/entry.S          | 108 ++++++++++++++++++++++---------------
 arch/arm64/kernel/irq.c            |  33 +++++++++++-
 drivers/irqchip/Kconfig            |   9 ----
 include/linux/irq.h                |   2 +
 10 files changed, 121 insertions(+), 66 deletions(-)

-- 
2.11.0

[PATCH 1/8] ARM: ep93xx: Select GENERIC_IRQ_MULTI_HANDLER directly

[Mark Rutland]

From: Marc Zyngier <maz@kernel.org>

ep93xx currently relies of CONFIG_ARM_VIC to select
GENERIC_IRQ_MULTI_HANDLER. Given that this is logically a platform
architecture property, add the selection of GENERIC_IRQ_MULTI_HANDLER
at the platform level.

Further patches will remove the selection from the irqchip side.

Reported-by: Marc Rutland <mark.rutland@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Hector Martin <marcan@marcan.st>
Cc: James Morse <james.morse@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 138248999df7..8efa01363da3 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -348,6 +348,7 @@ config ARCH_EP93XX
 	select ARM_AMBA
 	imply ARM_PATCH_PHYS_VIRT
 	select ARM_VIC
+	select GENERIC_IRQ_MULTI_HANDLER
 	select AUTO_ZRELADDR
 	select CLKDEV_LOOKUP
 	select CLKSRC_MMIO
-- 
2.11.0

[PATCH 2/8] irqchip: Do not blindly select CONFIG_GENERIC_IRQ_MULTI_HANDLER

[Mark Rutland]

From: Marc Zyngier <maz@kernel.org>

Implementing CONFIG_GENERIC_IRQ_MULTI_HANDLER is a decision that is
made at the architecture level, and shouldn't involve the irqchip
at all (we even provide a fallback helper when the option isn't
selected).

Drop all instances of such selection from non-arch code.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20210217142800.2547737-1-maz@kernel.org
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Hector Martin <marcan@marcan.st>
Cc: James Morse <james.morse@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
---
 drivers/irqchip/Kconfig | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index b147f22a78f4..7b72df7a0761 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -8,7 +8,6 @@ config IRQCHIP
 config ARM_GIC
 	bool
 	select IRQ_DOMAIN_HIERARCHY
-	select GENERIC_IRQ_MULTI_HANDLER
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
 
 config ARM_GIC_PM
@@ -33,7 +32,6 @@ config GIC_NON_BANKED
 
 config ARM_GIC_V3
 	bool
-	select GENERIC_IRQ_MULTI_HANDLER
 	select IRQ_DOMAIN_HIERARCHY
 	select PARTITION_PERCPU
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
@@ -64,7 +62,6 @@ config ARM_NVIC
 config ARM_VIC
 	bool
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_MULTI_HANDLER
 
 config ARM_VIC_NR
 	int
@@ -99,14 +96,12 @@ config ATMEL_AIC_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_MULTI_HANDLER
 	select SPARSE_IRQ
 
 config ATMEL_AIC5_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_MULTI_HANDLER
 	select SPARSE_IRQ
 
 config I8259
@@ -153,7 +148,6 @@ config DW_APB_ICTL
 config FARADAY_FTINTC010
 	bool
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_MULTI_HANDLER
 	select SPARSE_IRQ
 
 config HISILICON_IRQ_MBIGEN
@@ -169,7 +163,6 @@ config IMGPDC_IRQ
 config IXP4XX_IRQ
 	bool
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_MULTI_HANDLER
 	select SPARSE_IRQ
 
 config MADERA_IRQ
@@ -186,7 +179,6 @@ config CLPS711X_IRQCHIP
 	bool
 	depends on ARCH_CLPS711X
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_MULTI_HANDLER
 	select SPARSE_IRQ
 	default y
 
@@ -205,7 +197,6 @@ config OMAP_IRQCHIP
 config ORION_IRQCHIP
 	bool
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_MULTI_HANDLER
 
 config PIC32_EVIC
 	bool
-- 
2.11.0

[PATCH 3/8] genirq: Allow architectures to override set_handle_irq() fallback

[Mark Rutland]

From: Marc Zyngier <maz@kernel.org>

Some architectures want to provide the generic set_handle_irq() API, but
for structural reasons need to provide their own implementation. For
example, arm64 needs to do this to provide unifoprm set_handle_irq() and
set_handle_fiq() registration functions.

Make this possible by allowing architectures to provide their own
implementation of set_handle_irq when CONFIG_GENERIC_IRQ_MULTI_HANDLER
is not selected.

Signed-off-by: Marc Zyngier <maz@kernel.org>
[Mark: expand commit message]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Hector Martin <marcan@marcan.st>
Cc: James Morse <james.morse@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
---
 include/linux/irq.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 2efde6a79b7e..9890180b84fd 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -1258,11 +1258,13 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *));
  */
 extern void (*handle_arch_irq)(struct pt_regs *) __ro_after_init;
 #else
+#ifndef set_handle_irq
 #define set_handle_irq(handle_irq)		\
 	do {					\
 		(void)handle_irq;		\
 		WARN_ON(1);			\
 	} while (0)
 #endif
+#endif
 
 #endif /* _LINUX_IRQ_H */
-- 
2.11.0

[PATCH 4/8] arm64: don't use GENERIC_IRQ_MULTI_HANDLER

[Mark Rutland]

From: Marc Zyngier <maz@kernel.org>

In subsequent patches we want to allow irqchip drivers to register as
FIQ handlers, with a set_handle_fiq() function. To keep the IRQ/FIQ
paths similar, we want arm64 to provide both set_handle_irq() and
set_handle_fiq(), rather than using GENERIC_IRQ_MULTI_HANDLER for the
former.

This patch adds an arm64-specific implementation of set_handle_irq().
There should be no functional change as a result of this patch.

Signed-off-by: Marc Zyngier <maz@kernel.org>
[Mark: use a single handler pointer]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Hector Martin <marcan@marcan.st>
Cc: James Morse <james.morse@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/Kconfig           |  1 -
 arch/arm64/include/asm/irq.h |  3 +++
 arch/arm64/kernel/irq.c      | 11 +++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index f39568b28ec1..6094214df91b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -108,7 +108,6 @@ config ARM64
 	select GENERIC_EARLY_IOREMAP
 	select GENERIC_IDLE_POLL_SETUP
 	select GENERIC_IRQ_IPI
-	select GENERIC_IRQ_MULTI_HANDLER
 	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_SHOW
 	select GENERIC_IRQ_SHOW_LEVEL
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index b2b0c6405eb0..8391c6f6f746 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -8,6 +8,9 @@
 
 struct pt_regs;
 
+int set_handle_irq(void (*handle_irq)(struct pt_regs *));
+#define set_handle_irq	set_handle_irq
+
 static inline int nr_legacy_irqs(void)
 {
 	return 0;
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index dfb1feab867d..ad63bd50fa7b 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -71,6 +71,17 @@ static void init_irq_stacks(void)
 }
 #endif
 
+void (*handle_arch_irq)(struct pt_regs *) __ro_after_init;
+
+int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
+{
+	if (handle_arch_irq)
+		return -EBUSY;
+
+	handle_arch_irq = handle_irq;
+	return 0;
+}
+
 void __init init_IRQ(void)
 {
 	init_irq_stacks();
-- 
2.11.0

[PATCH 5/8] arm64: irq: add a default handle_irq panic function

[Mark Rutland]

If we accidentally unmask IRQs before we've registered an IRQ
controller, handle_arch_irq will be NULL, and the IRQ exception handler
will branch to a bogus address.

To make this easier to debug, this patch initialises handle_arch_irq to
a default handler which will panic(), making such problems easier to
debug. When we add support for FIQ handlers, we can follow the same
approach.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Hector Martin <marcan@marcan.st>
Cc: James Morse <james.morse@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/irq.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index ad63bd50fa7b..00bcf37aa0ea 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -71,11 +71,16 @@ static void init_irq_stacks(void)
 }
 #endif
 
-void (*handle_arch_irq)(struct pt_regs *) __ro_after_init;
+void default_handle_irq(struct pt_regs *regs)
+{
+	panic("IRQ taken without a registered IRQ controller\n");
+}
+
+void (*handle_arch_irq)(struct pt_regs *) __ro_after_init = default_handle_irq;
 
 int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 {
-	if (handle_arch_irq)
+	if (handle_arch_irq != default_handle_irq)
 		return -EBUSY;
 
 	handle_arch_irq = handle_irq;
@@ -87,7 +92,7 @@ void __init init_IRQ(void)
 	init_irq_stacks();
 	init_irq_scs();
 	irqchip_init();
-	if (!handle_arch_irq)
+	if (handle_arch_irq == default_handle_irq)
 		panic("No interrupt controller found.");
 
 	if (system_uses_irq_prio_masking()) {
-- 
2.11.0

Re: [PATCH 5/8] arm64: irq: add a default handle_irq panic function

[Mark Rutland]

On Fri, Feb 19, 2021 at 11:39:01AM +0000, Mark Rutland wrote:
> If we accidentally unmask IRQs before we've registered an IRQ
> controller, handle_arch_irq will be NULL, and the IRQ exception handler
> will branch to a bogus address.
> 
> To make this easier to debug, this patch initialises handle_arch_irq to
> a default handler which will panic(), making such problems easier to
> debug. When we add support for FIQ handlers, we can follow the same
> approach.

> -void (*handle_arch_irq)(struct pt_regs *) __ro_after_init;
> +void default_handle_irq(struct pt_regs *regs)
> +{
> +	panic("IRQ taken without a registered IRQ controller\n");
> +}

The kbuild test robot pointed out that this should be static (likewise
with default_handle_fiq in patch 8), since it's only used within this
file, so I've updated that in my branch.

Mark.

> +
> +void (*handle_arch_irq)(struct pt_regs *) __ro_after_init = default_handle_irq;
>  
>  int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
>  {
> -	if (handle_arch_irq)
> +	if (handle_arch_irq != default_handle_irq)
>  		return -EBUSY;
>  
>  	handle_arch_irq = handle_irq;
> @@ -87,7 +92,7 @@ void __init init_IRQ(void)
>  	init_irq_stacks();
>  	init_irq_scs();
>  	irqchip_init();
> -	if (!handle_arch_irq)
> +	if (handle_arch_irq == default_handle_irq)
>  		panic("No interrupt controller found.");
>  
>  	if (system_uses_irq_prio_masking()) {
> -- 
> 2.11.0
> 

Re: [PATCH 5/8] arm64: irq: add a default handle_irq panic function

[Marc Zyngier]

On 2021-02-22 09:59, Mark Rutland wrote:
> On Fri, Feb 19, 2021 at 11:39:01AM +0000, Mark Rutland wrote:
>> If we accidentally unmask IRQs before we've registered an IRQ
>> controller, handle_arch_irq will be NULL, and the IRQ exception 
>> handler
>> will branch to a bogus address.
>> 
>> To make this easier to debug, this patch initialises handle_arch_irq 
>> to
>> a default handler which will panic(), making such problems easier to
>> debug. When we add support for FIQ handlers, we can follow the same
>> approach.
> 
>> -void (*handle_arch_irq)(struct pt_regs *) __ro_after_init;
>> +void default_handle_irq(struct pt_regs *regs)
>> +{
>> +	panic("IRQ taken without a registered IRQ controller\n");
>> +}
> 
> The kbuild test robot pointed out that this should be static (likewise
> with default_handle_fiq in patch 8), since it's only used within this
> file, so I've updated that in my branch.
> 
> Mark.
> 
>> +
>> +void (*handle_arch_irq)(struct pt_regs *) __ro_after_init = 
>> default_handle_irq;
>> 
>>  int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
>>  {
>> -	if (handle_arch_irq)
>> +	if (handle_arch_irq != default_handle_irq)
>>  		return -EBUSY;
>> 
>>  	handle_arch_irq = handle_irq;
>> @@ -87,7 +92,7 @@ void __init init_IRQ(void)
>>  	init_irq_stacks();
>>  	init_irq_scs();
>>  	irqchip_init();
>> -	if (!handle_arch_irq)
>> +	if (handle_arch_irq == default_handle_irq)
>>  		panic("No interrupt controller found.");

It also seems odd to have both default_handle_irq() that panics,
and init_IRQ that panics as well. Not a big deal, but maybe
we should just drop this altogether and get the firework on the
first interrupt.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 5/8] arm64: irq: add a default handle_irq panic function

[Mark Rutland]

On Mon, Feb 22, 2021 at 10:48:11AM +0000, Marc Zyngier wrote:
> On 2021-02-22 09:59, Mark Rutland wrote:
> > On Fri, Feb 19, 2021 at 11:39:01AM +0000, Mark Rutland wrote:
> > > +void (*handle_arch_irq)(struct pt_regs *) __ro_after_init =
> > > default_handle_irq;
> > > 
> > >  int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
> > >  {
> > > -	if (handle_arch_irq)
> > > +	if (handle_arch_irq != default_handle_irq)
> > >  		return -EBUSY;
> > > 
> > >  	handle_arch_irq = handle_irq;
> > > @@ -87,7 +92,7 @@ void __init init_IRQ(void)
> > >  	init_irq_stacks();
> > >  	init_irq_scs();
> > >  	irqchip_init();
> > > -	if (!handle_arch_irq)
> > > +	if (handle_arch_irq == default_handle_irq)
> > >  		panic("No interrupt controller found.");
> 
> It also seems odd to have both default_handle_irq() that panics,
> and init_IRQ that panics as well. Not a big deal, but maybe
> we should just drop this altogether and get the firework on the
> first interrupt.

My gut feeling was that both were useful, and served slightly different
cases:

* The panic in default_handle_irq() helps if we unexpectedly unmask IRQ
  too early. This is mostly a nicety over the current behaviour of
  branching to NULL in this case.

* The panic in init_IRQ() gives us a consistent point at which we can
  note the absence of a root IRQ controller even if all IRQs are
  quiescent. This is a bit nicer to debug than seeing a load of driver
  probes fail their request_irq() or whatever.

... so I'd err on the side of keeping both, but if you think otherwise
I'm happy to change this.

Thanks,
Mark.

Re: [PATCH 5/8] arm64: irq: add a default handle_irq panic function

[Marc Zyngier]

On 2021-02-22 11:25, Mark Rutland wrote:
> On Mon, Feb 22, 2021 at 10:48:11AM +0000, Marc Zyngier wrote:
>> On 2021-02-22 09:59, Mark Rutland wrote:
>> > On Fri, Feb 19, 2021 at 11:39:01AM +0000, Mark Rutland wrote:
>> > > +void (*handle_arch_irq)(struct pt_regs *) __ro_after_init =
>> > > default_handle_irq;
>> > >
>> > >  int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
>> > >  {
>> > > -	if (handle_arch_irq)
>> > > +	if (handle_arch_irq != default_handle_irq)
>> > >  		return -EBUSY;
>> > >
>> > >  	handle_arch_irq = handle_irq;
>> > > @@ -87,7 +92,7 @@ void __init init_IRQ(void)
>> > >  	init_irq_stacks();
>> > >  	init_irq_scs();
>> > >  	irqchip_init();
>> > > -	if (!handle_arch_irq)
>> > > +	if (handle_arch_irq == default_handle_irq)
>> > >  		panic("No interrupt controller found.");
>> 
>> It also seems odd to have both default_handle_irq() that panics,
>> and init_IRQ that panics as well. Not a big deal, but maybe
>> we should just drop this altogether and get the firework on the
>> first interrupt.
> 
> My gut feeling was that both were useful, and served slightly different
> cases:
> 
> * The panic in default_handle_irq() helps if we unexpectedly unmask IRQ
>   too early. This is mostly a nicety over the current behaviour of
>   branching to NULL in this case.
> 
> * The panic in init_IRQ() gives us a consistent point at which we can
>   note the absence of a root IRQ controller even if all IRQs are
>   quiescent. This is a bit nicer to debug than seeing a load of driver
>   probes fail their request_irq() or whatever.
> 
> ... so I'd err on the side of keeping both, but if you think otherwise
> I'm happy to change this.

As I said, it's not a big deal. I doubt that we'll see 
default_handle_irq()
exploding in practice. But the real nit here is the difference of 
treatment
between IRQ and FIQ. *IF* we ever get a system that only signals its
interrupt as FIQ (and I don't see why we'd forbid that), then we would

To be clear, I don't think we should care too much either way, and I'm
fine with the code as is.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 5/8] arm64: irq: add a default handle_irq panic function

[Mark Rutland]

On Mon, Feb 22, 2021 at 11:43:13AM +0000, Marc Zyngier wrote:
> On 2021-02-22 11:25, Mark Rutland wrote:
> > On Mon, Feb 22, 2021 at 10:48:11AM +0000, Marc Zyngier wrote:
> > > On 2021-02-22 09:59, Mark Rutland wrote:
> > > > On Fri, Feb 19, 2021 at 11:39:01AM +0000, Mark Rutland wrote:
> > > > > +void (*handle_arch_irq)(struct pt_regs *) __ro_after_init =
> > > > > default_handle_irq;
> > > > >
> > > > >  int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
> > > > >  {
> > > > > -	if (handle_arch_irq)
> > > > > +	if (handle_arch_irq != default_handle_irq)
> > > > >  		return -EBUSY;
> > > > >
> > > > >  	handle_arch_irq = handle_irq;
> > > > > @@ -87,7 +92,7 @@ void __init init_IRQ(void)
> > > > >  	init_irq_stacks();
> > > > >  	init_irq_scs();
> > > > >  	irqchip_init();
> > > > > -	if (!handle_arch_irq)
> > > > > +	if (handle_arch_irq == default_handle_irq)
> > > > >  		panic("No interrupt controller found.");
> > > 
> > > It also seems odd to have both default_handle_irq() that panics,
> > > and init_IRQ that panics as well. Not a big deal, but maybe
> > > we should just drop this altogether and get the firework on the
> > > first interrupt.
> > 
> > My gut feeling was that both were useful, and served slightly different
> > cases:
> > 
> > * The panic in default_handle_irq() helps if we unexpectedly unmask IRQ
> >   too early. This is mostly a nicety over the current behaviour of
> >   branching to NULL in this case.
> > 
> > * The panic in init_IRQ() gives us a consistent point at which we can
> >   note the absence of a root IRQ controller even if all IRQs are
> >   quiescent. This is a bit nicer to debug than seeing a load of driver
> >   probes fail their request_irq() or whatever.
> > 
> > ... so I'd err on the side of keeping both, but if you think otherwise
> > I'm happy to change this.
> 
> As I said, it's not a big deal. I doubt that we'll see default_handle_irq()
> exploding in practice. But the real nit here is the difference of treatment
> between IRQ and FIQ. *IF* we ever get a system that only signals its
> interrupt as FIQ (and I don't see why we'd forbid that), then we would

That's a fair point.

For consistency, we could remove the init_IRQ() panic() and instead log
the registered handlers, e.g.

| pr_info("Root IRQ handler is %ps\n", handle_arch_irq);
| pr_info("Root FIQ handler is %ps\n", handle_arch_fiq);

... or do that inside the set_handle_{irq,fiq}() functions. That way the
messages (or absence thereof) would be sufficient to diagnose the lack
of a root IRQ/FIQ handler when IRQ/FIQ happens to be quiescent.

Does that sound any better?

> To be clear, I don't think we should care too much either way, and I'm
> fine with the code as is.

Sure, and FWIW I agree with the nit!

Thanks,
Mark.

Re: [PATCH 5/8] arm64: irq: add a default handle_irq panic function

[Marc Zyngier]

On Mon, 22 Feb 2021 12:06:14 +0000,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Mon, Feb 22, 2021 at 11:43:13AM +0000, Marc Zyngier wrote:

[...]

> > As I said, it's not a big deal. I doubt that we'll see default_handle_irq()
> > exploding in practice. But the real nit here is the difference of treatment
> > between IRQ and FIQ. *IF* we ever get a system that only signals its
> > interrupt as FIQ (and I don't see why we'd forbid that), then we would
> 
> That's a fair point.
> 
> For consistency, we could remove the init_IRQ() panic() and instead log
> the registered handlers, e.g.
> 
> | pr_info("Root IRQ handler is %ps\n", handle_arch_irq);
> | pr_info("Root FIQ handler is %ps\n", handle_arch_fiq);
> 
> ... or do that inside the set_handle_{irq,fiq}() functions. That way the
> messages (or absence thereof) would be sufficient to diagnose the lack
> of a root IRQ/FIQ handler when IRQ/FIQ happens to be quiescent.
> 
> Does that sound any better?

Yup, I quite like the second variant (using set_handle_{irq,fiq}()).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH 6/8] arm64: entry: factor irq triage logic into macros

[Mark Rutland]

From: Marc Zyngier <maz@kernel.org>

In subsequent patches we'll allow an FIQ handler to be registered, and
FIQ exceptions will need to be triaged very similarly to IRQ exceptions.
So that we can reuse the existing logic, this patch factors the IRQ
triage logic out into macros that can be reused for FIQ.

The macros are named to follow the elX_foo_handler scheme used by the C
exception handlers. For consistency with other top-level exception
handlers, the kernel_entry/kernel_exit logic is not moved into the
macros. As FIQ will use a different C handler, this handler name is
provided as an argument to the macros.

There should be no functional change as a result of this patch.

Signed-off-by: Marc Zyngier <maz@kernel.org>
[Mark: rework macros, commit message, rebase before DAIF rework]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Hector Martin <marcan@marcan.st>
Cc: James Morse <james.morse@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/entry.S | 80 +++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index c9bae73f2621..acc677672277 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -491,8 +491,8 @@ tsk	.req	x28		// current thread_info
 /*
  * Interrupt handling.
  */
-	.macro	irq_handler
-	ldr_l	x1, handle_arch_irq
+	.macro	irq_handler, handler:req
+	ldr_l	x1, \handler
 	mov	x0, sp
 	irq_stack_entry
 	blr	x1
@@ -531,6 +531,45 @@ alternative_endif
 #endif
 	.endm
 
+	.macro el1_interrupt_handler, handler:req
+	gic_prio_irq_setup pmr=x20, tmp=x1
+	enable_da_f
+
+	mov	x0, sp
+	bl	enter_el1_irq_or_nmi
+
+	irq_handler	\handler
+
+#ifdef CONFIG_PREEMPTION
+	ldr	x24, [tsk, #TSK_TI_PREEMPT]	// get preempt count
+alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+	/*
+	 * DA_F were cleared at start of handling. If anything is set in DAIF,
+	 * we come back from an NMI, so skip preemption
+	 */
+	mrs	x0, daif
+	orr	x24, x24, x0
+alternative_else_nop_endif
+	cbnz	x24, 1f				// preempt count != 0 || NMI return path
+	bl	arm64_preempt_schedule_irq	// irq en/disable is done inside
+1:
+#endif
+
+	mov	x0, sp
+	bl	exit_el1_irq_or_nmi
+	.endm
+
+	.macro el0_interrupt_handler, handler:req
+	gic_prio_irq_setup pmr=x20, tmp=x0
+	user_exit_irqoff
+	enable_da_f
+
+	tbz	x22, #55, 1f
+	bl	do_el0_irq_bp_hardening
+1:
+	irq_handler	\handler
+	.endm
+
 	.text
 
 /*
@@ -660,32 +699,7 @@ SYM_CODE_END(el1_sync)
 	.align	6
 SYM_CODE_START_LOCAL_NOALIGN(el1_irq)
 	kernel_entry 1
-	gic_prio_irq_setup pmr=x20, tmp=x1
-	enable_da_f
-
-	mov	x0, sp
-	bl	enter_el1_irq_or_nmi
-
-	irq_handler
-
-#ifdef CONFIG_PREEMPTION
-	ldr	x24, [tsk, #TSK_TI_PREEMPT]	// get preempt count
-alternative_if ARM64_HAS_IRQ_PRIO_MASKING
-	/*
-	 * DA_F were cleared at start of handling. If anything is set in DAIF,
-	 * we come back from an NMI, so skip preemption
-	 */
-	mrs	x0, daif
-	orr	x24, x24, x0
-alternative_else_nop_endif
-	cbnz	x24, 1f				// preempt count != 0 || NMI return path
-	bl	arm64_preempt_schedule_irq	// irq en/disable is done inside
-1:
-#endif
-
-	mov	x0, sp
-	bl	exit_el1_irq_or_nmi
-
+	el1_interrupt_handler handle_arch_irq
 	kernel_exit 1
 SYM_CODE_END(el1_irq)
 
@@ -725,15 +739,7 @@ SYM_CODE_END(el0_error_compat)
 SYM_CODE_START_LOCAL_NOALIGN(el0_irq)
 	kernel_entry 0
 el0_irq_naked:
-	gic_prio_irq_setup pmr=x20, tmp=x0
-	user_exit_irqoff
-	enable_da_f
-
-	tbz	x22, #55, 1f
-	bl	do_el0_irq_bp_hardening
-1:
-	irq_handler
-
+	el0_interrupt_handler handle_arch_irq
 	b	ret_to_user
 SYM_CODE_END(el0_irq)
 
-- 
2.11.0

[PATCH 7/8] arm64: Always keep DAIF.[IF] in sync

[Mark Rutland]

From: Hector Martin <marcan@marcan.st>

Apple SoCs (A11 and newer) have some interrupt sources hardwired to the
FIQ line. We implement support for this by simply treating IRQs and FIQs
the same way in the interrupt vectors.

To support these systems, the FIQ mask bit needs to be kept in sync with
the IRQ mask bit, so both kinds of exceptions are masked together. No
other platforms should be delivering FIQ exceptions right now, and we
already unmask FIQ in normal process context, so this should not have an
effect on other systems - if spurious FIQs were arriving, they would
already panic the kernel.

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/assembler.h |  6 +++---
 arch/arm64/include/asm/daifflags.h |  4 ++--
 arch/arm64/include/asm/irqflags.h  | 19 +++++++++++--------
 arch/arm64/kernel/entry.S          |  8 ++++----
 4 files changed, 20 insertions(+), 17 deletions(-)

Note: as per discussion on v2 [1], there are a few additional fixups necessary;
and so we should not queue this version of the patch. I've included it for now
as it is a prerequisite for the last patch.

[1] https://lore.kernel.org/r/20210215121713.57687-9-marcan@marcan.st

Mark.

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index bf125c591116..ac4c823bf2b6 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -40,9 +40,9 @@
 	msr	daif, \flags
 	.endm
 
-	/* IRQ is the lowest priority flag, unconditionally unmask the rest. */
-	.macro enable_da_f
-	msr	daifclr, #(8 | 4 | 1)
+	/* IRQ/FIQ are the lowest priority flags, unconditionally unmask the rest. */
+	.macro enable_da
+	msr	daifclr, #(8 | 4)
 	.endm
 
 /*
diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index 1c26d7baa67f..9d1d4ab98585 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -13,8 +13,8 @@
 #include <asm/ptrace.h>
 
 #define DAIF_PROCCTX		0
-#define DAIF_PROCCTX_NOIRQ	PSR_I_BIT
-#define DAIF_ERRCTX		(PSR_I_BIT | PSR_A_BIT)
+#define DAIF_PROCCTX_NOIRQ	(PSR_I_BIT | PSR_F_BIT)
+#define DAIF_ERRCTX		(PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
 #define DAIF_MASK		(PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
 
 
diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index ff328e5bbb75..125201dced5f 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -12,15 +12,18 @@
 
 /*
  * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and
- * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'dai'
+ * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'daif'
  * order:
  * Masking debug exceptions causes all other exceptions to be masked too/
- * Masking SError masks irq, but not debug exceptions. Masking irqs has no
- * side effects for other flags. Keeping to this order makes it easier for
- * entry.S to know which exceptions should be unmasked.
+ * Masking SError masks IRQ/FIQ, but not debug exceptions. IRQ and FIQ are
+ * always masked and unmasked together, and have no side effects for other
+ * flags. Keeping to this order makes it easier for entry.S to know which
+ * exceptions should be unmasked.
  *
- * FIQ is never expected, but we mask it when we disable debug exceptions, and
- * unmask it at all other times.
+ * FIQ is never expected on most platforms, but we keep it synchronized
+ * with the IRQ mask status. On platforms that do not expect FIQ, that vector
+ * triggers a kernel panic. On platforms that do, the FIQ vector is unified
+ * with the IRQ vector.
  */
 
 /*
@@ -35,7 +38,7 @@ static inline void arch_local_irq_enable(void)
 	}
 
 	asm volatile(ALTERNATIVE(
-		"msr	daifclr, #2		// arch_local_irq_enable",
+		"msr	daifclr, #3		// arch_local_irq_enable",
 		__msr_s(SYS_ICC_PMR_EL1, "%0"),
 		ARM64_HAS_IRQ_PRIO_MASKING)
 		:
@@ -54,7 +57,7 @@ static inline void arch_local_irq_disable(void)
 	}
 
 	asm volatile(ALTERNATIVE(
-		"msr	daifset, #2		// arch_local_irq_disable",
+		"msr	daifset, #3		// arch_local_irq_disable",
 		__msr_s(SYS_ICC_PMR_EL1, "%0"),
 		ARM64_HAS_IRQ_PRIO_MASKING)
 		:
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index acc677672277..0474cca9f1a9 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -533,7 +533,7 @@ alternative_endif
 
 	.macro el1_interrupt_handler, handler:req
 	gic_prio_irq_setup pmr=x20, tmp=x1
-	enable_da_f
+	enable_da
 
 	mov	x0, sp
 	bl	enter_el1_irq_or_nmi
@@ -544,7 +544,7 @@ alternative_endif
 	ldr	x24, [tsk, #TSK_TI_PREEMPT]	// get preempt count
 alternative_if ARM64_HAS_IRQ_PRIO_MASKING
 	/*
-	 * DA_F were cleared at start of handling. If anything is set in DAIF,
+	 * DA were cleared at start of handling. If anything is set in DAIF,
 	 * we come back from an NMI, so skip preemption
 	 */
 	mrs	x0, daif
@@ -562,7 +562,7 @@ alternative_else_nop_endif
 	.macro el0_interrupt_handler, handler:req
 	gic_prio_irq_setup pmr=x20, tmp=x0
 	user_exit_irqoff
-	enable_da_f
+	enable_da
 
 	tbz	x22, #55, 1f
 	bl	do_el0_irq_bp_hardening
@@ -763,7 +763,7 @@ el0_error_naked:
 	mov	x0, sp
 	mov	x1, x25
 	bl	do_serror
-	enable_da_f
+	enable_da
 	b	ret_to_user
 SYM_CODE_END(el0_error)
 
-- 
2.11.0

[PATCH 7/8 v1.5] arm64: Always keep DAIF.[IF] in sync

[Hector Martin]

Apple SoCs (A11 and newer) have some interrupt sources hardwired to the
FIQ line. We implement support for this by simply treating IRQs and FIQs
the same way in the interrupt vectors.

To support these systems, the FIQ mask bit needs to be kept in sync with
the IRQ mask bit, so both kinds of exceptions are masked together. No
other platforms should be delivering FIQ exceptions right now, and we
already unmask FIQ in normal process context, so this should not have an
effect on other systems - if spurious FIQs were arriving, they would
already panic the kernel.

Signed-off-by: Hector Martin <marcan@marcan.st>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>

---
 arch/arm64/include/asm/arch_gicv3.h |  2 +-
 arch/arm64/include/asm/assembler.h  |  8 ++++----
 arch/arm64/include/asm/daifflags.h  | 10 +++++-----
 arch/arm64/include/asm/irqflags.h   | 16 +++++++---------
 arch/arm64/kernel/entry.S           | 12 +++++++-----
 arch/arm64/kernel/process.c         |  2 +-
 arch/arm64/kernel/smp.c             |  1 +
 7 files changed, 26 insertions(+), 25 deletions(-)

This is the updated patch after addressing the comments in the original
v2 review; we're moving it to this series now, so please review it in
this context.

diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index 880b9054d75c..934b9be582d2 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -173,7 +173,7 @@ static inline void gic_pmr_mask_irqs(void)

 static inline void gic_arch_enable_irqs(void)
 {
-	asm volatile ("msr daifclr, #2" : : : "memory");
+	asm volatile ("msr daifclr, #3" : : : "memory");
 }

 #endif /* __ASSEMBLY__ */
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index bf125c591116..53ff8c71eed7 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -40,9 +40,9 @@
 	msr	daif, \flags
 	.endm

-	/* IRQ is the lowest priority flag, unconditionally unmask the rest. */
-	.macro enable_da_f
-	msr	daifclr, #(8 | 4 | 1)
+	/* IRQ/FIQ are the lowest priority flags, unconditionally unmask the rest. */
+	.macro enable_da
+	msr	daifclr, #(8 | 4)
 	.endm

 /*
@@ -50,7 +50,7 @@
  */
 	.macro	save_and_disable_irq, flags
 	mrs	\flags, daif
-	msr	daifset, #2
+	msr	daifset, #3
 	.endm

 	.macro	restore_irq, flags
diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index 1c26d7baa67f..5eb7af9c4557 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -13,8 +13,8 @@
 #include <asm/ptrace.h>

 #define DAIF_PROCCTX		0
-#define DAIF_PROCCTX_NOIRQ	PSR_I_BIT
-#define DAIF_ERRCTX		(PSR_I_BIT | PSR_A_BIT)
+#define DAIF_PROCCTX_NOIRQ	(PSR_I_BIT | PSR_F_BIT)
+#define DAIF_ERRCTX		(PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
 #define DAIF_MASK		(PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)


@@ -47,7 +47,7 @@ static inline unsigned long local_daif_save_flags(void)
 	if (system_uses_irq_prio_masking()) {
 		/* If IRQs are masked with PMR, reflect it in the flags */
 		if (read_sysreg_s(SYS_ICC_PMR_EL1) != GIC_PRIO_IRQON)
-			flags |= PSR_I_BIT;
+			flags |= PSR_I_BIT | PSR_F_BIT;
 	}

 	return flags;
@@ -69,7 +69,7 @@ static inline void local_daif_restore(unsigned long flags)
 	bool irq_disabled = flags & PSR_I_BIT;

 	WARN_ON(system_has_prio_mask_debugging() &&
-		!(read_sysreg(daif) & PSR_I_BIT));
+		(read_sysreg(daif) & (PSR_I_BIT | PSR_F_BIT)) != (PSR_I_BIT | PSR_F_BIT));

 	if (!irq_disabled) {
 		trace_hardirqs_on();
@@ -86,7 +86,7 @@ static inline void local_daif_restore(unsigned long flags)
 			 * If interrupts are disabled but we can take
 			 * asynchronous errors, we can take NMIs
 			 */
-			flags &= ~PSR_I_BIT;
+			flags &= ~(PSR_I_BIT | PSR_F_BIT);
 			pmr = GIC_PRIO_IRQOFF;
 		} else {
 			pmr = GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET;
diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index ff328e5bbb75..b57b9b1e4344 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -12,15 +12,13 @@

 /*
  * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and
- * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'dai'
+ * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'daif'
  * order:
  * Masking debug exceptions causes all other exceptions to be masked too/
- * Masking SError masks irq, but not debug exceptions. Masking irqs has no
- * side effects for other flags. Keeping to this order makes it easier for
- * entry.S to know which exceptions should be unmasked.
- *
- * FIQ is never expected, but we mask it when we disable debug exceptions, and
- * unmask it at all other times.
+ * Masking SError masks IRQ/FIQ, but not debug exceptions. IRQ and FIQ are
+ * always masked and unmasked together, and have no side effects for other
+ * flags. Keeping to this order makes it easier for entry.S to know which
+ * exceptions should be unmasked.
  */

 /*
@@ -35,7 +33,7 @@ static inline void arch_local_irq_enable(void)
 	}

 	asm volatile(ALTERNATIVE(
-		"msr	daifclr, #2		// arch_local_irq_enable",
+		"msr	daifclr, #3		// arch_local_irq_enable",
 		__msr_s(SYS_ICC_PMR_EL1, "%0"),
 		ARM64_HAS_IRQ_PRIO_MASKING)
 		:
@@ -54,7 +52,7 @@ static inline void arch_local_irq_disable(void)
 	}

 	asm volatile(ALTERNATIVE(
-		"msr	daifset, #2		// arch_local_irq_disable",
+		"msr	daifset, #3		// arch_local_irq_disable",
 		__msr_s(SYS_ICC_PMR_EL1, "%0"),
 		ARM64_HAS_IRQ_PRIO_MASKING)
 		:
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index acc677672277..af04ce5088ca 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -533,7 +533,7 @@ alternative_endif

 	.macro el1_interrupt_handler, handler:req
 	gic_prio_irq_setup pmr=x20, tmp=x1
-	enable_da_f
+	enable_da

 	mov	x0, sp
 	bl	enter_el1_irq_or_nmi
@@ -544,8 +544,10 @@ alternative_endif
 	ldr	x24, [tsk, #TSK_TI_PREEMPT]	// get preempt count
 alternative_if ARM64_HAS_IRQ_PRIO_MASKING
 	/*
-	 * DA_F were cleared at start of handling. If anything is set in DAIF,
-	 * we come back from an NMI, so skip preemption
+	 * DA were cleared at start of handling, and IF are cleared by
+	 * the GIC irqchip driver using gic_arch_enable_irqs() for
+	 * normal IRQs. If anything is set, it means we come back from
+	 * an NMI instead of a normal IRQ, so skip preemption
 	 */
 	mrs	x0, daif
 	orr	x24, x24, x0
@@ -562,7 +564,7 @@ alternative_else_nop_endif
 	.macro el0_interrupt_handler, handler:req
 	gic_prio_irq_setup pmr=x20, tmp=x0
 	user_exit_irqoff
-	enable_da_f
+	enable_da

 	tbz	x22, #55, 1f
 	bl	do_el0_irq_bp_hardening
@@ -763,7 +765,7 @@ el0_error_naked:
 	mov	x0, sp
 	mov	x1, x25
 	bl	do_serror
-	enable_da_f
+	enable_da
 	b	ret_to_user
 SYM_CODE_END(el0_error)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 6616486a58fe..34ec400288d0 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -84,7 +84,7 @@ static void noinstr __cpu_do_idle_irqprio(void)
 	unsigned long daif_bits;

 	daif_bits = read_sysreg(daif);
-	write_sysreg(daif_bits | PSR_I_BIT, daif);
+	write_sysreg(daif_bits | PSR_I_BIT | PSR_F_BIT, daif);

 	/*
 	 * Unmask PMR before going idle to make sure interrupts can
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index ad00f99ee9b0..9dee8a17b1ac 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -188,6 +188,7 @@ static void init_gic_priority_masking(void)
 	cpuflags = read_sysreg(daif);

 	WARN_ON(!(cpuflags & PSR_I_BIT));
+	WARN_ON(!(cpuflags & PSR_F_BIT));

 	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
 }
--
2.30.0

Re: [PATCH 7/8 v1.5] arm64: Always keep DAIF.[IF] in sync

[Mark Rutland]

On Sat, Feb 20, 2021 at 02:25:30AM +0900, Hector Martin wrote:
> Apple SoCs (A11 and newer) have some interrupt sources hardwired to the
> FIQ line. We implement support for this by simply treating IRQs and FIQs
> the same way in the interrupt vectors.
> 
> To support these systems, the FIQ mask bit needs to be kept in sync with
> the IRQ mask bit, so both kinds of exceptions are masked together. No
> other platforms should be delivering FIQ exceptions right now, and we
> already unmask FIQ in normal process context, so this should not have an
> effect on other systems - if spurious FIQs were arriving, they would
> already panic the kernel.

This looks good to me; I've picked this up and pushed out my arm64/fiq
branch [1,2] incorporating this, tagged as arm64-fiq-20210219.

I'll give this version a few days to gather comments before I post a v2.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/fiq
[2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/fiqA

Thanks,
Mark.

> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will@kernel.org>
> 
> ---
>  arch/arm64/include/asm/arch_gicv3.h |  2 +-
>  arch/arm64/include/asm/assembler.h  |  8 ++++----
>  arch/arm64/include/asm/daifflags.h  | 10 +++++-----
>  arch/arm64/include/asm/irqflags.h   | 16 +++++++---------
>  arch/arm64/kernel/entry.S           | 12 +++++++-----
>  arch/arm64/kernel/process.c         |  2 +-
>  arch/arm64/kernel/smp.c             |  1 +
>  7 files changed, 26 insertions(+), 25 deletions(-)
> 
> This is the updated patch after addressing the comments in the original
> v2 review; we're moving it to this series now, so please review it in
> this context.
> 
> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index 880b9054d75c..934b9be582d2 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -173,7 +173,7 @@ static inline void gic_pmr_mask_irqs(void)
> 
>  static inline void gic_arch_enable_irqs(void)
>  {
> -	asm volatile ("msr daifclr, #2" : : : "memory");
> +	asm volatile ("msr daifclr, #3" : : : "memory");
>  }
> 
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index bf125c591116..53ff8c71eed7 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -40,9 +40,9 @@
>  	msr	daif, \flags
>  	.endm
> 
> -	/* IRQ is the lowest priority flag, unconditionally unmask the rest. */
> -	.macro enable_da_f
> -	msr	daifclr, #(8 | 4 | 1)
> +	/* IRQ/FIQ are the lowest priority flags, unconditionally unmask the rest. */
> +	.macro enable_da
> +	msr	daifclr, #(8 | 4)
>  	.endm
> 
>  /*
> @@ -50,7 +50,7 @@
>   */
>  	.macro	save_and_disable_irq, flags
>  	mrs	\flags, daif
> -	msr	daifset, #2
> +	msr	daifset, #3
>  	.endm
> 
>  	.macro	restore_irq, flags
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index 1c26d7baa67f..5eb7af9c4557 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -13,8 +13,8 @@
>  #include <asm/ptrace.h>
> 
>  #define DAIF_PROCCTX		0
> -#define DAIF_PROCCTX_NOIRQ	PSR_I_BIT
> -#define DAIF_ERRCTX		(PSR_I_BIT | PSR_A_BIT)
> +#define DAIF_PROCCTX_NOIRQ	(PSR_I_BIT | PSR_F_BIT)
> +#define DAIF_ERRCTX		(PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
>  #define DAIF_MASK		(PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
> 
> 
> @@ -47,7 +47,7 @@ static inline unsigned long local_daif_save_flags(void)
>  	if (system_uses_irq_prio_masking()) {
>  		/* If IRQs are masked with PMR, reflect it in the flags */
>  		if (read_sysreg_s(SYS_ICC_PMR_EL1) != GIC_PRIO_IRQON)
> -			flags |= PSR_I_BIT;
> +			flags |= PSR_I_BIT | PSR_F_BIT;
>  	}
> 
>  	return flags;
> @@ -69,7 +69,7 @@ static inline void local_daif_restore(unsigned long flags)
>  	bool irq_disabled = flags & PSR_I_BIT;
> 
>  	WARN_ON(system_has_prio_mask_debugging() &&
> -		!(read_sysreg(daif) & PSR_I_BIT));
> +		(read_sysreg(daif) & (PSR_I_BIT | PSR_F_BIT)) != (PSR_I_BIT | PSR_F_BIT));
> 
>  	if (!irq_disabled) {
>  		trace_hardirqs_on();
> @@ -86,7 +86,7 @@ static inline void local_daif_restore(unsigned long flags)
>  			 * If interrupts are disabled but we can take
>  			 * asynchronous errors, we can take NMIs
>  			 */
> -			flags &= ~PSR_I_BIT;
> +			flags &= ~(PSR_I_BIT | PSR_F_BIT);
>  			pmr = GIC_PRIO_IRQOFF;
>  		} else {
>  			pmr = GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET;
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index ff328e5bbb75..b57b9b1e4344 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -12,15 +12,13 @@
> 
>  /*
>   * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and
> - * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'dai'
> + * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'daif'
>   * order:
>   * Masking debug exceptions causes all other exceptions to be masked too/
> - * Masking SError masks irq, but not debug exceptions. Masking irqs has no
> - * side effects for other flags. Keeping to this order makes it easier for
> - * entry.S to know which exceptions should be unmasked.
> - *
> - * FIQ is never expected, but we mask it when we disable debug exceptions, and
> - * unmask it at all other times.
> + * Masking SError masks IRQ/FIQ, but not debug exceptions. IRQ and FIQ are
> + * always masked and unmasked together, and have no side effects for other
> + * flags. Keeping to this order makes it easier for entry.S to know which
> + * exceptions should be unmasked.
>   */
> 
>  /*
> @@ -35,7 +33,7 @@ static inline void arch_local_irq_enable(void)
>  	}
> 
>  	asm volatile(ALTERNATIVE(
> -		"msr	daifclr, #2		// arch_local_irq_enable",
> +		"msr	daifclr, #3		// arch_local_irq_enable",
>  		__msr_s(SYS_ICC_PMR_EL1, "%0"),
>  		ARM64_HAS_IRQ_PRIO_MASKING)
>  		:
> @@ -54,7 +52,7 @@ static inline void arch_local_irq_disable(void)
>  	}
> 
>  	asm volatile(ALTERNATIVE(
> -		"msr	daifset, #2		// arch_local_irq_disable",
> +		"msr	daifset, #3		// arch_local_irq_disable",
>  		__msr_s(SYS_ICC_PMR_EL1, "%0"),
>  		ARM64_HAS_IRQ_PRIO_MASKING)
>  		:
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index acc677672277..af04ce5088ca 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -533,7 +533,7 @@ alternative_endif
> 
>  	.macro el1_interrupt_handler, handler:req
>  	gic_prio_irq_setup pmr=x20, tmp=x1
> -	enable_da_f
> +	enable_da
> 
>  	mov	x0, sp
>  	bl	enter_el1_irq_or_nmi
> @@ -544,8 +544,10 @@ alternative_endif
>  	ldr	x24, [tsk, #TSK_TI_PREEMPT]	// get preempt count
>  alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>  	/*
> -	 * DA_F were cleared at start of handling. If anything is set in DAIF,
> -	 * we come back from an NMI, so skip preemption
> +	 * DA were cleared at start of handling, and IF are cleared by
> +	 * the GIC irqchip driver using gic_arch_enable_irqs() for
> +	 * normal IRQs. If anything is set, it means we come back from
> +	 * an NMI instead of a normal IRQ, so skip preemption
>  	 */
>  	mrs	x0, daif
>  	orr	x24, x24, x0
> @@ -562,7 +564,7 @@ alternative_else_nop_endif
>  	.macro el0_interrupt_handler, handler:req
>  	gic_prio_irq_setup pmr=x20, tmp=x0
>  	user_exit_irqoff
> -	enable_da_f
> +	enable_da
> 
>  	tbz	x22, #55, 1f
>  	bl	do_el0_irq_bp_hardening
> @@ -763,7 +765,7 @@ el0_error_naked:
>  	mov	x0, sp
>  	mov	x1, x25
>  	bl	do_serror
> -	enable_da_f
> +	enable_da
>  	b	ret_to_user
>  SYM_CODE_END(el0_error)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 6616486a58fe..34ec400288d0 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -84,7 +84,7 @@ static void noinstr __cpu_do_idle_irqprio(void)
>  	unsigned long daif_bits;
> 
>  	daif_bits = read_sysreg(daif);
> -	write_sysreg(daif_bits | PSR_I_BIT, daif);
> +	write_sysreg(daif_bits | PSR_I_BIT | PSR_F_BIT, daif);
> 
>  	/*
>  	 * Unmask PMR before going idle to make sure interrupts can
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index ad00f99ee9b0..9dee8a17b1ac 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -188,6 +188,7 @@ static void init_gic_priority_masking(void)
>  	cpuflags = read_sysreg(daif);
> 
>  	WARN_ON(!(cpuflags & PSR_I_BIT));
> +	WARN_ON(!(cpuflags & PSR_F_BIT));
> 
>  	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
>  }
> --
> 2.30.0
> 

Re: [PATCH 7/8 v1.5] arm64: Always keep DAIF.[IF] in sync

[Hector Martin]

On 20/02/2021 03.26, Mark Rutland wrote:
> On Sat, Feb 20, 2021 at 02:25:30AM +0900, Hector Martin wrote:
>> Apple SoCs (A11 and newer) have some interrupt sources hardwired to the
>> FIQ line. We implement support for this by simply treating IRQs and FIQs
>> the same way in the interrupt vectors.
>>
>> To support these systems, the FIQ mask bit needs to be kept in sync with
>> the IRQ mask bit, so both kinds of exceptions are masked together. No
>> other platforms should be delivering FIQ exceptions right now, and we
>> already unmask FIQ in normal process context, so this should not have an
>> effect on other systems - if spurious FIQs were arriving, they would
>> already panic the kernel.
> 
> This looks good to me; I've picked this up and pushed out my arm64/fiq
> branch [1,2] incorporating this, tagged as arm64-fiq-20210219.
> 
> I'll give this version a few days to gather comments before I post a v2.
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/fiq
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/fiqA

Thanks! Any chance you can do a rebase on top of torvalds/master? Since 
Marc's nVHE changes went in, we're going to need to add a workaround 
patch for the M1's lack of nVHE mode, which is going to be in the next 
version of my M1 bringup series - but right now that would involve 
telling people to merge two trees to build a base to apply it on, which 
is sub-optimal.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 7/8 v1.5] arm64: Always keep DAIF.[IF] in sync

[Mark Rutland]

On Tue, Feb 23, 2021 at 02:39:11AM +0900, Hector Martin wrote:
> On 20/02/2021 03.26, Mark Rutland wrote:
> > On Sat, Feb 20, 2021 at 02:25:30AM +0900, Hector Martin wrote:
> > > Apple SoCs (A11 and newer) have some interrupt sources hardwired to the
> > > FIQ line. We implement support for this by simply treating IRQs and FIQs
> > > the same way in the interrupt vectors.
> > > 
> > > To support these systems, the FIQ mask bit needs to be kept in sync with
> > > the IRQ mask bit, so both kinds of exceptions are masked together. No
> > > other platforms should be delivering FIQ exceptions right now, and we
> > > already unmask FIQ in normal process context, so this should not have an
> > > effect on other systems - if spurious FIQs were arriving, they would
> > > already panic the kernel.
> > 
> > This looks good to me; I've picked this up and pushed out my arm64/fiq
> > branch [1,2] incorporating this, tagged as arm64-fiq-20210219.
> > 
> > I'll give this version a few days to gather comments before I post a v2.
> > 
> > [1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/fiq
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/fiqA
> 
> Thanks! Any chance you can do a rebase on top of torvalds/master? Since
> Marc's nVHE changes went in, we're going to need to add a workaround patch
> for the M1's lack of nVHE mode, which is going to be in the next version of
> my M1 bringup series - but right now that would involve telling people to
> merge two trees to build a base to apply it on, which is sub-optimal.

I generally try to base on a stable tag/commit, so I'd prefer to avoid
rebasing the development branch until rc1 if possible. I've pushed out a
new arm64-fiq-mainline-20210222 tag rebased atop torvalds/master:

git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64-fiq-mainline-20210222
https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/tag/?h=arm64-fiq-mainline-20210222

... leaving the main branch atop v5.11. Is that good enough for now? If
that's painful for development I can shuffle the main branch along too.

Thanks,
Mark.

[PATCH 8/8] arm64: irq: allow FIQs to be handled

[Mark Rutland]

On contemporary platforms we don't use FIQ, and treat any stray FIQ as a
fatal event. However, some platforms have an interrupt controller wired
to FIQ, and need to handle FIQ as part of regular operation.

So that we can support both cases dynamically, this patch updates the
FIQ exception handling code to operate the same way as the IRQ handling
code, with its own handle_arch_fiq handler. Where an FIQ handler is not
registered, an unexpected FIQ exception will trigger the default FIQ
handler, which will panic() as today. Where a FIQ handler is registered,
handling of the FIQ is deferred to that handler.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Hector Martin <marcan@marcan.st>
Cc: James Morse <james.morse@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/irq.h |  1 +
 arch/arm64/kernel/entry.S    | 26 ++++++++++++++++++++++----
 arch/arm64/kernel/irq.c      | 15 +++++++++++++++
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index 8391c6f6f746..fac08e18bcd5 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -10,6 +10,7 @@ struct pt_regs;
 
 int set_handle_irq(void (*handle_irq)(struct pt_regs *));
 #define set_handle_irq	set_handle_irq
+int set_handle_fiq(void (*handle_fiq)(struct pt_regs *));
 
 static inline int nr_legacy_irqs(void)
 {
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 0474cca9f1a9..a8290bd87a49 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -586,23 +586,23 @@ SYM_CODE_START(vectors)
 
 	kernel_ventry	1, sync				// Synchronous EL1h
 	kernel_ventry	1, irq				// IRQ EL1h
-	kernel_ventry	1, fiq_invalid			// FIQ EL1h
+	kernel_ventry	1, fiq				// FIQ EL1h
 	kernel_ventry	1, error			// Error EL1h
 
 	kernel_ventry	0, sync				// Synchronous 64-bit EL0
 	kernel_ventry	0, irq				// IRQ 64-bit EL0
-	kernel_ventry	0, fiq_invalid			// FIQ 64-bit EL0
+	kernel_ventry	0, fiq				// FIQ 64-bit EL0
 	kernel_ventry	0, error			// Error 64-bit EL0
 
 #ifdef CONFIG_COMPAT
 	kernel_ventry	0, sync_compat, 32		// Synchronous 32-bit EL0
 	kernel_ventry	0, irq_compat, 32		// IRQ 32-bit EL0
-	kernel_ventry	0, fiq_invalid_compat, 32	// FIQ 32-bit EL0
+	kernel_ventry	0, fiq_compat, 32		// FIQ 32-bit EL0
 	kernel_ventry	0, error_compat, 32		// Error 32-bit EL0
 #else
 	kernel_ventry	0, sync_invalid, 32		// Synchronous 32-bit EL0
 	kernel_ventry	0, irq_invalid, 32		// IRQ 32-bit EL0
-	kernel_ventry	0, fiq_invalid, 32		// FIQ 32-bit EL0
+	kernel_ventry	0, fiq, 32			// FIQ 32-bit EL0
 	kernel_ventry	0, error_invalid, 32		// Error 32-bit EL0
 #endif
 SYM_CODE_END(vectors)
@@ -703,6 +703,12 @@ SYM_CODE_START_LOCAL_NOALIGN(el1_irq)
 	kernel_exit 1
 SYM_CODE_END(el1_irq)
 
+SYM_CODE_START_LOCAL_NOALIGN(el1_fiq)
+	kernel_entry 1
+	el1_interrupt_handler handle_arch_fiq
+	kernel_exit 1
+SYM_CODE_END(el1_fiq)
+
 /*
  * EL0 mode handlers.
  */
@@ -729,6 +735,11 @@ SYM_CODE_START_LOCAL_NOALIGN(el0_irq_compat)
 	b	el0_irq_naked
 SYM_CODE_END(el0_irq_compat)
 
+SYM_CODE_START_LOCAL_NOALIGN(el0_fiq_compat)
+	kernel_entry 0, 32
+	b	el0_fiq_naked
+SYM_CODE_END(el0_fiq_compat)
+
 SYM_CODE_START_LOCAL_NOALIGN(el0_error_compat)
 	kernel_entry 0, 32
 	b	el0_error_naked
@@ -743,6 +754,13 @@ el0_irq_naked:
 	b	ret_to_user
 SYM_CODE_END(el0_irq)
 
+SYM_CODE_START_LOCAL_NOALIGN(el0_fiq)
+	kernel_entry 0
+el0_fiq_naked:
+	el0_interrupt_handler handle_arch_fiq
+	b	ret_to_user
+SYM_CODE_END(el0_fiq)
+
 SYM_CODE_START_LOCAL(el1_error)
 	kernel_entry 1
 	mrs	x1, esr_el1
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 00bcf37aa0ea..bc3215dedf47 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -76,7 +76,13 @@ void default_handle_irq(struct pt_regs *regs)
 	panic("IRQ taken without a registered IRQ controller\n");
 }
 
+void default_handle_fiq(struct pt_regs *regs)
+{
+	panic("FIQ taken without a registered FIQ controller\n");
+}
+
 void (*handle_arch_irq)(struct pt_regs *) __ro_after_init = default_handle_irq;
+void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init = default_handle_fiq;
 
 int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 {
@@ -87,6 +93,15 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 	return 0;
 }
 
+int __init set_handle_fiq(void (*handle_fiq)(struct pt_regs *))
+{
+	if (handle_arch_fiq != default_handle_fiq)
+		return -EBUSY;
+
+	handle_arch_fiq = handle_fiq;
+	return 0;
+}
+
 void __init init_IRQ(void)
 {
 	init_irq_stacks();
-- 
2.11.0

Re: [PATCH 8/8] arm64: irq: allow FIQs to be handled

[Joey Gouly]

Hi Mark,

On Fri, Feb 19, 2021 at 11:39:04AM +0000, Mark Rutland wrote:
> On contemporary platforms we don't use FIQ, and treat any stray FIQ as a
> fatal event. However, some platforms have an interrupt controller wired
> to FIQ, and need to handle FIQ as part of regular operation.
> 

[...]

> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 0474cca9f1a9..a8290bd87a49 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -586,23 +586,23 @@ SYM_CODE_START(vectors)
>  
>  	kernel_ventry	1, sync				// Synchronous EL1h
>  	kernel_ventry	1, irq				// IRQ EL1h
> -	kernel_ventry	1, fiq_invalid			// FIQ EL1h
> +	kernel_ventry	1, fiq				// FIQ EL1h
>  	kernel_ventry	1, error			// Error EL1h
>  
>  	kernel_ventry	0, sync				// Synchronous 64-bit EL0
>  	kernel_ventry	0, irq				// IRQ 64-bit EL0
> -	kernel_ventry	0, fiq_invalid			// FIQ 64-bit EL0
> +	kernel_ventry	0, fiq				// FIQ 64-bit EL0
>  	kernel_ventry	0, error			// Error 64-bit EL0
>  
>  #ifdef CONFIG_COMPAT
>  	kernel_ventry	0, sync_compat, 32		// Synchronous 32-bit EL0
>  	kernel_ventry	0, irq_compat, 32		// IRQ 32-bit EL0
> -	kernel_ventry	0, fiq_invalid_compat, 32	// FIQ 32-bit EL0
> +	kernel_ventry	0, fiq_compat, 32		// FIQ 32-bit EL0
>  	kernel_ventry	0, error_compat, 32		// Error 32-bit EL0
>  #else
>  	kernel_ventry	0, sync_invalid, 32		// Synchronous 32-bit EL0
>  	kernel_ventry	0, irq_invalid, 32		// IRQ 32-bit EL0
> -	kernel_ventry	0, fiq_invalid, 32		// FIQ 32-bit EL0
> +	kernel_ventry	0, fiq, 32			// FIQ 32-bit EL0
>  	kernel_ventry	0, error_invalid, 32		// Error 32-bit EL0
>  #endif
>  SYM_CODE_END(vectors)

I believe you can now remove functions `el0_fiq_invalid` and `el0_fiq_invalid_compat`.
`el1_fiq_invalid` is still used by Synchronous EL1t, so can't be removed.

Thanks,
Joey

Re: [PATCH 8/8] arm64: irq: allow FIQs to be handled

[Mark Rutland]

On Fri, Feb 19, 2021 at 03:37:25PM +0000, Joey Gouly wrote:
> Hi Mark,
> 
> On Fri, Feb 19, 2021 at 11:39:04AM +0000, Mark Rutland wrote:
> > On contemporary platforms we don't use FIQ, and treat any stray FIQ as a
> > fatal event. However, some platforms have an interrupt controller wired
> > to FIQ, and need to handle FIQ as part of regular operation.
> > 
> 
> [...]
> 
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index 0474cca9f1a9..a8290bd87a49 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -586,23 +586,23 @@ SYM_CODE_START(vectors)
> >  
> >  	kernel_ventry	1, sync				// Synchronous EL1h
> >  	kernel_ventry	1, irq				// IRQ EL1h
> > -	kernel_ventry	1, fiq_invalid			// FIQ EL1h
> > +	kernel_ventry	1, fiq				// FIQ EL1h
> >  	kernel_ventry	1, error			// Error EL1h
> >  
> >  	kernel_ventry	0, sync				// Synchronous 64-bit EL0
> >  	kernel_ventry	0, irq				// IRQ 64-bit EL0
> > -	kernel_ventry	0, fiq_invalid			// FIQ 64-bit EL0
> > +	kernel_ventry	0, fiq				// FIQ 64-bit EL0
> >  	kernel_ventry	0, error			// Error 64-bit EL0
> >  
> >  #ifdef CONFIG_COMPAT
> >  	kernel_ventry	0, sync_compat, 32		// Synchronous 32-bit EL0
> >  	kernel_ventry	0, irq_compat, 32		// IRQ 32-bit EL0
> > -	kernel_ventry	0, fiq_invalid_compat, 32	// FIQ 32-bit EL0
> > +	kernel_ventry	0, fiq_compat, 32		// FIQ 32-bit EL0
> >  	kernel_ventry	0, error_compat, 32		// Error 32-bit EL0
> >  #else
> >  	kernel_ventry	0, sync_invalid, 32		// Synchronous 32-bit EL0
> >  	kernel_ventry	0, irq_invalid, 32		// IRQ 32-bit EL0
> > -	kernel_ventry	0, fiq_invalid, 32		// FIQ 32-bit EL0
> > +	kernel_ventry	0, fiq, 32			// FIQ 32-bit EL0
> >  	kernel_ventry	0, error_invalid, 32		// Error 32-bit EL0
> >  #endif
> >  SYM_CODE_END(vectors)
> 
> I believe you can now remove functions `el0_fiq_invalid` and `el0_fiq_invalid_compat`.
> `el1_fiq_invalid` is still used by Synchronous EL1t, so can't be removed.

Good spot; el0_fiq_invalid_compat can go. For the !CONFIG_COMPAT it was
wrong to move away from fiq_invalid as that vector should never
legimitately fire, so I'll revert back to fiq_invalid for that case and
match the rest of the !CONFIG_COMPAT AArch32 EL0 handlers.

Thanks,
Mark.

Re: [PATCH 0/8] arm64: Support FIQ controller registration

[Hector Martin]

Hi Mark,

Thanks for tackling this side of the problem!

On 19/02/2021 20.38, Mark Rutland wrote:
> The only functional difference here is that if an IRQ
> is somehow taken prior to set_handle_irq() the default handler will directly
> panic() rather than the vector branching to NULL.

That sounds like the right thing to do, certainly.

> The penultimate patch is cherry-picked from the v2 M1 series, and as per
> discussion there [3] will need a few additional fixups. I've included it for
> now as the DAIF.IF alignment is necessary for the FIQ exception handling added
> in the final patch.

> The final patch adds the low-level FIQ exception handling and registration
> mechanism atop the prior rework.
> 
> I'm hoping that we can somehow queue the first 6 patches of this series as a
> base for the M1 support. With that we can either cherry-pick a later version of
> the DAIF.IF patch here, or the M1 support series can take the FIQ handling
> patch. I've pushed the series out to my arm64/fiq branch [4] on kernel.org,
> atop v5.11.

Looks good! I cherry picked my updated version of the DAIF.IF patch into 
your series at [1] (3322522d), and then rebased the M1 series on top of 
it (with the change to use set_handle_fiq(), minus all the other 
obsoleted FIQ stuff) at [2]. It all boots and works as expected.

I think it makes sense for you to take the DAIF.IF patch, as it goes 
along with this series. Then we can base the M1 series off of it. If you 
think that works, I can send it off as a one-off reply to the version in 
this series and we can review it here if you want, or otherwise feel 
free to cherry-pick it into a v2 (CC as appropriate).

If this all makes sense, the v3 of the M1 series will then be based off 
of this patchset as in [2], and I'll link to your tree in the cover 
letter so others know where to apply it. Arnd (CCed) is going to be 
merging that one via the SoC tree, so as long as we coordinate a stable 
base once everything is reviewed and ready to merge, I believe it should 
all work out fine on the way up.

Just for completeness, the current DAIF.IF patch in the context of the 
original series is at [3] (4dd6330f), in case that's useful to someone 
for some reason (since there were conflicts due to the refactoring 
happening before it, it changed a bit).

[1] https://github.com/AsahiLinux/linux/tree/fiq
[2] https://github.com/AsahiLinux/linux/tree/upstream-bringup-v3
[3] https://github.com/AsahiLinux/linux/tree/upstream-bringup-v2.5

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 0/8] arm64: Support FIQ controller registration

[Mark Rutland]

On Sat, Feb 20, 2021 at 12:41:01AM +0900, Hector Martin wrote:
> Hi Mark,
> 
> Thanks for tackling this side of the problem!

No problem -- I have a vested interest in the arm64 exception management
code lookin the way I expect/prefer! ;)

> On 19/02/2021 20.38, Mark Rutland wrote:

> > I'm hoping that we can somehow queue the first 6 patches of this series as a
> > base for the M1 support. With that we can either cherry-pick a later version of
> > the DAIF.IF patch here, or the M1 support series can take the FIQ handling
> > patch. I've pushed the series out to my arm64/fiq branch [4] on kernel.org,
> > atop v5.11.
> 
> Looks good! I cherry picked my updated version of the DAIF.IF patch into
> your series at [1] (3322522d), and then rebased the M1 series on top of it
> (with the change to use set_handle_fiq(), minus all the other obsoleted FIQ
> stuff) at [2]. It all boots and works as expected.
> 
> I think it makes sense for you to take the DAIF.IF patch, as it goes along
> with this series. Then we can base the M1 series off of it. 

Sure; that works for me!

> If you think that works, I can send it off as a one-off reply to the
> version in this series and we can review it here if you want, or
> otherwise feel free to cherry-pick it into a v2 (CC as appropriate).

If you could do a one-off reply, that'd be fantastic -- that way
lore.kernel.org will archive it and it gives people a chance to provide
any tags or comments before the next respin of the whole series.

> If this all makes sense, the v3 of the M1 series will then be based off of
> this patchset as in [2], and I'll link to your tree in the cover letter so
> others know where to apply it.

As a heads-up, I'm currently treating my arm64/fiq branch as unstable
(and I've already applied a typo fix since this posting), but I can tag
versions of that to make it possible to refer to a specific version.

I'll make sure to do that once I fold in the new DAIF.[IF] patch, since
I assume that's the first version worth noting as a base.

> Arnd (CCed) is going to be merging that one via the SoC tree, so as
> long as we coordinate a stable base once everything is reviewed and
> ready to merge, I believe it should all work out fine on the way up.

That sounds about right to me.

I think the first step is for Marc and I to figure out how the core IRQ
bits go in (some of that might be an fix early in the current v5.12
cycle), and I'd expect to have a stable branch atop somewhere between
v5.12-rc1 and v5.12-rc4. For context, usually the arm64 core bits get
based on the previous rc3/rc4.

Thanks,
Mark.

> Just for completeness, the current DAIF.IF patch in the context of the
> original series is at [3] (4dd6330f), in case that's useful to someone for
> some reason (since there were conflicts due to the refactoring happening
> before it, it changed a bit).
> 
> [1] https://github.com/AsahiLinux/linux/tree/fiq
> [2] https://github.com/AsahiLinux/linux/tree/upstream-bringup-v3
> [3] https://github.com/AsahiLinux/linux/tree/upstream-bringup-v2.5
> 
> -- 
> Hector Martin (marcan@marcan.st)
> Public Key: https://mrcn.st/pub

Re: [PATCH 0/8] arm64: Support FIQ controller registration

[Marc Zyngier]

Hi Mark,

On Fri, 19 Feb 2021 11:38:56 +0000,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> Hector's M1 support series [1] shows that some platforms have critical
> interrupts wired to FIQ, and to support these platforms we need to support
> handling FIQ exceptions. Other contemporary platforms don't use FIQ (since e.g.
> this is usually routed to EL3), and as we never expect to take an FIQ, we have
> the FIQ vector cause a panic.
> 
> Since the use of FIQ is a platform integration detail (which can differ across
> bare-metal and virtualized environments), we need be able to explicitly opt-in
> to handling FIQs while retaining the existing behaviour otherwise. This series
> adds a new set_handle_fiq() hook so that the FIQ controller can do so, and
> where no controller is registered the default handler will panic(). For
> consistency the set_handle_irq() code is made to do the same.
> 
> The first couple of patches are from Marc's irq/drop-generic_irq_multi_handler
> branch [2] on kernel.org, and clean up CONFIG_GENERIC_IRQ_MULTI_HANDLER usage.
> The next four patches move arm64 over to a local set_handle_irq()
> implementation, which is written to share code with a set_handle_fiq() function
> in the last two patches. The only functional difference here is that if an IRQ
> is somehow taken prior to set_handle_irq() the default handler will directly
> panic() rather than the vector branching to NULL.
> 
> The penultimate patch is cherry-picked from the v2 M1 series, and as per
> discussion there [3] will need a few additional fixups. I've included it for
> now as the DAIF.IF alignment is necessary for the FIQ exception handling added
> in the final patch.
> 
> The final patch adds the low-level FIQ exception handling and registration
> mechanism atop the prior rework.

Thanks for putting this together. I have an extra patch on top of this
series[1] that prevents the kernel from catching fire if a FIQ fires
whilst running a guest. Nothing urgent, we can queue it at a later time.

Thanks,

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/fiq

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/8] arm64: Support FIQ controller registration

[Mark Rutland]

On Fri, Feb 19, 2021 at 06:10:56PM +0000, Marc Zyngier wrote:
> Hi Mark,
> 
> On Fri, 19 Feb 2021 11:38:56 +0000,
> Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > Hector's M1 support series [1] shows that some platforms have critical
> > interrupts wired to FIQ, and to support these platforms we need to support
> > handling FIQ exceptions. Other contemporary platforms don't use FIQ (since e.g.
> > this is usually routed to EL3), and as we never expect to take an FIQ, we have
> > the FIQ vector cause a panic.
> > 
> > Since the use of FIQ is a platform integration detail (which can differ across
> > bare-metal and virtualized environments), we need be able to explicitly opt-in
> > to handling FIQs while retaining the existing behaviour otherwise. This series
> > adds a new set_handle_fiq() hook so that the FIQ controller can do so, and
> > where no controller is registered the default handler will panic(). For
> > consistency the set_handle_irq() code is made to do the same.
> > 
> > The first couple of patches are from Marc's irq/drop-generic_irq_multi_handler
> > branch [2] on kernel.org, and clean up CONFIG_GENERIC_IRQ_MULTI_HANDLER usage.
> > The next four patches move arm64 over to a local set_handle_irq()
> > implementation, which is written to share code with a set_handle_fiq() function
> > in the last two patches. The only functional difference here is that if an IRQ
> > is somehow taken prior to set_handle_irq() the default handler will directly
> > panic() rather than the vector branching to NULL.
> > 
> > The penultimate patch is cherry-picked from the v2 M1 series, and as per
> > discussion there [3] will need a few additional fixups. I've included it for
> > now as the DAIF.IF alignment is necessary for the FIQ exception handling added
> > in the final patch.
> > 
> > The final patch adds the low-level FIQ exception handling and registration
> > mechanism atop the prior rework.
> 
> Thanks for putting this together. I have an extra patch on top of this
> series[1] that prevents the kernel from catching fire if a FIQ fires
> whilst running a guest. Nothing urgent, we can queue it at a later time.
> 
> Thanks,
> 
> 	M.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/fiq

IIUC for that "invalid_vect" should be changed to "valid_vect", to match
the other valid vector entries, but otherwise that looks sane to me.

I guess we could take that as a prerequisite ahead of the rest?

Thanks,
Mark.

Re: [PATCH 0/8] arm64: Support FIQ controller registration

[Marc Zyngier]

On 2021-02-24 14:06, Mark Rutland wrote:
> On Fri, Feb 19, 2021 at 06:10:56PM +0000, Marc Zyngier wrote:
>> Hi Mark,
>> 
>> On Fri, 19 Feb 2021 11:38:56 +0000,
>> Mark Rutland <mark.rutland@arm.com> wrote:
>> >
>> > Hector's M1 support series [1] shows that some platforms have critical
>> > interrupts wired to FIQ, and to support these platforms we need to support
>> > handling FIQ exceptions. Other contemporary platforms don't use FIQ (since e.g.
>> > this is usually routed to EL3), and as we never expect to take an FIQ, we have
>> > the FIQ vector cause a panic.
>> >
>> > Since the use of FIQ is a platform integration detail (which can differ across
>> > bare-metal and virtualized environments), we need be able to explicitly opt-in
>> > to handling FIQs while retaining the existing behaviour otherwise. This series
>> > adds a new set_handle_fiq() hook so that the FIQ controller can do so, and
>> > where no controller is registered the default handler will panic(). For
>> > consistency the set_handle_irq() code is made to do the same.
>> >
>> > The first couple of patches are from Marc's irq/drop-generic_irq_multi_handler
>> > branch [2] on kernel.org, and clean up CONFIG_GENERIC_IRQ_MULTI_HANDLER usage.
>> > The next four patches move arm64 over to a local set_handle_irq()
>> > implementation, which is written to share code with a set_handle_fiq() function
>> > in the last two patches. The only functional difference here is that if an IRQ
>> > is somehow taken prior to set_handle_irq() the default handler will directly
>> > panic() rather than the vector branching to NULL.
>> >
>> > The penultimate patch is cherry-picked from the v2 M1 series, and as per
>> > discussion there [3] will need a few additional fixups. I've included it for
>> > now as the DAIF.IF alignment is necessary for the FIQ exception handling added
>> > in the final patch.
>> >
>> > The final patch adds the low-level FIQ exception handling and registration
>> > mechanism atop the prior rework.
>> 
>> Thanks for putting this together. I have an extra patch on top of this
>> series[1] that prevents the kernel from catching fire if a FIQ fires
>> whilst running a guest. Nothing urgent, we can queue it at a later 
>> time.
>> 
>> Thanks,
>> 
>> 	M.
>> 
>> [1] 
>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/fiq
> 
> IIUC for that "invalid_vect" should be changed to "valid_vect", to 
> match
> the other valid vector entries, but otherwise that looks sane to me.

Err, yes. I though I had fixed that, but obviously didn't.

> I guess we could take that as a prerequisite ahead of the rest?

Sure, that's mostly independent anyway. And it would make more sense
for an unexpected FIQ to crash the host at at the point where we
handle interrupts rather than in KVM with very little debug information.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...