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] [day] [month] [year] [list]
Message-ID: <a4dac5af-44e2-0485-446f-fae09fa66a3c@os.amperecomputing.com>
Date:   Fri, 1 Sep 2023 17:45:20 +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, eauger@...hat.com,
        miguel.luis@...cle.com, darren@...amperecomputing.com,
        scott@...amperecomputing.com,
        Christoffer Dall <Christoffer.Dall@....com>
Subject: Re: [PATCH 2/2] KVM: arm64: timers: Adjust CVAL of a ptimer across
 guest entry and exits



On 28-08-2023 03:47 pm, Marc Zyngier wrote:
> On Thu, 24 Aug 2023 07:37:42 +0100,
> Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com> 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.
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