Re: [PATCH v2 2/2] KVM: arm64: timers: Save restore CVAL of a ptimer across guest entry and exits

From: Ganapatrao Kulkarni
Date: Tue Oct 10 2023 - 00:55:21 EST



Hi Marc,

On 25-09-2023 11:10 am, Ganapatrao Kulkarni wrote:


On 25-09-2023 11:05 am, Ganapatrao Kulkarni wrote:


On 24-09-2023 03:18 pm, Marc Zyngier wrote:
On Tue, 19 Sep 2023 07:15:44 +0100,
Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx> wrote:



On 18-09-2023 04:59 pm, Marc Zyngier wrote:
On Fri, 15 Sep 2023 10:57:46 +0100,
Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx> wrote:

This patch did not work.
After adding changes as in below diff, it is started working.

Thanks for looking into this.


diff --git a/arch/arm64/kvm/hyp/vhe/switch.c
b/arch/arm64/kvm/hyp/vhe/switch.c
index b0b07658f77d..91d2cfb03e26 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -117,7 +117,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
                          val = __vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2);

                  if (map.direct_ptimer) {
-                       write_sysreg_s(val, SYS_CNTP_CVAL_EL0);
+                       write_sysreg_el0(val, SYS_CNTP_CVAL);

Duh, of course. Silly me.

                          isb();
                  }
          }
@@ -161,8 +161,6 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)

          ___deactivate_traps(vcpu);

-       write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
-
          if (has_cntpoff()) {
                  struct timer_map map;
                  u64 val, offset;
@@ -173,7 +171,7 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
                   * We're exiting the guest. Save the latest CVAL value
                   * to memory and apply the offset now that TGE is set.
                   */
-               val = read_sysreg_s(SYS_CNTP_CVAL_EL0);
+               val = read_sysreg_el0(SYS_CNTP_CVAL);
                  if (map.direct_ptimer == vcpu_ptimer(vcpu))
                          __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0) = val;
                  if (map.direct_ptimer == vcpu_hptimer(vcpu))
@@ -182,12 +180,13 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
                  offset = read_sysreg_s(SYS_CNTPOFF_EL2);

                  if (map.direct_ptimer && offset) {
-                       offset = read_sysreg_s(SYS_CNTPOFF_EL2);
-                       write_sysreg_s(val + offset, SYS_CNTP_CVAL_EL0);
+                       write_sysreg_el0(val + offset, SYS_CNTP_CVAL);
                          isb();
                  }
          }

+       write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);

Why moving the HCR_EL2 update? I don't grok what it changes. Or is it

This the line of code which flips the TGE and making timer cval ready
to handle the TGE flip is more safe way(avoids even corner case of
false interrupt triggers) than changing after the flipping?

That's pretty dubious. Do you actually see it firing on your HW?


that you end-up with spurious interrupts because your GIC is slow to
retire interrupts that are transiently pending?

IIUC, If there are any transient interrupts or asserted already,
anyway they will be handled when irq is unmasked.

That's the idea. But my question is whether you observe spurious
interrupts when not changing the ordering.

I tried with keeping the ordering (i.e flip TGE then change cval) and i don't see any issue as such. IMO, it is better to have cval updated before TGE flip, anyway either way works for us.


Please feel free to add,
Tested-by: Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx>


Are you planning to take this patch as part of NV-V11 or going as fix patch to next?

Thanks,
Ganapat