Re: [PATCH kvmtool v3 6/6] arm64: Generate HYP timer interrupt specifiers

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

Hi Andre,

On Tue, Jul 29, 2025 at 10:57:45AM +0100, Andre Przywara wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
> FEAT_VHE introduced a non-secure EL2 virtual timer, along with its
> interrupt line. Consequently the arch timer DT binding introduced a fifth
> interrupt to communicate this interrupt number.
> 
> Refactor the interrupts property generation code to deal with a variable
> number of interrupts, and forward five interrupts instead of four in case
> nested virt is enabled.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm64/arm-cpu.c           |  4 +---
>  arm64/include/kvm/timer.h |  2 +-
>  arm64/timer.c             | 29 ++++++++++++-----------------
>  3 files changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/arm64/arm-cpu.c b/arm64/arm-cpu.c
> index 1e456f2c6..abdd6324f 100644
> --- a/arm64/arm-cpu.c
> +++ b/arm64/arm-cpu.c
> @@ -12,11 +12,9 @@
>  
>  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,
>  				kvm->cfg.arch.nested_virt);
> -	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
> +	timer__generate_fdt_nodes(fdt, kvm);
>  	pmu__generate_fdt_nodes(fdt, kvm);
>  }
>  
> diff --git a/arm64/include/kvm/timer.h b/arm64/include/kvm/timer.h
> index 928e9ea7a..81e093e46 100644
> --- a/arm64/include/kvm/timer.h
> +++ b/arm64/include/kvm/timer.h
> @@ -1,6 +1,6 @@
>  #ifndef ARM_COMMON__TIMER_H
>  #define ARM_COMMON__TIMER_H
>  
> -void timer__generate_fdt_nodes(void *fdt, struct kvm *kvm, int *irqs);
> +void timer__generate_fdt_nodes(void *fdt, struct kvm *kvm);
>  
>  #endif /* ARM_COMMON__TIMER_H */
> diff --git a/arm64/timer.c b/arm64/timer.c
> index 861f2d994..2ac6144f9 100644
> --- a/arm64/timer.c
> +++ b/arm64/timer.c
> @@ -5,31 +5,26 @@
>  #include "kvm/timer.h"
>  #include "kvm/util.h"
>  
> -void timer__generate_fdt_nodes(void *fdt, struct kvm *kvm, int *irqs)
> +void timer__generate_fdt_nodes(void *fdt, struct kvm *kvm)
>  {
>  	const char compatible[] = "arm,armv8-timer\0arm,armv7-timer";
>  	u32 cpu_mask = gic__get_fdt_irq_cpumask(kvm);
> -	u32 irq_prop[] = {
> -		cpu_to_fdt32(GIC_FDT_IRQ_TYPE_PPI),
> -		cpu_to_fdt32(irqs[0]),
> -		cpu_to_fdt32(cpu_mask | IRQ_TYPE_LEVEL_LOW),
> +	int irqs[5] = {13, 14, 11, 10, 12};
> +	int nr = ARRAY_SIZE(irqs);
> +	u32 irq_prop[nr * 3];
>  
> -		cpu_to_fdt32(GIC_FDT_IRQ_TYPE_PPI),
> -		cpu_to_fdt32(irqs[1]),
> -		cpu_to_fdt32(cpu_mask | IRQ_TYPE_LEVEL_LOW),
> +	if (!kvm->cfg.arch.nested_virt)
> +		nr--;

I'm confused.

FEAT_VHE introduced the EL2 virtual timer, and my interpretation of the Arm ARM
is that the EL2 virtual timer is present if an only if FEAT_VHE:

"In an implementation of the Generic Timer that includes EL3, if EL3 can use
AArch64, the following timers are implemented:
[..]
* When FEAT_VHE is implemented, a Non-secure EL2 virtual timer."

Is my interpretation correct?

KVM doesn't allow FEAT_VHE and FEAT_E2H0 to coexist (in
nested.c::limit_nv_id_reg()), to force E2H to be RES0. Assuming my interpretion
is correct, shouldn't the check be:

	if (!kvm->cfg.arch.nested_virt || kvm->cfg.arch.e2h0)
		nr--;

Thanks,
Alex

>  
> -		cpu_to_fdt32(GIC_FDT_IRQ_TYPE_PPI),
> -		cpu_to_fdt32(irqs[2]),
> -		cpu_to_fdt32(cpu_mask | IRQ_TYPE_LEVEL_LOW),
> -
> -		cpu_to_fdt32(GIC_FDT_IRQ_TYPE_PPI),
> -		cpu_to_fdt32(irqs[3]),
> -		cpu_to_fdt32(cpu_mask | IRQ_TYPE_LEVEL_LOW),
> -	};
> +	for (int i = 0; i < nr; i++) {
> +		irq_prop[i * 3 + 0] = cpu_to_fdt32(GIC_FDT_IRQ_TYPE_PPI);
> +		irq_prop[i * 3 + 1] = cpu_to_fdt32(irqs[i]);
> +		irq_prop[i * 3 + 2] = cpu_to_fdt32(cpu_mask | IRQ_TYPE_LEVEL_LOW);
> +	}
>  
>  	_FDT(fdt_begin_node(fdt, "timer"));
>  	_FDT(fdt_property(fdt, "compatible", compatible, sizeof(compatible)));
> -	_FDT(fdt_property(fdt, "interrupts", irq_prop, sizeof(irq_prop)));
> +	_FDT(fdt_property(fdt, "interrupts", irq_prop, nr * 3 * sizeof(irq_prop[0])));
>  	_FDT(fdt_property(fdt, "always-on", NULL, 0));
>  	if (kvm->cfg.arch.force_cntfrq > 0)
>  		_FDT(fdt_property_cell(fdt, "clock-frequency", kvm->cfg.arch.force_cntfrq));
> -- 
> 2.25.1
>