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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <38722ba7-b7d9-368b-f946-b6c0c0a661b8@os.amperecomputing.com>
Date:   Tue, 19 Sep 2023 11:45:44 +0530
From:   Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     linux-kernel@...r.kernel.org, kvmarm@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org, Christoffer.Dall@....com,
        eauger@...hat.com, miguel.luis@...cle.com,
        darren@...amperecomputing.com, scott@...amperecomputing.com
Subject: Re: [PATCH v2 2/2] KVM: arm64: timers: Save restore CVAL of a ptimer
 across guest entry and exits



On 18-09-2023 04:59 pm, Marc Zyngier wrote:
> On Fri, 15 Sep 2023 10:57:46 +0100,
> Ganapatrao Kulkarni <gankulkarni@...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 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.

> 
> Thanks,
> 
> 	M.
> 

Thanks,
Ganapat