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

[Marc Zyngier <maz@kernel.org>]

On Tue, 14 Jul 2026 10:53:49 +0100,
sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Removing the split-state preservation for `CNTHCTL_EL2` on FEAT_NV2p1 systems permanently corrupts the guest's in-memory ECV bit and leaks host-specific trap overrides to userspace.
> --
> 
> --- Patch [10]: [PATCH v2 10/28] KVM: arm64: Relax CNTHCTL_EL2 handling when FEAT_NV2p1 is present ---
> commit e272e43dc6ce85d3337902d78430aad6bf5b83fc
> Author: Marc Zyngier <maz@kernel.org>
> 
> KVM: arm64: Relax CNTHCTL_EL2 handling when FEAT_NV2p1 is present
>     
> With NV2p1, it is no longer necessary to use the split approach
> where bits of CNTHCTL_EL2 cannot be accessed via CNTKCTL_EL1,
> and we can treat the CNTKCTL_EL1 accessor as if it was "normal".
> 
> > 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);
> >  }
> 
> [ ... ]
> 
> > diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> > index 6f0f046e4ca4e..0c4ef1ce32ae7 100644
> > --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> > +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> 
> [ ... ]
> 
> > @@ -69,11 +69,18 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
> >  		 * The EL1 view of CNTKCTL_EL1 has a bunch of RES0 bits where
> >  		 * the interesting CNTHCTL_EL2 bits live. So preserve these
> >  		 * bits when reading back the guest-visible value.
> > +		 *
> > +		 * While NV2p1 fixes some of that, it makes CNTHCTL_EL2.ECV
> > +		 * even more broken than it already was with NV2.
> >  		 */
> >  		val = read_sysreg_el1(SYS_CNTKCTL);
> > -		val &= CNTKCTL_VALID_BITS;
> > -		__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, &=, ~CNTKCTL_VALID_BITS);
> > -		__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, |=, val);
> > +		if (!cpus_have_final_cap(ARM64_HAS_NV2P1)) {
> > +			val &= CNTKCTL_VALID_BITS;
> > +			__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, &=, ~CNTKCTL_VALID_BITS);
> > +			__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, |=, val);
> > +		} else {
> > +			__vcpu_assign_sys_reg(vcpu, CNTHCTL_EL2, val);
> > +		}
> 
> [Severity: High]
> Does removing the split-state preservation for CNTHCTL_EL2 unconditionally
> overwrite the guest's in-memory register state?

How?

> 
> Since timer_set_traps() applies host-specific overrides to CNTKCTL_EL1 on the
> host side, calling __vcpu_assign_sys_reg() here appears to blindly copy those
> host-specific traps directly into the guest's architectural state on exit.

No. CNTKCTL_EL1 is not CNTKCTL_EL12. Not only you don't understand NV,
you don't even understand VHE.

>
> Furthermore, as the comment above notes that NV2p1 makes the ECV bit even more
> broken, won't this assignment corrupt the guest's ECV state as well?
> 
> >  	}
> >  
> >  	__vcpu_assign_sys_reg(vcpu, SP_EL2,	 read_sysreg(sp_el1));
> 
> [ ... ]
> 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 1dfc1f88bec82..9439c5b2b1fe8 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> 
> [ ... ]
> 
> > @@ -322,8 +322,10 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg reg)
> >  		switch (reg) {
> >  		case CNTHCTL_EL2:
> >  			val = read_sysreg_el1(SYS_CNTKCTL);
> > -			val &= CNTKCTL_VALID_BITS;
> > -			val |= __vcpu_sys_reg(vcpu, reg) & ~CNTKCTL_VALID_BITS;
> > +			if (!cpus_have_final_cap(ARM64_HAS_NV2P1)) {
> > +				val &= CNTKCTL_VALID_BITS;
> > +				val |= __vcpu_sys_reg(vcpu, reg) & ~CNTKCTL_VALID_BITS;
> > +			}
> >  			return val;
> 
> [Severity: High]
> Does this directly return the host-polluted physical register to userspace
> during KVM_GET_ONE_REG?
> 
> Because the host-specific overrides from timer_set_traps() are present in
> CNTKCTL_EL1, skipping the bitmasking on ARM64_HAS_NV2P1 systems seems to leak
> these hypervisor-specific configurations to userspace. This could break live
> migration by restoring an altered execution environment.

CNTKCTL_EL1 != CNTKCTL_EL12.

	M.

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