[PATCH v2 0/3] KVM/arm64: timer fixes for 6.14

[PATCH v2 0/3] KVM/arm64: timer fixes for 6.14

[Marc Zyngier]

It's been recently pointed out that the NV timer code that is
(hopefully) on its way upstream is marginally sub-optimal (read:
terminally broken). This short series attempts to fix things:

- Correctly arm a background timer in all situations

- Avoid corrupting the EL2 state by shoving the EL1 state into it, and
  simplify the handling of emulated timers

- Drop the weird HV timer offset dance dance on E2H switch that never
  happens and make HV timer correctly UNDEF on E2H==0.

Thanks to Volodymyr Babchuk and Wei-Lin Chang for their help.

Patches on top of kvmarm/fixes, and merged back on top of my
kvm-arm64/nv-next branch.

Marc Zyngier (3):
  KVM: arm64: timer: Always evaluate the need for a soft timer
  KVM: arm64: timer: Correctly handle EL1 timer emulation when !FEAT_ECV
  KVM: arm64: timer: Don't adjust the EL2 virtual timer offset

 arch/arm64/kvm/arch_timer.c | 49 +++++++++----------------------------
 arch/arm64/kvm/sys_regs.c   | 16 +++++++++---
 2 files changed, 24 insertions(+), 41 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] KVM: arm64: timer: Always evaluate the need for a soft timer

[Marc Zyngier]

When updating the interrupt state for an emulated timer, we return
early and skip the setup of a soft timer that runs in parallel
with the guest.

While this is OK if we have set the interrupt pending, it is pretty
wrong if the guest moved CVAL into the future.  In that case,
no timer is armed and the guest can wait for a very long time
(it will take a full put/load cycle for the situation to resolve).

This is specially visible with EDK2 running at EL2, but still
using the EL1 virtual timer, which in that case is fully emulated.
Any key-press takes ages to be captured, as there is no UART
interrupt and EDK2 relies on polling from a timer...

The fix is simply to drop the early return. If the timer interrupt
is pending, we will still return early, and otherwise arm the soft
timer.

Fixes: 4d74ecfa6458b ("KVM: arm64: Don't arm a hrtimer for an already pending timer")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/arch_timer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index d3d243366536c..035e43f5d4f9a 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -471,10 +471,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
 
 	trace_kvm_timer_emulate(ctx, should_fire);
 
-	if (should_fire != ctx->irq.level) {
+	if (should_fire != ctx->irq.level)
 		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
-		return;
-	}
 
 	kvm_timer_update_status(ctx, should_fire);
 
-- 
2.39.2

Re: [PATCH v2 1/3] KVM: arm64: timer: Always evaluate the need for a soft timer

[Dmytro Terletskyi]

Hello, Marc.

On 2/4/25 13:00, Marc Zyngier wrote:
> When updating the interrupt state for an emulated timer, we return
> early and skip the setup of a soft timer that runs in parallel
> with the guest.
>
> While this is OK if we have set the interrupt pending, it is pretty
> wrong if the guest moved CVAL into the future.  In that case,
> no timer is armed and the guest can wait for a very long time
> (it will take a full put/load cycle for the situation to resolve).
>
> This is specially visible with EDK2 running at EL2, but still
> using the EL1 virtual timer, which in that case is fully emulated.
> Any key-press takes ages to be captured, as there is no UART
> interrupt and EDK2 relies on polling from a timer...
>
> The fix is simply to drop the early return. If the timer interrupt
> is pending, we will still return early, and otherwise arm the soft
> timer.
>
> Fixes: 4d74ecfa6458b ("KVM: arm64: Don't arm a hrtimer for an already pending timer")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org


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


> ---
>   arch/arm64/kvm/arch_timer.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index d3d243366536c..035e43f5d4f9a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -471,10 +471,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>   
>   	trace_kvm_timer_emulate(ctx, should_fire);
>   
> -	if (should_fire != ctx->irq.level) {
> +	if (should_fire != ctx->irq.level)
>   		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
> -		return;
> -	}
>   
>   	kvm_timer_update_status(ctx, should_fire);
>   

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

[Marc Zyngier]

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
---
 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);
-- 
2.39.2

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

[Dmytro Terletskyi]

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);

[PATCH v2 3/3] KVM: arm64: timer: Don't adjust the EL2 virtual timer offset

[Marc Zyngier]

The way we deal with the EL2 virtual timer is a bit odd.

We try to cope with E2H being flipped, and adjust which offset
applies to that timer depending on the current E2H value. But that's
a complexity we shouldn't have to worry about.

What we have to deal with is either E2H being RES1, in which case
there is no offset, or E2H being RES0, and the virtual timer simply
does not exist.

Drop the adjusting of the timer offset, which makes things a bit
simpler. At the same time, make sure that accessing the HV timer
when E2H is RES0 results in an UNDEF in the guest.

