Re: [PATCH v3 01/40] irqchip/gic-v5: Allow KVM setup without a maintenance IRQ

[Sascha Bischoff <Sascha.Bischoff@arm.com>]

On Fri, 2026-07-03 at 16:33 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> New issues:
> - [Medium] Leaked KVM `gic_kvm_info` allocation on error path when
> GICv5 legacy is supported but maintenance IRQ is absent.
> 
> Pre-existing issues:
> - [High] Writes to GICv5 control-plane system registers (ICC_*_EL1)
> lack immediate `isb()` synchronization.
> --
> 
> commit 77500f7dacbe8c6ac1528bc594d810bf95cef563
> Author: Sascha Bischoff <Sascha.Bischoff@arm.com>
> Subject: irqchip/gic-v5: Allow KVM setup without a maintenance IRQ
> 
> This commit modifies the GICv5 driver to allow KVM setup even if the
> maintenance interrupt is absent, provided that legacy GICv3 support
> is
> not required. It sets a flag to inform KVM that the maintenance
> interrupt
> is not needed for native GICv5 operation.
> 
> > diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-
> > gic-v5.c
> > index e9d1795235a66..c04523a438cee 100644
> > --- a/drivers/irqchip/irq-gic-v5.c
> > +++ b/drivers/irqchip/irq-gic-v5.c
> > @@ -1143,10 +1143,18 @@ static void __init
> > gic_of_setup_kvm_info(struct device_node *node)
> >  	/* GIC Virtual CPU interface maintenance interrupt */
> >  	gic_v5_kvm_info.no_maint_irq_mask = false;
> >  	gic_v5_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
> > -	if (!gic_v5_kvm_info.maint_irq) {
> > -		pr_warn("cannot find GICv5 virtual CPU interface
> > maintenance interrupt\n");
> > -		return;
> > -	}
> > +
> > +	/*
> > +	 * We require an MI if we have legacy support, but don't,
> > otherwise.
> > +	 * Given that there's an existing flag to convey that an
> > MI isn't
> > +	 * needed, we (ab)use it to tell KVM that the MI isn't
> > needed if we
> > +	 * don't support legacy.
> > +	 *
> > +	 * The check for ARM64_HAS_GICV5_LEGACY explicitly doesn't
> > use
> > +	 * cpus_have_final_cap() here as we run too early.
> > +	 */
> > +	if (!cpus_have_cap(ARM64_HAS_GICV5_LEGACY) &&
> > !gic_v5_kvm_info.maint_irq)
> > +		gic_v5_kvm_info.no_maint_irq_mask = true;
> >  
> >  	vgic_set_kvm_info(&gic_v5_kvm_info);
> 
> [Severity: Medium]
> Does this introduce a memory leak of the gic_kvm_info allocation?
> 
> If ARM64_HAS_GICV5_LEGACY is supported and maint_irq is 0, this
> bypasses
> the early return and proceeds to vgic_set_kvm_info(), which
> dynamically
> allocates gic_kvm_info.
> 
> Later, kvm_vgic_hyp_init() processes this configuration:
> 
> arch/arm64/kvm/vgic/vgic-init.c:kvm_vgic_hyp_init() {
>     ...
> 	if (has_mask && !gic_kvm_info->maint_irq) {
> 		kvm_err("No vgic maintenance irq\n");
> 		return -ENXIO;
> 	}
>     ...
> }
> 
> Will this early return bypass the kfree(gic_kvm_info) at the end of
> the
> function and leak the memory?

This will indeed bypass that free, but does so in just the same was as
an existing GICv3 system would. We're just preserving the same
behaviour for a GICv5 system that supports GICv3 VMs if no MI is
provided.

This is not a bug with the under-review patch, but it is something that
might be worth cleaning up in the future. I'll consider it for an
upcoming fixes series.

> 
> 
> [Severity: High]
> This is a pre-existing issue, but do the writes to GICv5 control-
> plane system
> registers lack immediate isb() synchronization?
> 
> For example, in drivers/irqchip/irq-gic-
> v5.c:gicv5_cpu_disable_interrupts():
> 
> 	cr0 = FIELD_PREP(ICC_CR0_EL1_EN, 0);
> 	write_sysreg_s(cr0, SYS_ICC_CR0_EL1);
> 
> And in gicv5_cpu_enable_interrupts():
> 
> 	write_sysreg_s(0, SYS_ICC_PPI_ENABLER0_EL1);
> 	write_sysreg_s(0, SYS_ICC_PPI_ENABLER1_EL1);
> 
> 	gicv5_ppi_priority_init();
> 
> ARM64 architectural invariants require writes to control-plane system
> registers to be followed immediately by an isb() as the very next
> instruction. Missing this can cause the CPU to operate under a stale
> configuration for several cycles.

These are fine. The ISB is only required if one needs to explicitly
ensure that the effects have completed. However, the cases you are
highlighting are related to initial interrupt setup and enabling of
interrupt delivery. There is no need for additional synchronisation
here - the interrupts, if there are any that can signal, will be
delivered eventually and the ISB gives us nothing meaningful.

Thanks,
Sascha