Re: [PATCH v2 26/36] KVM: arm64: gic-v5: Bump arch timer for GICv5

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 19 Dec 2025 15:52:45 +0000
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:

> Now that GICv5 has arrived, the arch timer requires some TLC to
> address some of the key differences introduced with GICv5.
> 
> For PPIs on GICv5, the set_pending_state and queue_irq_unlock irq_ops
> are used as AP lists are not required at all for GICv5. The arch timer
> also introduces an irq_op - get_input_level. Extend the
> arch-timer-provided irq_ops to include the two PPI ops for vgic_v5
> guests.
> 
> When possible, DVI (Direct Virtual Interrupt) is set for PPIs when
> using a vgic_v5, which directly inject the pending state in to the

into ?

> guest. This means that the host never sees the interrupt for the guest
> for these interrupts. This has two impacts.
> 
> * First of all, the kvm_cpu_has_pending_timer check is updated to
>   explicitly check if the timers are expected to fire.
> 
> * Secondly, for mapped timers (which use DVI) they must be masked on
>   the host prior to entering a GICv5 guest, and unmasked on the return
>   path. This is handled in set_timer_irq_phys_masked.
> 
> The final, but rather important, change is that the architected PPIs
> for the timers are made mandatory for a GICv5 guest. Attempts to set
> them to anything else are actively rejected. Once a vgic_v5 is
> initialised, the arch timer PPIs are also explicitly reinitialised to
> ensure the correct GICv5-compatible PPIs are used - this also adds in
> the GICv5 PPI type to the intid.
> 
> Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
Various comments inline. 

J
> ---
>  arch/arm64/kvm/arch_timer.c     | 110 ++++++++++++++++++++++++++------
>  arch/arm64/kvm/vgic/vgic-init.c |   9 +++
>  arch/arm64/kvm/vgic/vgic-v5.c   |   8 +--
>  include/kvm/arm_arch_timer.h    |   7 +-
>  include/kvm/arm_vgic.h          |   4 ++
>  5 files changed, 115 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 6f033f6644219..78d66a67b34ac 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c


>  void kvm_timer_sync_nested(struct kvm_vcpu *vcpu)
> @@ -1034,12 +1079,15 @@ void kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu)
>  	if (timer->enabled) {
>  		for (int i = 0; i < nr_timers(vcpu); i++)
>  			kvm_timer_update_irq(vcpu, false,
> -					     vcpu_get_timer(vcpu, i));
> +					vcpu_get_timer(vcpu, i));

Unrelated change, and a bad one at that!


