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

Andre Przywara
2025-09-23T16:21:25Z

On Mon, 4 Aug 2025 15:47:55 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi,

> 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:

Even at the risk of going even deeper into that nitpicking rabbit hole:
"If FEAT_E2H0 is implemented, then FEAT_VHE is implemented."
So we have that timer, regardless of FEAT_E2H0, and regardless of whether
HCR_EL2.E2H is actually 0 or 1?
And indeed the configuration stanza and the pseudocode in "D24.10.9
CNTHV_CTL_EL2, Counter-timer Virtual Timer Control Register (EL2)" do not
mention SCR_EL2.E2H0 at all, just FEAT_VHE.

Cheers,
Andre

> 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
> >