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/[email protected]
[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/[email protected]
[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
From: Marc Zyngier <[email protected]>
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 <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Signed-off-by: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Hector Martin <[email protected]>
Cc: James Morse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
---
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
From: Marc Zyngier <[email protected]>
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 <[email protected]>
[Mark: expand commit message]
Signed-off-by: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Hector Martin <[email protected]>
Cc: James Morse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
---
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
From: Marc Zyngier <[email protected]>
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 <[email protected]>
[Mark: use a single handler pointer]
Signed-off-by: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Hector Martin <[email protected]>
Cc: James Morse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
---
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
From: Marc Zyngier <[email protected]>
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 <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Hector Martin <[email protected]>
Cc: James Morse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
---
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
From: Marc Zyngier <[email protected]>
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 <[email protected]>
[Mark: rework macros, commit message, rebase before DAIF rework]
Signed-off-by: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Hector Martin <[email protected]>
Cc: James Morse <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
---
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
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 <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Hector Martin <[email protected]>
Cc: James Morse <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
---
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
From: Hector Martin <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: James Morse <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
---
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/[email protected]
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
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 <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Hector Martin <[email protected]>
Cc: James Morse <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
---
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
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
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 ([email protected])
Public Key: https://mrcn.st/pub
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 ([email protected])
> Public Key: https://mrcn.st/pub
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 <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: James Morse <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
---
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
Hi Mark,
On Fri, 19 Feb 2021 11:38:56 +0000,
Mark Rutland <[email protected]> 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.
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.
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 <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: James Morse <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Will Deacon <[email protected]>
>
> ---
> 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
>
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
>
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...
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.
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...
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.
On Mon, 22 Feb 2021 12:06:14 +0000,
Mark Rutland <[email protected]> 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.
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 ([email protected])
Public Key: https://mrcn.st/pub
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.
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 <[email protected]> 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.
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 <[email protected]> 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...