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

[Ganapatrao Kulkarni]

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

> Thanks,
>
> 	M.

Thanks,
Ganapat