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: <87v8bz52bq.wl-maz@kernel.org>
Date:   Sun, 24 Sep 2023 10:48:41 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>
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 Tue, 19 Sep 2023 07:15:44 +0100,
Ganapatrao Kulkarni <gankulkarni@...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@...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.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.