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

[sashiko-bot@kernel.org]

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().

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

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.

>  }
>  
>  void kvm_timer_vcpu_load(struct kvm_vcpu *vcpu)

[ ... ]

> 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]
Will directly assigning the hardware value here clobber the saved in-memory
guest state for the broken ECV bit?

As noted in the comment above, NV2p1 makes CNTHCTL_EL2.ECV broken when
accessed via CNTKCTL_EL1. Bypassing the mask-and-merge step could permanently
destroy the guest's saved ECV configuration.

>  	}
>  
>  	__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]
By skipping this mask-and-merge step for NV2p1, won't this directly return
the broken hardware value to userspace?

This appears to hide the ECV bit from userspace entirely, which might break
live migration.

>  		case CPTR_EL2:
>  			if (cpus_have_final_cap(ARM64_HAS_NV2P1))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702160248.1377250-1-maz@kernel.org?part=10