Messages in this thread | | | Date | Tue, 10 Oct 2023 10:24:34 +0530 | Subject | Re: [PATCH v2 2/2] KVM: arm64: timers: Save restore CVAL of a ptimer across guest entry and exits | From | 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@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. >> > > Please feel free to add, > Tested-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> >
Are you planning to take this patch as part of NV-V11 or going as fix patch to next?
Thanks, Ganapat
| |