Re: [PATCH v2 2/3] KVM: arm64: timer: Correctly handle EL1 timer emulation when !FEAT_ECV

[Dmytro Terletskyi <Dmytro_Terletskyi@epam.com>]

Hello, Marc.

On 2/4/25 13:00, Marc Zyngier wrote:
> Both Wei-Lin Chang and Volodymyr Babchuk report that the way we
> handle the emulation of EL1 timers with NV is completely wrong,
> specially in the case of HCR_EL2.E2H==0.
>
> There are three problems in about as many lines of code:
>
> - With E2H==0, the EL1 timers are overwritten with the EL1 state,
>    while they should actually contain the EL2 state (as per the timer
>    map)
>
> - With E2H==1, we run the full EL1 timer emulation even when ECV
>    is present, hiding a bug in timer_emulate() (see previous patch)
>
> - The comments are actively misleading, and say all the wrong things.
>
> This is only attributable to the code having been initially written
> for FEAT_NV, hacked up to handle FEAT_NV2 *in parallel*, and vaguely
> hacked again to be FEAT_NV2 only. Oh, and yours truly being a gold
> plated idiot.
>
> The fix is obvious: just delete most of the E2H==0 code, have a unified
> handling of the timers (because they really are E2H agnostic), and
> make sure we don't execute any of that when FEAT_ECV is present.
>
> Fixes: 4bad3068cfa9f ("KVM: arm64: nv: Sync nested timer state with FEAT_NV2")
> Reported-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw>
> Reported-by: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Link: https://lore.kernel.org/r/fqiqfjzwpgbzdtouu2pwqlu7llhnf5lmy4hzv5vo6ph4v3vyls@jdcfy3fjjc5k
> Link: https://lore.kernel.org/r/87frl51tse.fsf@epam.com


Tested-by: Dmytro Terletskyi <dmytro_terletskyi@epam.com>


> ---
>   arch/arm64/kvm/arch_timer.c | 30 ++++++++++--------------------
>   1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 035e43f5d4f9a..e59836e0260cf 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -974,31 +974,21 @@ void kvm_timer_sync_nested(struct kvm_vcpu *vcpu)
>   	 * which allows trapping of the timer registers even with NV2.
>   	 * Still, this is still worse than FEAT_NV on its own. Meh.
>   	 */
> -	if (!vcpu_el2_e2h_is_set(vcpu)) {
> -		if (cpus_have_final_cap(ARM64_HAS_ECV))
> -			return;
> -
> -		/*
> -		 * A non-VHE guest hypervisor doesn't have any direct access
> -		 * to its timers: the EL2 registers trap (and the HW is
> -		 * fully emulated), while the EL0 registers access memory
> -		 * despite the access being notionally direct. Boo.
> -		 *
> -		 * We update the hardware timer registers with the
> -		 * latest value written by the guest to the VNCR page
> -		 * and let the hardware take care of the rest.
> -		 */
> -		write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTV_CTL_EL0),  SYS_CNTV_CTL);
> -		write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTV_CVAL_EL0), SYS_CNTV_CVAL);
> -		write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTP_CTL_EL0),  SYS_CNTP_CTL);
> -		write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTP_CVAL_EL0), SYS_CNTP_CVAL);
> -	} else {
> +	if (!cpus_have_final_cap(ARM64_HAS_ECV)) {
>   		/*
>   		 * For a VHE guest hypervisor, the EL2 state is directly
> -		 * stored in the host EL1 timers, while the emulated EL0
> +		 * stored in the host EL1 timers, while the emulated EL1
>   		 * state is stored in the VNCR page. The latter could have
>   		 * been updated behind our back, and we must reset the
>   		 * emulation of the timers.
> +		 *
> +		 * A non-VHE guest hypervisor doesn't have any direct access
> +		 * to its timers: the EL2 registers trap despite being
> +		 * notionally direct (we use the EL1 HW, as for VHE), while
> +		 * the EL1 registers access memory.
> +		 *
> +		 * In both cases, process the emulated timers on each guest
> +		 * exit. Boo.
>   		 */
>   		struct timer_map map;
>   		get_timer_map(vcpu, &map);