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

[Ganapatrao Kulkarni]

On 28-08-2023 03:47 pm, Marc Zyngier wrote:
> On Thu, 24 Aug 2023 07:37:42 +0100,
> Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > > 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?
> >
> > I tried this and seems not working, this is due to timer save/restore
> > are not called for some of the kvm_exit and entry paths(lighter
> > switches).
>
> Can you point me to such paths? Are you referring to the ECV handling
> of the physical timer registers?
>
> > I tried changing this patch like, Removed cval adjust from the
> > kvm_entry and still cval is adjusted on kvm_exit and in
> > timer_restore_state function, reduced cval by offset.
> >
> > Please let me know, if this is not you intended to try?
> > If possible, please share the steps or pseudo code.
>
> What I want to get to is that:
>
> - on entry (TGE having been flipped to 0), the guest's CVAL is always
>    reload from memory, because that's the absolute reference. We should
>    never load anything else on the CPU.
>
> - on exit (TGE having been flipped to 1), the guest's CVAL is stored
>    as the one true value to memory, and the CPU's view is offset by the
>    offset.
>
> - the high-level save/restore helpers apply the offsets back and forth
>    as if CNTPOFF didn't exist (because that's exactly the case if
>    TGE=1).
>
> Now, I'm pretty sure I'm still missing something, but the above is
> roughly the scheme I'm trying to follow?

Sorry for the confusion. It was my bad, I did some mistake while trying 
your hack patch. Based on your patch and above explanation I have 
created below diff which is working and clean.

diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
index 4eb601e7de50..f22cc733efb1 100644
--- a/arch/arm64/include/asm/virt.h
+++ b/arch/arm64/include/asm/virt.h
@@ -132,6 +132,11 @@ static __always_inline bool has_vhe(void)
 		return cpus_have_final_cap(ARM64_HAS_VIRT_HOST_EXTN);
 }

+static __always_inline bool has_cntpoff(void)
+{
+	return (has_vhe() && cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF));
+}
+
 static __always_inline bool is_protected_kvm_enabled(void)
 {
 	if (is_vhe_hyp_code())
diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 75bddab3224f..de625bc7c25c 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 = {
 	.get_input_level = kvm_arch_timer_get_input_level,
 };

-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_nv(vcpu))
@@ -566,10 +561,7 @@ static void timer_save_state(struct 
arch_timer_context *ctx)
 	case TIMER_HPTIMER:
 		timer_set_ctl(ctx, read_sysreg_el0(SYS_CNTP_CTL));
 		cval = read_sysreg_el0(SYS_CNTP_CVAL);
-
-		if (!has_cntpoff())
-			cval -= timer_get_offset(ctx);
-
+		cval -= timer_get_offset(ctx);
 		timer_set_cval(ctx, cval);

 		/* Disable the timer */
@@ -655,8 +647,7 @@ static void timer_restore_state(struct 
arch_timer_context *ctx)
 		cval = timer_get_cval(ctx);
 		offset = timer_get_offset(ctx);
 		set_cntpoff(offset);
-		if (!has_cntpoff())
-			cval += offset;
+		cval += offset;
 		write_sysreg_el0(cval, SYS_CNTP_CVAL);
 		isb();
 		write_sysreg_el0(timer_get_ctl(ctx), SYS_CNTP_CTL);
@@ -960,6 +951,59 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
 		kvm_timer_blocking(vcpu);
 }

+static void ptimer_cval_save(struct arch_timer_context *ctx, u64 offset)
+{
+	unsigned long flags;
+	u64 cval;
+
+	local_irq_save(flags);
+	cval = read_sysreg_el0(SYS_CNTP_CVAL);
+	timer_set_cval(ctx, cval);
+	cval += offset;
+	write_sysreg_el0(cval, SYS_CNTP_CVAL);
+	isb();
+	local_irq_restore(flags);
+}
+
+static void ptimer_cval_restore(struct arch_timer_context *ctx, u64 offset)
+{
+	unsigned long flags;
+	u64 cval;
+
+	local_irq_save(flags);
+	cval = timer_get_cval(ctx);
+	write_sysreg_el0(cval, SYS_CNTP_CVAL);
+	isb();
+	local_irq_restore(flags);
+}
+
+void kvm_ptimer_cval_save_restore(struct kvm_vcpu *vcpu, bool save)
+{
+	struct timer_map map;
+	struct arch_timer_cpu *timer;
+	struct arch_timer_context *ctxp;
+	u64 offset;
+
+	get_timer_map(vcpu, &map);
+	ctxp = map.direct_ptimer;
+
+	if (unlikely(ctxp == NULL))
+		return;
+	
+	offset = timer_get_offset(ctxp);
+	if (!offset)
+		return;
+
+	timer = vcpu_timer(ctxp->vcpu);
+	if (!timer->enabled || !ctxp->loaded)
+		return;
+
+	if (save)
+		ptimer_cval_save(ctxp, offset);
+	else
+		ptimer_cval_restore(ctxp, offset);
+}
+
 void kvm_timer_sync_nested(struct kvm_vcpu *vcpu)
 {
 	/*
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c 
b/arch/arm64/kvm/hyp/vhe/switch.c
index 561cb53e19ce..097fcaf7b208 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -100,6 +100,10 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
 		hcr |= vhcr_el2;
 	}

+	/* Restore CVAL */
+	if (has_cntpoff())
+		kvm_ptimer_cval_save_restore(vcpu, false);
+
 	___activate_traps(vcpu, hcr);

 	val = read_sysreg(cpacr_el1);
@@ -141,6 +145,15 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)

 	___deactivate_traps(vcpu);

+	/*
+	 * For VHE Host, HCR_EL2.{E2H, TGE} is {1, 1}, FEAT_ECV
+	 * is disabled and CNTPOFF_EL2 value is treated as zero.
+	 * Hence, need to save guest written CVAL in memory and
+	 * increment PTIMER's CVAL by CNTPOFF to avoid false interrupt.
+	 */
+	if (has_cntpoff())
+		kvm_ptimer_cval_save_restore(vcpu, true);
+
 	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);

 	/*
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index ea77a569a907..ce3f4d9e7dd4 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -117,6 +117,7 @@ void kvm_timer_vcpu_load(struct kvm_vcpu *vcpu);
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);

 void kvm_timer_init_vhe(void);
+void kvm_ptimer_cval_save_restore(struct kvm_vcpu *vcpu, bool save);

 #define vcpu_timer(v)	(&(v)->arch.timer_cpu)
 #define vcpu_get_timer(v,t)	(&vcpu_timer(v)->timers[(t)])


Thanks,
Ganapat

> Thanks,
>
> 	M.