Suggested-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/arch_timer.c | 15 ---------------
 arch/arm64/kvm/sys_regs.c   | 16 +++++++++++++---
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index e59836e0260cf..231c0cd9c7b4b 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -759,21 +759,6 @@ static void kvm_timer_vcpu_load_nested_switch(struct kvm_vcpu *vcpu,
 					    timer_irq(map->direct_ptimer),
 					    &arch_timer_irq_ops);
 		WARN_ON_ONCE(ret);
-
-		/*
-		 * The virtual offset behaviour is "interesting", as it
-		 * always applies when HCR_EL2.E2H==0, but only when
-		 * accessed from EL1 when HCR_EL2.E2H==1. So make sure we
-		 * track E2H when putting the HV timer in "direct" mode.
-		 */
-		if (map->direct_vtimer == vcpu_hvtimer(vcpu)) {
-			struct arch_timer_offset *offs = &map->direct_vtimer->offset;
-
-			if (vcpu_el2_e2h_is_set(vcpu))
-				offs->vcpu_offset = NULL;
-			else
-				offs->vcpu_offset = &__vcpu_sys_reg(vcpu, CNTVOFF_EL2);
-		}
 	}
 }
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 526d66f24e34a..7968bee0d27ea 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1452,6 +1452,16 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool access_hv_timer(struct kvm_vcpu *vcpu,
+			    struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	if (!vcpu_el2_e2h_is_set(vcpu))
+		return undef_access(vcpu, p, r);
+
+	return access_arch_timer(vcpu, p, r);
+}
+
 static s64 kvm_arm64_ftr_safe_value(u32 id, const struct arm64_ftr_bits *ftrp,
 				    s64 new, s64 cur)
 {
@@ -3099,9 +3109,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	EL2_REG(CNTHP_CTL_EL2, access_arch_timer, reset_val, 0),
 	EL2_REG(CNTHP_CVAL_EL2, access_arch_timer, reset_val, 0),
 
-	{ SYS_DESC(SYS_CNTHV_TVAL_EL2), access_arch_timer },
-	EL2_REG(CNTHV_CTL_EL2, access_arch_timer, reset_val, 0),
-	EL2_REG(CNTHV_CVAL_EL2, access_arch_timer, reset_val, 0),
+	{ SYS_DESC(SYS_CNTHV_TVAL_EL2), access_hv_timer },
+	EL2_REG(CNTHV_CTL_EL2, access_hv_timer, reset_val, 0),
+	EL2_REG(CNTHV_CVAL_EL2, access_hv_timer, reset_val, 0),
 
 	{ SYS_DESC(SYS_CNTKCTL_EL12), access_cntkctl_el12 },
 
-- 
2.39.2

Re: [PATCH v2 0/3] KVM/arm64: timer fixes for 6.14

[Oliver Upton]

On Tue, Feb 04, 2025 at 11:00:47AM +0000, Marc Zyngier wrote:
> It's been recently pointed out that the NV timer code that is
> (hopefully) on its way upstream is marginally sub-optimal (read:
> terminally broken). This short series attempts to fix things:
> 
> - Correctly arm a background timer in all situations
> 
> - Avoid corrupting the EL2 state by shoving the EL1 state into it, and
>   simplify the handling of emulated timers
> 
> - Drop the weird HV timer offset dance dance on E2H switch that never
>   happens and make HV timer correctly UNDEF on E2H==0.
> 
> Thanks to Volodymyr Babchuk and Wei-Lin Chang for their help.
> 
> Patches on top of kvmarm/fixes, and merged back on top of my
> kvm-arm64/nv-next branch.

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

-- 
Thanks,
Oliver

Re: [PATCH v2 0/3] KVM/arm64: timer fixes for 6.14

[Marc Zyngier]

On Tue, 04 Feb 2025 11:00:47 +0000, Marc Zyngier wrote:
> It's been recently pointed out that the NV timer code that is
> (hopefully) on its way upstream is marginally sub-optimal (read:
> terminally broken). This short series attempts to fix things:
> 
> - Correctly arm a background timer in all situations
> 
> - Avoid corrupting the EL2 state by shoving the EL1 state into it, and
>   simplify the handling of emulated timers
> 
> [...]

Applied to fixes, thanks!

[1/3] KVM: arm64: timer: Always evaluate the need for a soft timer
      commit: b450dcce93bc2cf6d2bfaf5a0de88a94ebad8f89
[2/3] KVM: arm64: timer: Correctly handle EL1 timer emulation when !FEAT_ECV
      commit: 1b8705ad5365b5333240b46d5cd24e88ef2ddb14
[3/3] KVM: arm64: timer: Don't adjust the EL2 virtual timer offset
      commit: 0e459810285503fb354537e84049e212c5917c33

Cheers,

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