Re: [PATCH] irqchip/gic-v3: init SRE before poking sysregs

[Marc Zyngier <maz@kernel.org>]

On Tue, 20 Aug 2024 16:55:06 +0100,
Mark Rutland <mark.rutland@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@arm.com>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will@kernel.org>
> ---
>  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@kernel.org>

	M.

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