| 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
| ||
|
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