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

[Marc Zyngier <maz@kernel.org>]

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.