Hi Andre,
On Tue, Jul 29, 2025 at 10:57:45AM +0100, Andre Przywara wrote:
> From: Marc Zyngier <maz@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
> 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
>
