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

[Mark Rutland]

On Tue, Aug 20, 2024 at 07:15:00PM +0100, Marc Zyngier wrote:
> 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.

I've replaced the above with:
For the above, how about:

  The GICv3 driver pokes GICv3 system registers in gic_prio_init()
  before gic_cpu_sys_reg_init() ensures that GICv3 system registers have
  been enabled by writing to ICC_SRE_EL1.SRE. On arm64 this is benign as
  has_useable_gicv3_cpuif() runs earlier during cpufeature detection,
  and this enables the GICv3 system registers. On 32-bit arm we're not
  so lucky, and when booting on an FVP using the boot-wrapper, the
  accesses in gic_prio_init() end up being UNDEFINED and crash the
  kernel during boot.

... and dropped the full splat given Thomas's comment on that.

[...]

> > -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.

Makes sense to me; done.

[...]

> Other than that, this looks good to me. With my nitpicking addressed:
> 
> Reviewed-by: Marc Zyngier <maz at kernel.org>

Thanks!

Mark.