Re: [PATCH 10/28] KVM: arm64: Relax CNTHCTL_EL2 handling when FEAT_NV2p1 is present

[Marc Zyngier <maz@kernel.org>]

On Thu, 02 Jul 2026 17:21:31 +0100,
sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Unconditional use of the CNTKCTL_EL1 hardware alias under FEAT_NV2p1 clobbers the CNTHCTL_EL2.ECV bit for both the host and the guest.
> 
> Pre-existing issues:
> - [High] Writes to the control-plane system registers CNTHCTL_EL2 and CNTKCTL_EL1 are missing a mandatory ISB instruction.
> --
> 
> commit 71cc9698fb316a6dc28bed5d5465cd03b6074925
> Author: Marc Zyngier <maz@kernel.org>
> 
> KVM: arm64: Relax CNTHCTL_EL2 handling when FEAT_NV2p1 is present
> 
> This patch optimizes CNTHCTL_EL2 accesses when FEAT_NV2p1 is present by
> treating the CNTKCTL_EL1 accessor as normal. It replaces the split approach
> with direct use of the CNTKCTL_EL1 hardware alias.
> 
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index 4155fe89b58a1..db60facad9f3c 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> 
> [ ... ]
> 
> > @@ -876,8 +876,14 @@ static void timer_set_traps(struct kvm_vcpu *vcpu, struct timer_map *map)
> >  	assign_clear_set_bit(tvt02, CNTHCTL_EL1NVVCT, clr, set);
> >  	assign_clear_set_bit(tpt02, CNTHCTL_EL1NVPCT, clr, set);
> >  
> > -	/* This only happens on VHE, so use the CNTHCTL_EL2 accessor. */
> > -	sysreg_clear_set(cnthctl_el2, clr, set);
> > +	/*
> > +	 * This only happens on VHE, so use the CNTHCTL_EL2 accessor, unless
> > +	 * we are sure CNTKCTL_EL1 is completely stateful with FEAT_NV2p1.
> > +	 */
> > +	if (!cpus_have_final_cap(ARM64_HAS_NV2P1))
> > +		sysreg_clear_set(cnthctl_el2, clr, set);
> > +	else
> > +		sysreg_clear_set(cntkctl_el1, clr, set);
> 
> [Severity: High]
> Does this read-modify-write on the host's physical state using the
> cntkctl_el1 alias inadvertently clear KVM's own physical CNTHCTL_EL2.ECV bit?
> 
> Since the alias drops or mishandles the ECV bit, reading it might return ECV
> as 0 and write it back as 0. This could silently disable the host's ECV
> functionality, such as CNTPOFF initialization from kvm_timer_init_vhe().

No, and that's the whole point of NV2p1: the CNTKCTL_EL1 bits are
guaranteed to be stateful, and we don't need to do anything else

> [Severity: High]
> This is a pre-existing issue, but shouldn't there be an isb() instruction
> immediately following these control-plane system register writes?

"control-plane"? WTF???

And no. These bits only need to take effect when entering the guest,
for which we already have a context synchronisation event.

> Writing to CNTHCTL_EL2 or CNTKCTL_EL1 without an explicit context
> synchronization event can cause the CPU to operate under a stale timer trap
> configuration for several cycles before returning.

Bollocks.

	M.

-- 
Jazz isn't dead. It just smells funny.