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

Ganapatrao Kulkarni
2023-09-25T05:35:26Z

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