Re: [PATCH] KVM: arm64: nv: Set ISTATUS for emulated timers, If timer expired

[Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>]

> Please give the fixup below a go.
> 
> 	M.
> 
> [1] https://lore.kernel.org/all/20241202172134.384923-6-maz@kernel.org/
> 
>  From 2bbd6f9b41a20ad573376c20c158ff3c12db5009 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Mon, 9 Dec 2024 10:58:08 +0000
> Subject: [PATCH] fixup! KVM: arm64: nv: Publish emulated timer interrupt state
>   in the in-memory state
> 
> ---
>   arch/arm64/kvm/arch_timer.c | 32 +++++++++++++-------------------
>   1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 895f09658ef83..91bda986c344b 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -432,25 +432,6 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>   {
>   	int ret;
>   
> -	/*
> -	 * Paper over NV2 brokenness by publishing the interrupt status
> -	 * bit. This still results in a poor quality of emulation (guest
> -	 * writes will have no effect until the next exit).
> -	 *
> -	 * But hey, it's fast, right?
> -	 */
> -	if (is_hyp_ctxt(vcpu) &&
> -	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
> -		u32 ctl = timer_get_ctl(timer_ctx);
> -
> -		if (new_level)
> -			ctl |= ARCH_TIMER_CTRL_IT_STAT;
> -		else
> -			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
> -
> -		timer_set_ctl(timer_ctx, ctl);
> -	}
> -
>   	timer_ctx->irq.level = new_level;
>   	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_irq(timer_ctx),
>   				   timer_ctx->irq.level);
> @@ -471,6 +452,19 @@ static void timer_emulate(struct arch_timer_context *ctx)
>   
>   	trace_kvm_timer_emulate(ctx, should_fire);
>   
> +	/*
> +	 * Paper over NV2 brokenness by publishing the interrupt status
> +	 * bit. This still results in a poor quality of emulation (guest
> +	 * writes will have no effect until the next exit).
> +	 *
> +	 * But hey, it's fast, right?
> +	 */
> +	if (is_hyp_ctxt(ctx->vcpu)) {
> +		unsigned long val = timer_get_ctl(ctx);
> +		__assign_bit(__ffs(ARCH_TIMER_CTRL_IT_STAT), &val, should_fire);
> +		timer_set_ctl(ctx, val);
> +	}
> +
>   	if (should_fire != ctx->irq.level) {
>   		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
>   		return;

I tried this patch and it did not work, looks like there are some 
exit/entry paths which updates timer interrupt without going though 
timer_emulate (like kvm_hrtimer_expire). If I undo the deleted lines in 
above diff in the function kvm_timer_update_irq, then it is working.

below is the diff which is working.

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index dd038d62e99b..a54bdb6dc566 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -432,23 +432,11 @@ static void kvm_timer_update_irq(struct kvm_vcpu 
*vcpu, bool new_level,
  {
         int ret;

-       /*
-        * Paper over NV2 brokenness by publishing the interrupt status
-        * bit. This still results in a poor quality of emulation (guest
-        * writes will have no effect until the next exit).
-        *
-        * But hey, it's fast, right?
-        */
         if (is_hyp_ctxt(vcpu) &&
             (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == 
vcpu_ptimer(vcpu))) {
-               u32 ctl = timer_get_ctl(timer_ctx);
-
-               if (new_level)
-                       ctl |= ARCH_TIMER_CTRL_IT_STAT;
-               else
-                       ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
-
-               timer_set_ctl(timer_ctx, ctl);
+               unsigned long val = timer_get_ctl(timer_ctx);
+               __assign_bit(__ffs(ARCH_TIMER_CTRL_IT_STAT), &val, 
new_level);
+               timer_set_ctl(timer_ctx, val);
         }

         timer_ctx->irq.level = new_level;
@@ -471,6 +459,19 @@ static void timer_emulate(struct arch_timer_context 
*ctx)

         trace_kvm_timer_emulate(ctx, should_fire);

+       /*
+        * Paper over NV2 brokenness by publishing the interrupt status
+        * bit. This still results in a poor quality of emulation (guest
+        * writes will have no effect until the next exit).
+        *
+        * But hey, it's fast, right?
+        */
+       if (is_hyp_ctxt(ctx->vcpu)) {
+               unsigned long val = timer_get_ctl(ctx);
+               __assign_bit(__ffs(ARCH_TIMER_CTRL_IT_STAT), &val, 
should_fire);
+               timer_set_ctl(ctx, val);
+       }
+
         if (should_fire != ctx->irq.level) {
                 kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
                 return;
@@ -948,9 +949,6 @@ 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 (cpus_have_final_cap(ARM64_HAS_ECV) || !is_hyp_ctxt(vcpu))
-               return;
-
         if (!vcpu_el2_e2h_is_set(vcpu)) {
                 /*
                  * A non-VHE guest hypervisor doesn't have any direct 
access
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 2c35e1f8193e..52a263e7dcca 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1238,7 +1238,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
                 if (static_branch_unlikely(&userspace_irqchip_in_use))
                         kvm_timer_sync_user(vcpu);

-               if (vcpu_has_nv(vcpu))
+               if (is_hyp_ctxt(vcpu))
                         kvm_timer_sync_nested(vcpu);

                 kvm_arch_vcpu_ctxsync_fp(vcpu);


-- 
Thanks,
Ganapat/GK