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

Marc Zyngier
2025-09-23T18:16:22Z

On Tue, 23 Sep 2025 17:21:15 +0100,
Andre Przywara <andre.przywara@arm.com> wrote:
>
> 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."

This is written as such not to make ARMv8.0 illegal, as E2H is RES0
there. Yes, this is odd, but there is a logic behind it.

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

That's mostly a KVM bug. If we want to pretend we don't have VHE, then
CNTHV_*_EL2 must UNDEF, which isn't a big deal.

M.

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