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 <[email protected]> wrote:
>>>
>>>
>>>
>>> On 18-09-2023 04:59 pm, Marc Zyngier wrote:
>>>> On Fri, 15 Sep 2023 10:57:46 +0100,
>>>> Ganapatrao Kulkarni <[email protected]> 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 <[email protected]>
>>
>> Thanks,
>>
>> M.
>>
>
> Thanks,
> Ganapat
Thanks,
Ganapat
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 <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 18-09-2023 04:59 pm, Marc Zyngier wrote:
>>>>> On Fri, 15 Sep 2023 10:57:46 +0100,
>>>>> Ganapatrao Kulkarni <[email protected]> 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 <[email protected]>
>
Are you planning to take this patch as part of NV-V11 or going as fix
patch to next?
Thanks,
Ganapat