Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 22 Aug 2023 13:16:36 +0100
Message-ID: <86il97ff17.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
Cc:     linux-kernel@vger.kernel.org, kvmarm@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, eauger@redhat.com,
        miguel.luis@oracle.com, darren@os.amperecomputing.com,
        scott@os.amperecomputing.com,
        Christoffer Dall <Christoffer.Dall@arm.com>
Subject: Re: [PATCH 2/2] KVM: arm64: timers: Adjust CVAL of a ptimer across guest entry and exits
In-Reply-To: <0c5fb304-8c69-80c3-6f1e-487828554244@os.amperecomputing.com>
References: <20230817060314.535987-1-gankulkarni@os.amperecomputing.com>
        <20230817060314.535987-3-gankulkarni@os.amperecomputing.com>
        <87bkf6oyyt.wl-maz@kernel.org>
        <0c5fb304-8c69-80c3-6f1e-487828554244@os.amperecomputing.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, 17 Aug 2023 10:27:55 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>=20
>=20
> Hi Marc,
>=20
> On 17-08-2023 01:57 pm, Marc Zyngier wrote:
> > [Fixing Christoffer's email address]
>=20
> Thanks.
> >=20
> > On Thu, 17 Aug 2023 07:03:14 +0100,
> > Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> >>=20
> >> As per FEAT_ECV, when HCR_EL2.{E2H, TGE} =3D=3D {1, 1}, Enhanced Count=
er
> >> Virtualization functionality is disabled and CNTPOFF_EL2 value is trea=
ted
> >> as zero. On VHE host, E2H and TGE are set, hence it is required
> >> to adjust CVAL by incrementing it by CNTPOFF_EL2 after guest
> >> exit to avoid false physical timer interrupts and also
> >> decrement/restore CVAL before the guest entry.
> >=20
> > No, this is wrong. Neither E2H nor TGE have any impact on writing to
> > CNTPOFF_EL2, nor does it have an impact on CNTP_CVAL_EL0. Just read
> > the pseudocode to convince yourself.
> >=20
> > CNTPOFF_EL2 is applied at exactly two points: when SW is reading
> > CNTPCT_EL0 from EL1 while {E2H,TGE}=3D=3D{1, 0} and when the HW is
> > comparing CNTPCT_EL0 with the CNTP_CVAL_EL0. In both cases the offset
> > is subtracted from the counter. And that's the point where the running
> > EL matters. Which means that CNTPOFF_EL2 behaves exactly like
> > CNTVOFF_EL2. No ifs, no buts.
>=20
> As per ARM ARM (ARM DDI 0487J.a page D11-5989)
> "When FEAT_ECV is implemented, the CNTPOFF_EL2 register allows an
> offset to be applied to the physical counter, as viewed from EL1 and
> EL0, and to the EL1 physical timer. The functionality of this 64-bit
> register is affected by CNTHCTL_EL2.ECV."
>=20
> As per ARM ARM (ARM DDI 0487J.a page D19-7857)
> "When HCR_EL2.{E2H, TGE} =3D=3D {1, 1} or SCR_EL3.{NS, EEL2} =3D=3D {0, 0=
}, then
> Enhanced Counter Virtualization functionality is disabled."
>=20
> "The EL1 physical timer interrupt is triggered when ((PCount<63:0> -
> CNTPOFF_EL2<63:0>) - PCVal<63:0>) is greater than or equal to 0."
>=20
> As per ARM ARM (ARM DDI 0487J.a page D19-7938)
> "When EL2 is implemented and enabled in the current Security state,
> the physical counter uses a fixed physical offset of *zero* if any of
> the following are true:
> =E2=80=A2 CNTHCTL_EL2.ECV is 0.
> =E2=80=A2 SCR_EL3.ECVEn is 0.
> =E2=80=A2 HCR_EL2.{E2H, TGE} is {1, 1}."
>=20
> In VHE host hypervisor, E2H=3DTGE=3D1 hence ECV is disabled and Ptimer
> interrupt is triggered based on PCount<63:0> - PCVal<63:0>
>=20
> Since cval is set by Guest as per offsetted PCounter value and pCount
> is not subtracted by CNTPOFF when in VHE-L0, results in cval becoming
> much lesser than physical counter(bumped up since CNTPOFF is zero) and
> timer interrupt trigger condition is met falsely.
>=20
> There is no issue/impact on cval due to ECV, however it can be/is
> manipulated to handle this on and off of CNTPOFF/ECV.
>=20
> IIUC, CNTPOFF and CNTVOFF are not same as per specification.

I owe you an apology. You are correct, and I was totally wrong.  I'm
truly amazed how wrong we got this part of the architecture, but it is
way too late for any change, and we'll have to live with it.

