Messages in this thread Patch in this message | | | Date | Fri, 1 Sep 2023 17:45:20 +0530 | Subject | Re: [PATCH 2/2] KVM: arm64: timers: Adjust CVAL of a ptimer across guest entry and exits | From | Ganapatrao Kulkarni <> |
| |
On 28-08-2023 03:47 pm, Marc Zyngier wrote: > On Thu, 24 Aug 2023 07:37:42 +0100, > Ganapatrao Kulkarni <gankulkarni@os.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. >
| |