[PATCH] irqchip/gic-v3: init SRE before poking sysregs
Marc Zyngier
maz at kernel.org
Tue Aug 20 11:15:00 PDT 2024
On Tue, 20 Aug 2024 16:55:06 +0100,
Mark Rutland <mark.rutland at arm.com> wrote:
>
> The GICv3 driver pokes GICv3 system registers in gic_prio_init() before
> gic_cpu_sys_reg_init() ensures that SRE has been initialized. On arm64
> the architecture code will have initialized ZRE prior to this, but on
s/ZRE/SRE/, unless you're talking about a new SVE-aware GICv3... ;-)
It'd be worth indicating that this is done as part of the GICv3
probing in the cpu feature discovery code, and that 32bit doesn't have
(for better or worse) anything like that.
> 32-bit ARM that is not the case, and consequently in gic_prio_init() the
> system register accesses may result in an UNDEF.
This is interesting, as this doesn't trigger under KVM, which has a
much more inflexible implementation of GICv3 where ICC_SRE_EL1.SRE is
RAO/WI (I just booted -rc4 on the usual AArch32-capable suspect).
But I expect this would trigger on a bare metal platform with any of
ARM's v8.0 cores and a GIC500.
>
> This is a regression introduced by commit:
>
> d447bf09a4013541 ("irqchip/gic-v3: Detect GICD_CTRL.DS and SCR_EL3.FIQ earlier")
>
> ... which added gic_prio_init().
>
> This has been observed to result in boot failures when booting a 32-bit
> kernel on an FVP using the boot-wrapper, e.g.
>
> | Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
> | Modules linked in:
> | CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.11.0-rc3-00002-g102b1595b998 #6
> | Hardware name: ARM-Versatile Express
> | PC is at gic_init_bases+0x378/0x76c
> | LR is at gic_init_bases+0x30c/0x76c
> | pc : [<c1a34804>] lr : [<c1a34798>] psr: 600000d3
> | sp : c1c01e18 ip : 00000000 fp : 00000001
> | r10: 2f000000 r9 : c1ebcc68 r8 : 00000000
> | r7 : c1c097c0 r6 : c17adae0 r5 : eeff7edc r4 : c1c05af8
> | r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : 0000001e
> | Flags: nZCv IRQs off FIQs off Mode SVC_32 ISA ARM Segment none
> | Control: 10c0383d Table: 8020406a DAC: 00000051
> | Register r0 information: non-paged memory
> | Register r1 information: NULL pointer
> | Register r2 information: NULL pointer
> | Register r3 information: NULL pointer
> | Register r4 information: non-slab/vmalloc memory
> | Register r5 information: non-slab/vmalloc memory
> | Register r6 information: non-slab/vmalloc memory
> | Register r7 information: non-slab/vmalloc memory
> | Register r8 information: NULL pointer
> | Register r9 information: non-slab/vmalloc memory
> | Register r10 information: non-paged memory
> | Register r11 information: non-paged memory
> | Register r12 information: NULL pointer
> | Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
> | Stack: (0xc1c01e18 to 0xc1c02000)
> | 1e00: c0207c28 2f280000
> | 1e20: f0a7ffff ffe00000 fffff000 eeff7edc 00000000 00000000 ffffffff 00000000
> | 1e40: 00000000 c133cd3c c1c05b00 00000000 00000000 00000000 00000000 c2092410
> | 1e60: c17d615c c04b6710 ff800000 00200000 00000000 f0880000 ff8024c8 eeff7f5c
> | 1e80: c17d6280 c0f90b00 c1ee1434 a00000d3 eeff7ed0 c17d6280 00000001 c2092410
> | 1ea0: c17d615c 00000000 c133cd24 eeff7ed0 2f000000 f0820000 c2092400 00000001
> | 1ec0: c2092410 c17d615c 00000001 c1a34db8 00000000 00000000 eeff7edc c17d7e84
> | 1ee0: c1c01efc 00000001 00000000 00000000 00000000 2f100000 2f2fffff eeff7f3c
> | 1f00: 00000200 00000000 00000000 00000000 00000000 c0f90aec c1b55078 00000000
> | 1f20: 00000000 c1b5513c 00000000 00000000 00000000 00000000 c1c01f6c c2092340
> | 1f40: 00000000 c1c01f6c c1c01f74 c1c01f6c 00000122 00000100 c18183d8 c1aa489c
> | 1f60: 00000000 00000007 00000000 c1c01f6c c1c01f6c c1c01f74 c1c01f74 00000000
> | 1f80: 00000000 c1acfa50 c1b5a000 c191b3c8 c1a0100c efffee00 00000000 00000038
> | 1fa0: 00000000 c1a03fd0 c1a0100c c1a1f6cc 00000000 c1e7c000 c19196d8 00000000
> | 1fc0: c1c04e00 c1a0100c ffffffff ffffffff 00000000 c1a006ec 00000000 00000000
> | 1fe0: 00000000 c1acfa60 00000000 ffffffff 00000000 00000000 00000000 00000000
> | Call trace:
> | gic_init_bases from gic_of_init+0x1c0/0x29c
> | gic_of_init from of_irq_init+0x1d4/0x324
> | of_irq_init from init_IRQ+0xa8/0x108
> | init_IRQ from start_kernel+0x540/0x6b8
> | start_kernel from 0x0
> | Code: e2033040 e3530000 13a01001 03a01000 (ee140f16)
> | ---[ end trace 0000000000000000 ]---
> | Kernel panic - not syncing: Attempted to kill the idle task!
> | ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
>
> Fix this by factoring out the SRE initialization into a new
> gic_sre_init(), and calling this in the early in the three paths where
> SRE may not have been initialized:
>
> (1) gic_init_bases(), before the primary CPU pokes GICv3 sysregs in
> gic_prio_init().
>
> (2) gic_starting_cpu(), before secondary CPUs initialize GICv3 sysregs
> in gic_cpu_init().
>
> (3) gic_cpu_pm_notifier(), before CPUs re-initialize GICv3 sysregs in
> gic_cpu_sys_reg_init().
>
> Fixes: d447bf09a4013541 ("irqchip/gic-v3: Detect GICD_CTRL.DS and SCR_EL3.FIQ earlier")
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Alexandru Elisei <alexandru.elisei at arm.com>
> Cc: Andre Przywara <andre.przywara at arm.com>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Marc Zyngier <maz at kernel.org>
> Cc: Thomas Gleixner <tglx at linutronix.de>
> Cc: Will Deacon <will at kernel.org>
> ---
> drivers/irqchip/irq-gic-v3.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index c19083bfb9432..60cbfe37d5380 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1154,14 +1154,8 @@ static void gic_update_rdist_properties(void)
> gic_data.rdists.has_vpend_valid_dirty ? "Valid+Dirty " : "");
> }
>
> -static void gic_cpu_sys_reg_init(void)
> +static void gic_sre_init(void)
I'm nitpicking, but that's the only thing I have to do on a train:
"SRE" really means "System Register Enable", so maybe something along
the lines of gic_cpu_sys_reg_enable() would be more appropriate. It
would also make clear that before initialising the sysregs, you need
to enable them.
> {
> - int i, cpu = smp_processor_id();
> - u64 mpidr = gic_cpu_to_affinity(cpu);
> - u64 need_rss = MPIDR_RS(mpidr);
> - bool group0;
> - u32 pribits;
> -
> /*
> * Need to check that the SRE bit has actually been set. If
> * not, it means that SRE is disabled at EL2. We're going to
> @@ -1172,6 +1166,16 @@ static void gic_cpu_sys_reg_init(void)
> if (!gic_enable_sre())
> pr_err("GIC: unable to set SRE (disabled at EL2), panic ahead\n");
>
> +}
> +
> +static void gic_cpu_sys_reg_init(void)
> +{
> + int i, cpu = smp_processor_id();
> + u64 mpidr = gic_cpu_to_affinity(cpu);
> + u64 need_rss = MPIDR_RS(mpidr);
> + bool group0;
> + u32 pribits;
> +
> pribits = gic_get_pribits();
>
> group0 = gic_has_group0();
> @@ -1333,6 +1337,7 @@ static int gic_check_rdist(unsigned int cpu)
>
> static int gic_starting_cpu(unsigned int cpu)
> {
> + gic_sre_init();
> gic_cpu_init();
>
> if (gic_dist_supports_lpis())
> @@ -1498,6 +1503,7 @@ static int gic_cpu_pm_notifier(struct notifier_block *self,
> if (cmd == CPU_PM_EXIT) {
> if (gic_dist_security_disabled())
> gic_enable_redist(true);
> + gic_sre_init();
> gic_cpu_sys_reg_init();
> } else if (cmd == CPU_PM_ENTER && gic_dist_security_disabled()) {
> gic_write_grpen1(0);
> @@ -2070,6 +2076,7 @@ static int __init gic_init_bases(phys_addr_t dist_phys_base,
>
> gic_update_rdist_properties();
>
> + gic_sre_init();
> gic_prio_init();
> gic_dist_init();
> gic_cpu_init();
Other than that, this looks good to me. With my nitpicking addressed:
Reviewed-by: Marc Zyngier <maz at kernel.org>
M.
--
Without deviation from the norm, progress is not possible.
More information about the linux-arm-kernel
mailing list