| Message ID | 20250204110050.150560-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM/arm64: timer fixes for 6.14 | expand |
Hello, Marc. On 2/4/25 13:00, Marc Zyngier wrote: > Both Wei-Lin Chang and Volodymyr Babchuk report that the way we > handle the emulation of EL1 timers with NV is completely wrong, > specially in the case of HCR_EL2.E2H==0. > > There are three problems in about as many lines of code: > > - With E2H==0, the EL1 timers are overwritten with the EL1 state, > while they should actually contain the EL2 state (as per the timer > map) > > - With E2H==1, we run the full EL1 timer emulation even when ECV > is present, hiding a bug in timer_emulate() (see previous patch) > > - The comments are actively misleading, and say all the wrong things. > > This is only attributable to the code having been initially written > for FEAT_NV, hacked up to handle FEAT_NV2 *in parallel*, and vaguely > hacked again to be FEAT_NV2 only. Oh, and yours truly being a gold > plated idiot. > > The fix is obvious: just delete most of the E2H==0 code, have a unified > handling of the timers (because they really are E2H agnostic), and > make sure we don't execute any of that when FEAT_ECV is present. > > Fixes: 4bad3068cfa9f ("KVM: arm64: nv: Sync nested timer state with FEAT_NV2") > Reported-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw> > Reported-by: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > Link: https://lore.kernel.org/r/fqiqfjzwpgbzdtouu2pwqlu7llhnf5lmy4hzv5vo6ph4v3vyls@jdcfy3fjjc5k > Link: https://lore.kernel.org/r/87frl51tse.fsf@epam.com Tested-by: Dmytro Terletskyi <dmytro_terletskyi@epam.com> > --- > arch/arm64/kvm/arch_timer.c | 30 ++++++++++-------------------- > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > index 035e43f5d4f9a..e59836e0260cf 100644 > --- a/arch/arm64/kvm/arch_timer.c > +++ b/arch/arm64/kvm/arch_timer.c > @@ -974,31 +974,21 @@ void kvm_timer_sync_nested(struct kvm_vcpu *vcpu) > * which allows trapping of the timer registers even with NV2. > * Still, this is still worse than FEAT_NV on its own. Meh. > */ > - if (!vcpu_el2_e2h_is_set(vcpu)) { > - if (cpus_have_final_cap(ARM64_HAS_ECV)) > - return; > - > - /* > - * A non-VHE guest hypervisor doesn't have any direct access > - * to its timers: the EL2 registers trap (and the HW is > - * fully emulated), while the EL0 registers access memory > - * despite the access being notionally direct. Boo. > - * > - * We update the hardware timer registers with the > - * latest value written by the guest to the VNCR page > - * and let the hardware take care of the rest. > - */ > - write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTV_CTL_EL0), SYS_CNTV_CTL); > - write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTV_CVAL_EL0), SYS_CNTV_CVAL); > - write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTP_CTL_EL0), SYS_CNTP_CTL); > - write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTP_CVAL_EL0), SYS_CNTP_CVAL); > - } else { > + if (!cpus_have_final_cap(ARM64_HAS_ECV)) { > /* > * For a VHE guest hypervisor, the EL2 state is directly > - * stored in the host EL1 timers, while the emulated EL0 > + * stored in the host EL1 timers, while the emulated EL1 > * state is stored in the VNCR page. The latter could have > * been updated behind our back, and we must reset the > * emulation of the timers. > + * > + * A non-VHE guest hypervisor doesn't have any direct access > + * to its timers: the EL2 registers trap despite being > + * notionally direct (we use the EL1 HW, as for VHE), while > + * the EL1 registers access memory. > + * > + * In both cases, process the emulated timers on each guest > + * exit. Boo. > */ > struct timer_map map; > get_timer_map(vcpu, &map);
diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c index 035e43f5d4f9a..e59836e0260cf 100644 --- a/arch/arm64/kvm/arch_timer.c +++ b/arch/arm64/kvm/arch_timer.c @@ -974,31 +974,21 @@ void kvm_timer_sync_nested(struct kvm_vcpu *vcpu) * which allows trapping of the timer registers even with NV2. * Still, this is still worse than FEAT_NV on its own. Meh. */ - if (!vcpu_el2_e2h_is_set(vcpu)) { - if (cpus_have_final_cap(ARM64_HAS_ECV)) - return; - - /* - * A non-VHE guest hypervisor doesn't have any direct access - * to its timers: the EL2 registers trap (and the HW is - * fully emulated), while the EL0 registers access memory - * despite the access being notionally direct. Boo. - * - * We update the hardware timer registers with the - * latest value written by the guest to the VNCR page - * and let the hardware take care of the rest. - */ - write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTV_CTL_EL0), SYS_CNTV_CTL); - write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTV_CVAL_EL0), SYS_CNTV_CVAL); - write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTP_CTL_EL0), SYS_CNTP_CTL); - write_sysreg_el0(__vcpu_sys_reg(vcpu, CNTP_CVAL_EL0), SYS_CNTP_CVAL); - } else { + if (!cpus_have_final_cap(ARM64_HAS_ECV)) { /* * For a VHE guest hypervisor, the EL2 state is directly - * stored in the host EL1 timers, while the emulated EL0 + * stored in the host EL1 timers, while the emulated EL1 * state is stored in the VNCR page. The latter could have * been updated behind our back, and we must reset the * emulation of the timers. + * + * A non-VHE guest hypervisor doesn't have any direct access + * to its timers: the EL2 registers trap despite being + * notionally direct (we use the EL1 HW, as for VHE), while + * the EL1 registers access memory. + * + * In both cases, process the emulated timers on each guest + * exit. Boo. */ struct timer_map map; get_timer_map(vcpu, &map);
Both Wei-Lin Chang and Volodymyr Babchuk report that the way we handle the emulation of EL1 timers with NV is completely wrong, specially in the case of HCR_EL2.E2H==0. There are three problems in about as many lines of code: - With E2H==0, the EL1 timers are overwritten with the EL1 state, while they should actually contain the EL2 state (as per the timer map) - With E2H==1, we run the full EL1 timer emulation even when ECV is present, hiding a bug in timer_emulate() (see previous patch) - The comments are actively misleading, and say all the wrong things. This is only attributable to the code having been initially written for FEAT_NV, hacked up to handle FEAT_NV2 *in parallel*, and vaguely hacked again to be FEAT_NV2 only. Oh, and yours truly being a gold plated idiot. The fix is obvious: just delete most of the E2H==0 code, have a unified handling of the timers (because they really are E2H agnostic), and make sure we don't execute any of that when FEAT_ECV is present. Fixes: 4bad3068cfa9f ("KVM: arm64: nv: Sync nested timer state with FEAT_NV2") Reported-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw> Reported-by: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> Signed-off-by: Marc Zyngier <maz@kernel.org> Link: https://lore.kernel.org/r/fqiqfjzwpgbzdtouu2pwqlu7llhnf5lmy4hzv5vo6ph4v3vyls@jdcfy3fjjc5k Link: https://lore.kernel.org/r/87frl51tse.fsf@epam.com --- arch/arm64/kvm/arch_timer.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-)