Re: [PATCH 05/43] KVM: arm64: gic-v5: Extract host IRS caps from IRS config frame

[Marc Zyngier <maz@kernel.org>]

On Mon, 27 Apr 2026 17:07:44 +0100,
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> 
> The host irqchip driver provides KVM with a pointer to an IRS's config
> frame, which allows KVM to directly interact with the host's IRS. The
> MMIO registers in the config frame are used to configure VMs (in
> addition to them being used by the host). The IRS's config frame also
> includes a set of ID registers which describe the capabilities that
> the IRS has.
> 
> Stash the pointer to the config frame, and extract the VM capabilities
> (from IRS_IDR3 & IRS_IDR4), as well as the IST
> capabilities/requirements (IRS_IDR2) from the IRS.
> 
> Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> ---
>  arch/arm64/kvm/Makefile              |  2 +-
>  arch/arm64/kvm/vgic/vgic-v5-tables.c |  8 +++++
>  arch/arm64/kvm/vgic/vgic-v5-tables.h | 41 ++++++++++++++++++++++
>  arch/arm64/kvm/vgic/vgic-v5.c        | 52 ++++++++++++++++++++++++++++
>  include/linux/irqchip/arm-gic-v5.h   | 10 ++++++
>  5 files changed, 112 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/kvm/vgic/vgic-v5-tables.c
>  create mode 100644 arch/arm64/kvm/vgic/vgic-v5-tables.h
> 
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 59612d2f277c1..431de9b145ca1 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -24,7 +24,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
>  	 vgic/vgic-mmio.o vgic/vgic-mmio-v2.o \
>  	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
>  	 vgic/vgic-its.o vgic/vgic-debug.o vgic/vgic-v3-nested.o \
> -	 vgic/vgic-v5.o
> +	 vgic/vgic-v5.o vgic/vgic-v5-tables.o
>  
>  kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o pmu.o
>  kvm-$(CONFIG_ARM64_PTR_AUTH)  += pauth.o
> diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> new file mode 100644
> index 0000000000000..30e2b108b1aa3
> --- /dev/null
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025, 2026 Arm Ltd.
> + */
> +
> +#include "vgic-v5-tables.h"
> +
> +struct vgic_v5_host_ist_caps gicv5_host_ist_caps;
> diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.h b/arch/arm64/kvm/vgic/vgic-v5-tables.h
> new file mode 100644
> index 0000000000000..cf00a248eabd5
> --- /dev/null
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2025, 2026 Arm Ltd.
> + */
> +
> +#ifndef __KVM_ARM_VGICV5_TABLES_H__
> +#define __KVM_ARM_VGICV5_TABLES_H__
> +
> +#include <linux/irqchip/arm-gic-v5.h>
> +
> +struct vgic_v5_host_ist_caps {
> +	/* IST Capabilities */
> +
> +	/* Apply to LPIs and SPIs */
> +	u8	ist_id_bits;
> +	bool	ist_levels;
> +	u8	ist_l2sz;
> +	bool	istmd;
> +	u8	istmd_sz;
> +
> +	/* LPI only */
> +	u8	min_lpi_id_bits;
> +
> +	/* VM Table, VPE Table */
> +	bool	two_level_vmt_support;
> +	u32	max_vms;
> +	u32	max_vpes;
> +	u16	vmd_size;
> +	u16	vped_size;
> +
> +	/* Is the IRS coherent with us, or not? */
> +	bool	irs_non_coherent;
> +};
> +
> +extern struct vgic_v5_host_ist_caps gicv5_host_ist_caps;
> +static inline struct vgic_v5_host_ist_caps *vgic_v5_host_caps(void)
> +{
> +	return &gicv5_host_ist_caps;
> +}

Err. No. Make gicv5_host_ist_caps static, and move the helper as
non-inline in vgic-v5-tables.c. It's not like this is anywhere near
performance-critical stuff, is it?

But also, if that's global information, we have kvm_vgic_global_state.
Isn't that where these things should live? Then the introduction of
vgic-v5-tables.[ch] can be moved to the point where it actually
matters.

> +
> +#endif
> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
> index d4789ff3e7402..fd3d6299a2baa 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> @@ -9,6 +9,7 @@
>  #include <linux/irqchip/arm-vgic-info.h>
>  
>  #include "vgic.h"
> +#include "vgic-v5-tables.h"
>  
>  #define ppi_caps	kvm_vgic_global_state.vgic_v5_ppi_caps
>  
> @@ -34,6 +35,54 @@ static void vgic_v5_get_implemented_ppis(void)
>  	__assign_bit(GICV5_ARCH_PPI_PMUIRQ, ppi_caps.impl_ppi_mask, system_supports_pmuv3());
>  }
>  
> +static void __iomem *irs_base;

