Re: [PATCH kvmtool v3 3/6] arm64: nested: add support for setting maintenance IRQ

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Andre,

'add' should be capitalized.

On Tue, Jul 29, 2025 at 10:57:42AM +0100, Andre Przywara wrote:
> Uses the new VGIC KVM device attribute to set the maintenance IRQ.
> This is fixed to use PPI 9, as a platform decision made by kvmtool,
> matching the SBSA recommendation.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm64/arm-cpu.c         |  3 ++-
>  arm64/gic.c             | 21 ++++++++++++++++++++-
>  arm64/include/kvm/gic.h |  2 +-
>  3 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/arm64/arm-cpu.c b/arm64/arm-cpu.c
> index 69bb2cb2c..1e456f2c6 100644
> --- a/arm64/arm-cpu.c
> +++ b/arm64/arm-cpu.c
> @@ -14,7 +14,8 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
>  
> -	gic__generate_fdt_nodes(fdt, kvm->cfg.arch.irqchip);
> +	gic__generate_fdt_nodes(fdt, kvm->cfg.arch.irqchip,
> +				kvm->cfg.arch.nested_virt);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  	pmu__generate_fdt_nodes(fdt, kvm);
>  }
> diff --git a/arm64/gic.c b/arm64/gic.c
> index b0d3a1abb..7461b0f3f 100644
> --- a/arm64/gic.c
> +++ b/arm64/gic.c
> @@ -11,6 +11,8 @@
>  
>  #define IRQCHIP_GIC 0
>  
> +#define GIC_MAINT_IRQ	9
> +
>  static int gic_fd = -1;
>  static u64 gic_redists_base;
>  static u64 gic_redists_size;
> @@ -302,10 +304,15 @@ static int gic__init_gic(struct kvm *kvm)
>  
>  	int lines = irq__get_nr_allocated_lines();
>  	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
> +	u32 maint_irq = GIC_MAINT_IRQ + 16;			/* PPI */

There's already a define for PPIs:

	u32 maint_irq = GIC_PPI_IRQ_BASE + GIC_MAINT_IRQ;

>  	struct kvm_device_attr nr_irqs_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
>  		.addr	= (u64)(unsigned long)&nr_irqs,
>  	};
> +	struct kvm_device_attr maint_irq_attr = {
> +		.group	= KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ,
> +		.addr	= (u64)(unsigned long)&maint_irq,
> +	};
>  	struct kvm_device_attr vgic_init_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_CTRL,
>  		.attr	= KVM_DEV_ARM_VGIC_CTRL_INIT,
> @@ -325,6 +332,13 @@ static int gic__init_gic(struct kvm *kvm)
>  			return ret;
>  	}
>  
> +	if (kvm->cfg.arch.nested_virt &&
> +	    !ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &maint_irq_attr)) {

I'm not sure how useful the HAS_DEVICE_ATTR call is here: kvm_cpu__arch_init(),
which checks for KVM_CAP_ARM_EL2 capability, is called before gic__init_gic()
(base_init() vs late_init()). So at this point we know that KVM supports nested
virtualization.

Was it that KVM at some point supported nested virtualization but didn't have
the KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ device attribute implemented? And if that was
the case, do we want to support that version of KVM in kvmtool?

> +		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &maint_irq_attr);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	irq__routing_init(kvm);
>  
>  	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &vgic_init_attr)) {
> @@ -342,7 +356,7 @@ static int gic__init_gic(struct kvm *kvm)
>  }
>  late_init(gic__init_gic)
>  
> -void gic__generate_fdt_nodes(void *fdt, enum irqchip_type type)
> +void gic__generate_fdt_nodes(void *fdt, enum irqchip_type type, bool nested)

I think you can drop 'type' and 'nested' and pass kvm directly, see below why.

>  {
>  	const char *compatible, *msi_compatible = NULL;
>  	u64 msi_prop[2];
> @@ -350,6 +364,8 @@ void gic__generate_fdt_nodes(void *fdt, enum irqchip_type type)
>  		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>  		0, 0,				/* to be filled */
>  	};
> +	u32 maint_irq[3] = {cpu_to_fdt32(1), cpu_to_fdt32(GIC_MAINT_IRQ),
                      ^
You can leave that empty for the compiler to figure it out, like for the
'reg_prop' local variable.

Also, there's a define to specify the IRQ type, it's GIC_FDT_IRQ_TYPE_PPI, you
might want to use that.

> +			    cpu_to_fdt32(0xff04)};
                                         ^^^^^^
I think gic__get_fdt_irq_cpumask(kvm) | IRQ_TYPE_LEVEL_HIGH is better, similar
to pmu.c and timer.c.

>  
>  	switch (type) {
>  	case IRQCHIP_GICV2M:
> @@ -377,6 +393,9 @@ void gic__generate_fdt_nodes(void *fdt, enum irqchip_type type)
>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
> +	if (nested)
> +		_FDT(fdt_property(fdt, "interrupts", maint_irq,
> +				  sizeof(maint_irq)));

Braces around the if if statement body? (it's multiline even though it's on
instruction)

Thanks,
Alex

>  	_FDT(fdt_property_cell(fdt, "phandle", PHANDLE_GIC));
>  	_FDT(fdt_property_cell(fdt, "#address-cells", 2));
>  	_FDT(fdt_property_cell(fdt, "#size-cells", 2));
> diff --git a/arm64/include/kvm/gic.h b/arm64/include/kvm/gic.h
> index ad8bcbf21..1541a5824 100644
> --- a/arm64/include/kvm/gic.h
> +++ b/arm64/include/kvm/gic.h
> @@ -36,7 +36,7 @@ struct kvm;
>  int gic__alloc_irqnum(void);
>  int gic__create(struct kvm *kvm, enum irqchip_type type);
>  int gic__create_gicv2m_frame(struct kvm *kvm, u64 msi_frame_addr);
> -void gic__generate_fdt_nodes(void *fdt, enum irqchip_type type);
> +void gic__generate_fdt_nodes(void *fdt, enum irqchip_type type, bool nested);
>  u32 gic__get_fdt_irq_cpumask(struct kvm *kvm);
>  
>  int gic__add_irqfd(struct kvm *kvm, unsigned int gsi, int trigger_fd,
> -- 
> 2.25.1
>