Re: [RFC PATCH] KVM: arm64: Restrict CNTP_CVAL/CNTHP_CVAL save/reload when ptimer is used

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: arm64: ptimer_used flag breaks physical timer after live migration
- [High] kvm: arm64: hardcoded cnthctl_el2 write breaks timer traps on NV2p1
--

commit a19fd66b3c0094c0f95419be8e827cd25f55a439
Author: Zhou Wang <wangzhou1@hisilicon.com>

KVM: arm64: Restrict CNTP_CVAL/CNTHP_CVAL save/reload when ptimer is used

This patch introduces a lazy detection mechanism to determine if a
non-nested VHE guest actually uses the physical timer, avoiding useless
timer state saves and reloads. It initially traps CNTP_CTL_EL0 writes
and clears the trap on the first guest enable, granting direct access.

> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 6ac3321f4c575..fcbd1e133ad0d 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
[ ... ]
> @@ -1054,6 +1058,7 @@ void kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu)
>  		offs->vm_offset = &vcpu->kvm->arch.timer_data.poffset;
>  	}
>  
> +	timer->ptimer_used = false;
>  	if (timer->enabled) {
>  		for (int i = 0; i < nr_timers(vcpu); i++)
>  			kvm_timer_update_irq(vcpu, false,
[ ... ]
> @@ -1270,6 +1275,16 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
>  		preempt_disable();
>  		timer_save_state(timer);
>  		kvm_arm_timer_write(vcpu, timer, treg, val);
> +
> +		/* Lazy ptimer detection: catch first CNTP_CTL enable */
> +		if (has_cntpoff() && !vcpu_has_nv(vcpu) &&
> +		    tmr == TIMER_PTIMER && treg == TIMER_REG_CTL &&
> +		    (val & ARCH_TIMER_CTRL_ENABLE) &&
> +		    !vcpu_timer(vcpu)->ptimer_used) {
> +			vcpu_timer(vcpu)->ptimer_used = true;

[Severity: High]
Could this cause the physical timer to break after a live migration?

When a guest enables the physical timer, kvm_arm_timer_write_sysreg()
sets ptimer_used to true. However, during a live migration, userspace
restores state via KVM_SET_ONE_REG, which bypasses this sysreg write
path.

Because kvm_timer_vcpu_reset() initializes ptimer_used to false, it
appears the destination host will permanently leave ptimer_used as
false. Consequently, __activate_traps() in
arch/arm64/kvm/hyp/vhe/switch.c will skip writing the un-offset guest
CVAL to the hardware SYS_CNTP_CVAL. This leaves the hardware retaining
the offset-shifted value, preventing the interrupt from firing at the
correct guest time.

> +			sysreg_clear_set(cnthctl_el2, 0,
> +					 CNTHCTL_EL1PCEN << 10);

[Severity: High]
Does this unconditionally write to the wrong register on hardware with
NV2p1?

If a host is equipped with FEAT_NV2p1 and FEAT_ECV, timer_set_traps() in
arch/arm64/kvm/arch_timer.c correctly checks for ARM64_HAS_NV2P1 and
uses cntkctl_el1 instead of cnthctl_el2 for EL1 trap configuration.

This unconditional write to cnthctl_el2 seems to ignore the NV2p1
hardware requirement. This might fail to clear the trap in hardware
and could potentially corrupt unrelated timer control state in
CNTHCTL_EL2.

> +		}
>  		timer_restore_state(timer);
>  		preempt_enable();
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921111840.3000905-1-wangzhou1@hisilicon.com?part=1