Global state?

> +
> +static u32 irs_readl_relaxed(const u32 reg_offset)
> +{
> +	return readl_relaxed(irs_base + reg_offset);
> +}
> +
> +static int gicv5_irs_extract_vm_caps(const struct gic_kvm_info *info)
> +{
> +	u64 idr;
> +
> +	irs_base = info->gicv5_irs.base;
> +	if (!irs_base) {
> +		kvm_info("No GICv5 MMIO IRS address; no GICv5 support\n");
> +		return -ENODEV;
> +	}

Should you instead bail out early by not registering the gic_kvm_info
when the IRS base is unknown, making this sort of checks irrelevant?

Also, it's not like we can make it very far without an IRS...

> +
> +	vgic_v5_host_caps()->irs_non_coherent = info->gicv5_irs.non_coherent;
> +
> +	idr = irs_readl_relaxed(GICV5_IRS_IDR2);
> +
> +	/* We skip the LPI field as it only applies to physical LPIs */
> +	vgic_v5_host_caps()->ist_id_bits = FIELD_GET(GICV5_IRS_IDR2_ID_BITS, idr);
> +	vgic_v5_host_caps()->min_lpi_id_bits = FIELD_GET(GICV5_IRS_IDR2_MIN_LPI_ID_BITS, idr);
> +	vgic_v5_host_caps()->ist_levels = !!FIELD_GET(GICV5_IRS_IDR2_IST_LEVELS, idr);
> +	vgic_v5_host_caps()->ist_l2sz = FIELD_GET(GICV5_IRS_IDR2_IST_L2SZ, idr);
> +	vgic_v5_host_caps()->istmd = !!FIELD_GET(GICV5_IRS_IDR2_ISTMD, idr);
> +	vgic_v5_host_caps()->istmd_sz = FIELD_GET(GICV5_IRS_IDR2_ISTMD_SZ, idr);
> +
> +	idr = irs_readl_relaxed(GICV5_IRS_IDR3);
> +
> +	vgic_v5_host_caps()->max_vms = BIT(FIELD_GET(GICV5_IRS_IDR3_VM_ID_BITS, idr));
> +	vgic_v5_host_caps()->two_level_vmt_support = !!FIELD_GET(GICV5_IRS_IDR3_VMT_LEVELS, idr);
> +
> +	if (FIELD_GET(GICV5_IRS_IDR3_VMD, idr))

The constant (ab)use of FIELD_GET() for fields that are single bit
wide is very hard to read. I'd like to see:

	vgic_v5_host_caps()->ist_levels = (idr & GICV5_IRS_IDR2_IST_LEVELS);
[...]
	vgic_v5_host_caps()->istmd = (idr & GICV5_IRS_IDR2_ISTMD);
[...]
	if (idr & GICV5_IRS_IDR3_VMD)
[...]

which is infinitely more readable.

> +		vgic_v5_host_caps()->vmd_size = BIT(FIELD_GET(GICV5_IRS_IDR3_VMD_SZ, idr));
> +	else
> +		vgic_v5_host_caps()->vmd_size = 0;
> +
> +	idr = irs_readl_relaxed(GICV5_IRS_IDR4);
> +
> +	vgic_v5_host_caps()->vped_size = BIT(FIELD_GET(GICV5_IRS_IDR4_VPED_SZ, idr));
> +	/* Field stores VPE_ID_BITS - 1 */
> +	vgic_v5_host_caps()->max_vpes = BIT(FIELD_GET(GICV5_IRS_IDR4_VPE_ID_BITS, idr) + 1);
> +
> +	return 0;
> +}
> +
>  /*
>   * Probe for a vGICv5 compatible interrupt controller, returning 0 on success.
>   */
> @@ -61,6 +110,9 @@ int vgic_v5_probe(const struct gic_kvm_info *info)
>  		goto skip_v5;
>  	}
>  
> +	if (gicv5_irs_extract_vm_caps(info))
> +		goto skip_v5;
> +

We shouldn't "skip_v5" anymore. If we can't initialise KVM with GICv5,
we're done, and we should not even try to register v3.

Thanks,

	M.

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