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

[Ganapatrao Kulkarni]

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