Now, to the actual patch: I think the way you offset CVAL isn't
great. You should never have to change it on entry, and you should
instead read the correct value from memory. Then, save/restore of CVAL
must be amended to always apply the offset. Can you give the hack
below a go on your HW?

Thanks,

	M.

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index ea46b4e1e7a8..bb80fdd84676 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -55,11 +55,6 @@ static struct irq_ops arch_timer_irq_ops =3D {
 	.get_input_level =3D kvm_arch_timer_get_input_level,
 };
=20
-static bool has_cntpoff(void)
-{
-	return (has_vhe() && cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF));
-}
-
 static int nr_timers(struct kvm_vcpu *vcpu)
 {
 	if (!vcpu_has_nv2(vcpu))
@@ -180,7 +175,7 @@ u64 kvm_phys_timer_read(void)
 	return timecounter->cc->read(timecounter->cc);
 }
=20
-static void get_timer_map(struct kvm_vcpu *vcpu, struct timer_map *map)
+void get_timer_map(struct kvm_vcpu *vcpu, struct timer_map *map)
 {
 	if (vcpu_has_nv2(vcpu)) {
 		if (is_hyp_ctxt(vcpu)) {
@@ -569,8 +564,7 @@ static void timer_save_state(struct arch_timer_context =
*ctx)
 		timer_set_ctl(ctx, read_sysreg_el0(SYS_CNTP_CTL));
 		cval =3D read_sysreg_el0(SYS_CNTP_CVAL);
=20
-		if (!has_cntpoff())
-			cval -=3D timer_get_offset(ctx);
+		cval -=3D timer_get_offset(ctx);
=20
 		timer_set_cval(ctx, cval);
=20
@@ -657,8 +651,7 @@ static void timer_restore_state(struct arch_timer_conte=
xt *ctx)
 		cval =3D timer_get_cval(ctx);
 		offset =3D timer_get_offset(ctx);
 		set_cntpoff(offset);
-		if (!has_cntpoff())
-			cval +=3D offset;
+		cval +=3D offset;
 		write_sysreg_el0(cval, SYS_CNTP_CVAL);
 		isb();
 		write_sysreg_el0(timer_get_ctl(ctx), SYS_CNTP_CTL);
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switc=
h.c
index 9611b4eaf661..6e3d3e16563f 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -90,6 +90,20 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
=20
 	___activate_traps(vcpu, hcr);
=20
+	if (has_cntpoff()) {
+		struct timer_map map;
+
+		get_timer_map(vcpu, &map);
+
+		if (map.direct_ptimer =3D=3D vcpu_ptimer(vcpu))
+			val =3D __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0);
+		else
+			val =3D __vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2);
+
+		isb();
+		write_sysreg_s(val, SYS_CNTP_CVAL_EL0);
+	}
+
 	val =3D read_sysreg(cpacr_el1);
 	val |=3D CPACR_ELx_TTA;
 	val &=3D ~(CPACR_EL1_ZEN_EL0EN | CPACR_EL1_ZEN_EL1EN |
@@ -131,6 +145,23 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
=20
 	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
=20
+	if (has_cntpoff()) {
+		struct timer_map map;
+		u64 val, offset;
+
+		get_timer_map(vcpu, &map);
+
+		val =3D read_sysreg_s(SYS_CNTP_CVAL_EL0);
+		if (map.direct_ptimer =3D=3D vcpu_ptimer(vcpu))
+			__vcpu_sys_reg(vcpu, CNTP_CVAL_EL0) =3D val;
+		else
+			__vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2) =3D val;
+
+		offset =3D read_sysreg_s(SYS_CNTPOFF_EL2);
+		write_sysreg_s(val + offset, SYS_CNTP_CVAL_EL0);
+		isb();
+	}
+
 	/*
 	 * ARM errata 1165522 and 1530923 require the actual execution of the
 	 * above before we can switch to the EL2/EL0 translation regime used by
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index ea77a569a907..86a73ad1446a 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -82,6 +82,8 @@ struct timer_map {
 	struct arch_timer_context *emul_ptimer;
 };
=20
+void get_timer_map(struct kvm_vcpu *vcpu, struct timer_map *map);
+
 struct arch_timer_cpu {
 	struct arch_timer_context timers[NR_KVM_TIMERS];
=20
@@ -149,4 +151,9 @@ void kvm_timer_cpu_down(void);
 /* CNTKCTL_EL1 valid bits as of DDI0476J.a */
 #define CNTKCTL_VALID_BITS	(BIT(17) | GENMASK_ULL(9, 0))
=20
+static inline bool has_cntpoff(void)
+{
+	return (has_vhe() && cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF));
+}
+
 #endif

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