Messages in this thread | | | Date | Mon, 25 Sep 2023 11:05:03 +0530 | Subject | Re: [PATCH v2 2/2] KVM: arm64: timers: Save restore CVAL of a ptimer across guest entry and exits | From | Ganapatrao Kulkarni <> |
| |
On 24-09-2023 03:18 pm, Marc Zyngier wrote: > On Tue, 19 Sep 2023 07:15:44 +0100, > Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote: >> >> >> >> On 18-09-2023 04:59 pm, Marc Zyngier wrote: >>> On Fri, 15 Sep 2023 10:57:46 +0100, >>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> 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.
> > Thanks, > > M. >
Thanks, Ganapat
| |