Re: [PATCH v5 03/49] irqchip/gic-v5: Set up gic_kvm_info on ACPI hosts

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Fri, Aug 07, 2026 at 11:14:07AM +0000, Sascha Bischoff wrote:
> Device-tree based GICv5 probing already passes the IRS details and
> maintenance interrupt to KVM, but the ACPI path only initialises the
> irqchip and installs the ACPI IRQ model. As a result, KVM never sees
> the GICv5 host information required to probe the vGIC on ACPI systems.
> 
> Add the ACPI equivalent of the DT KVM setup. Parse the MADT GICC
> entries for the maintenance interrupt, retaining the value from the
> first relevant entry and warning if later entries disagree. Warn if
> firmware describes the interrupt as edge-triggered, but register it as
> a level-sensitive, active-high GSI. Pass the resulting IRQ together
> with the IRS base and coherency information to KVM.
> 
> Native GICv5 does not require a maintenance interrupt unless the
> legacy GICv3-compatible CPU interface is present, so preserve the
> existing no-maintenance-IRQ handling for that case.
> 
> Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> ---
>  drivers/irqchip/irq-gic-v5.c | 95 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 91 insertions(+), 4 deletions(-)

Just a bunch of nits below.

> diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
> index e7a7aedcfaf78..5b8ea8551d832 100644
> --- a/drivers/irqchip/irq-gic-v5.c
> +++ b/drivers/irqchip/irq-gic-v5.c
> @@ -1126,7 +1126,7 @@ static void gicv5_set_cpuif_idbits(void)
>  #ifdef CONFIG_KVM
>  static struct gic_kvm_info gic_v5_kvm_info __initdata;
>  
> -static void __init gic_of_setup_kvm_info(struct device_node *node)
> +static void __init gic_setup_kvm_info(unsigned int maint_irq)
>  {
>  	struct gicv5_irs_chip_data *irs_data = gicv5_irs_get_chip_data();
>  
> @@ -1137,17 +1137,19 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
>  	 */
>  	if (!gicv5_global_data.virt_capable) {
>  		pr_info("GIC implementation is not virtualization capable\n");
> -		return;
> +		goto out_dispose_maint_irq;

Isn't it cleaner to dispose the mapping in the caller (by returning an
error value) rather than here ?

I appreciate this keeps the IRQ mapping disposal in one place but it
decouples the allocation from the failure path.

>  	}
>  
> -	gic_v5_kvm_info.type = GIC_V5;
> +	if (WARN_ON(!irs_data))
> +		goto out_dispose_maint_irq;
>  
> +	gic_v5_kvm_info.type = GIC_V5;
>  	gic_v5_kvm_info.gicv5_irs.base = irs_data->irs_base;
>  	gic_v5_kvm_info.gicv5_irs.non_coherent = !!(irs_data->flags & IRS_FLAGS_NON_COHERENT);
>  
>  	/* 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);
> +	gic_v5_kvm_info.maint_irq = maint_irq;
>  
>  	/*
>  	 * We require an MI if we have legacy support, but don't, otherwise.
> @@ -1162,11 +1164,95 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
>  		gic_v5_kvm_info.no_maint_irq_mask = true;
>  
>  	vgic_set_kvm_info(&gic_v5_kvm_info);
> +	return;
> +
> +out_dispose_maint_irq:
> +	irq_dispose_mapping(maint_irq);
> +}
> +
> +static void __init gic_of_setup_kvm_info(struct device_node *node)
> +{
> +	/* GIC Virtual CPU interface maintenance interrupt */
> +	gic_setup_kvm_info(irq_of_parse_and_map(node, 0));
> +}
> +
> +#ifdef CONFIG_ACPI
> +struct gicv5_acpi_kvm_info {
> +	u32 maint_irq;
> +};
> +
> +static struct gicv5_acpi_kvm_info acpi_v5_kvm_info __initdata;
> +
> +static int __init gic_acpi_parse_virt_madt_gicc(union acpi_subtable_headers *header,
> +						const unsigned long end)
> +{
> +	struct acpi_madt_generic_interrupt *gicc =
> +		(struct acpi_madt_generic_interrupt *)header;
> +	static int first_madt = true;
> +
> +	if (!(gicc->flags &
> +	      (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
> +		return 0;

This would count as a valid parsed entry even though you don't retrieve the
maintanance interrupt number, more below.

> +
> +	if (gicc->flags & ACPI_MADT_VGIC_IRQ_MODE)
> +		pr_warn_once(FW_BUG "MI wrongly advertised as Edge-triggered\n");
> +
> +	if (first_madt) {
> +		first_madt = false;
> +
> +		acpi_v5_kvm_info.maint_irq = gicc->vgic_interrupt;
> +		return 0;
> +	}
> +
> +	if (acpi_v5_kvm_info.maint_irq != gicc->vgic_interrupt)
> +		pr_warn_once(FW_BUG "MI not the same on all CPUs\n");
> +
> +	return 0;
> +}
> +
> +static bool __init gic_acpi_collect_virt_info(void)
> +{
> +	int count;
> +
> +	count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> +				      gic_acpi_parse_virt_madt_gicc, 0);
> +
> +	return count > 0;
>  }
> +
> +static void __init gic_acpi_setup_kvm_info(void)
> +{
> +	unsigned int maint_irq = 0;
> +	int irq;
> +
> +	if (!gic_acpi_collect_virt_info()) {
> +		pr_warn("Unable to get hardware information used for virtualization\n");
> +		return;
> +	}

Nit: gic_acpi_collect_virt_info() might return true and you still don't get
the info you need. Now, at least one GICC entry should be enabled and online
capable (we are booting on a core after all but never overestimate ACPI tables :))
so this is theory but I thought I would mention it.

> +
> +	if (acpi_v5_kvm_info.maint_irq) {
> +		irq = acpi_register_gsi(NULL, acpi_v5_kvm_info.maint_irq,
> +					ACPI_LEVEL_SENSITIVE,
> +					ACPI_ACTIVE_HIGH);
> +		if (irq > 0)
> +			maint_irq = irq;
> +		else
> +			pr_warn("Failed to register GSI for GICv5 maintenance IRQ\n");

Another nit: do we really need the maint_irq variable ?

Thanks,
Lorenzo

> +	}
> +
> +	gic_setup_kvm_info(maint_irq);
> +}
> +#endif // CONFIG_ACPI
>  #else
>  static inline void __init gic_of_setup_kvm_info(struct device_node *node)
>  {
>  }
> +
> +#ifdef CONFIG_ACPI
> +static inline void __init gic_acpi_setup_kvm_info(void)
> +{
> +}
> +#endif
>  #endif // CONFIG_KVM
>  
>  static int __init gicv5_init_common(struct fwnode_handle *parent_domain)
> @@ -1265,6 +1351,7 @@ static int __init gic_acpi_init(union acpi_subtable_headers *header, const unsig
>  		goto out_irs;
>  
>  	acpi_set_irq_model(ACPI_IRQ_MODEL_GIC_V5, gic_v5_get_gsi_domain_id);
> +	gic_acpi_setup_kvm_info();
>  
>  	return 0;
>  
> -- 
> 2.34.1