>  
>  		if (irqchip_in_kernel(vcpu->kvm)) {
> -			kvm_vgic_reset_mapped_irq(vcpu, timer_irq(map.direct_vtimer));
> +			kvm_vgic_reset_mapped_irq(
> +				vcpu, timer_irq(map.direct_vtimer));

Also unrelated and not a good change.

>  			if (map.direct_ptimer)
> -				kvm_vgic_reset_mapped_irq(vcpu, timer_irq(map.direct_ptimer));
> +				kvm_vgic_reset_mapped_irq(
> +					vcpu,
> +					timer_irq(map.direct_ptimer));

Leave all these alone.

>  		}
>  	}
>  
> @@ -1092,10 +1140,19 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
>  		      HRTIMER_MODE_ABS_HARD);
>  }
>  
> +/*
> + * This is always called during kvm_arch_init_vm, but will also be
> + * called from kvm_vgic_create if we have a vGICv5.
> + */
>  void kvm_timer_init_vm(struct kvm *kvm)
>  {
> +	/*
> +	 * Set up the default PPIs - note that we adjust them based on
> +	 * the model of the GIC as GICv5 uses a different way to
> +	 * describing interrupts.
> +	 */
>  	for (int i = 0; i < NR_KVM_TIMERS; i++)
> -		kvm->arch.timer_data.ppi[i] = default_ppi[i];
> +		kvm->arch.timer_data.ppi[i] = get_vgic_ppi(kvm, default_ppi[i]);
>  }
>  
>  void kvm_timer_cpu_up(void)
> @@ -1347,6 +1404,7 @@ static int kvm_irq_init(struct arch_timer_kvm_info *info)
>  		}
>  
>  		arch_timer_irq_ops.flags |= VGIC_IRQ_SW_RESAMPLE;
> +		arch_timer_irq_ops_vgic_v5.flags |= VGIC_IRQ_SW_RESAMPLE;
>  		WARN_ON(irq_domain_push_irq(domain, host_vtimer_irq,
>  					    (void *)TIMER_VTIMER));
>  	}
> @@ -1497,10 +1555,13 @@ static bool timer_irqs_are_valid(struct kvm_vcpu *vcpu)
>  			break;
>  
>  		/*
> -		 * We know by construction that we only have PPIs, so
> -		 * all values are less than 32.
> +		 * We know by construction that we only have PPIs, so all values
> +		 * are less than 32 for non-GICv5 vgics. On GICv5, they are

VGICs maybe?  It's not consistent in existing comments in this file though.

> +		 * architecturally defined to be under 32 too. However, we mask
> +		 * off most of the bits as we might be presented with a GICv5
> +		 * style PPI where the type is encoded in the top-bits.
>  		 */
> -		ppis |= BIT(irq);
> +		ppis |= BIT(irq & 0x1f);
>  	}
>  
>  	valid = hweight32(ppis) == nr_timers(vcpu);
> @@ -1538,7 +1599,9 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>  {
>  	struct arch_timer_cpu *timer = vcpu_timer(vcpu);
>  	struct timer_map map;
> +	struct irq_ops *ops;
>  	int ret;
> +	int irq;
Might as well put irq on same line as ret

>  
>  	if (timer->enabled)
>  		return 0;
> @@ -1556,20 +1619,22 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>  		return -EINVAL;
>  	}
>  
> +	ops = vgic_is_v5(vcpu->kvm) ? &arch_timer_irq_ops_vgic_v5 :
> +				      &arch_timer_irq_ops;
> +
>  	get_timer_map(vcpu, &map);
>  
> -	ret = kvm_vgic_map_phys_irq(vcpu,
> -				    map.direct_vtimer->host_timer_irq,
> -				    timer_irq(map.direct_vtimer),
> -				    &arch_timer_irq_ops);
> +	irq = timer_irq(map.direct_vtimer);
> +	ret = kvm_vgic_map_phys_irq(vcpu, map.direct_vtimer->host_timer_irq,
> +				    irq, ops);

As irq is only used with this value in here, I'd avoid having the local variable
that changes meaning.

	ret = kvm_vgic_map_phys_irq(vcpu, map.direct_vtimer->host_timer_irq,
				    timer_irq(map.direct_vtimer), ops);
>  	if (ret)
>  		return ret;
>  
>  	if (map.direct_ptimer) {
> +		irq = timer_irq(map.direct_ptimer);
>  		ret = kvm_vgic_map_phys_irq(vcpu,
>  					    map.direct_ptimer->host_timer_irq,
> -					    timer_irq(map.direct_ptimer),
> -					    &arch_timer_irq_ops);
> +					    irq, ops);
As above
					    timer_irq(map.direct_ptimer), ops);

Doesn't make it much harder to read and avoids the local variable
being needed.
>  	}
>  
>  	if (ret)
> @@ -1627,6 +1692,15 @@ int kvm_arm_timer_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		goto out;
>  	}
>  
> +	/*
> +	 * The PPIs for the Arch Timers arch architecturally defined for
> +	 * GICv5. Reject anything that changes them from the specified value.
> +	 */
> +	if (vgic_is_v5(vcpu->kvm) && vcpu->kvm->arch.timer_data.ppi[idx] != irq) {
> +		ret = -EINVAL;
> +		goto out;

Whilst you are here, maybe throw some guard() magic dust at this and do a direct return?
Or leave it for someone else who has more spare time ;)

> +	}
> +
>  	/*
>  	 * We cannot validate the IRQ unicity before we run, so take it at
>  	 * face value. The verdict will be given on first vcpu run, for each

> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index 7310841f45121..6cb9c20f9db65 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h

>  
>  struct arch_timer_context {
> @@ -130,6 +132,9 @@ void kvm_timer_init_vhe(void);
>  #define timer_vm_data(ctx)		(&(timer_context_to_vcpu(ctx)->kvm->arch.timer_data))
>  #define timer_irq(ctx)			(timer_vm_data(ctx)->ppi[arch_timer_ctx_index(ctx)])
>  
> +#define get_vgic_ppi(k, i) (((k)->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V5) ? \
> +				(i) : ((i) | FIELD_PREP(GICV5_HWIRQ_TYPE, GICV5_HWIRQ_TYPE_PPI)))

Similar to earlier comment I'd use FIELD_PREP() for i as well but not that important
I'm just lazy about remembering where the numbers go.



>