| Message ID | 1585031530-19823-1-git-send-email-wanpengli@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer | expand |
Wanpeng Li <kernellwp@gmail.com> writes: > From: Wanpeng Li <wanpengli@tencent.com> > > The timer is disarmed when switching between TSC deadline and other modes, > we should set everything to disarmed state, however, LAPIC timer can be > emulated by preemption timer, it still works if vmx->hv_deadline_timer is > not -1. This patch also cancels preemption timer when disarm LAPIC timer. > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > --- > arch/x86/kvm/lapic.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 338de38..a38f1a8 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1445,6 +1445,8 @@ static void limit_periodic_timer_frequency(struct kvm_lapic *apic) > } > } > > +static void cancel_hv_timer(struct kvm_lapic *apic); > + Nitpick: cancel_hv_timer() is only 4 lines long so I'd suggest we move it instead of adding a forward declaration. > static void apic_update_lvtt(struct kvm_lapic *apic) > { > u32 timer_mode = kvm_lapic_get_reg(apic, APIC_LVTT) & > @@ -1454,6 +1456,10 @@ static void apic_update_lvtt(struct kvm_lapic *apic) > if (apic_lvtt_tscdeadline(apic) != (timer_mode == > APIC_LVT_TIMER_TSCDEADLINE)) { > hrtimer_cancel(&apic->lapic_timer.timer); > + preempt_disable(); > + if (apic->lapic_timer.hv_timer_in_use) > + cancel_hv_timer(apic); > + preempt_enable(); > kvm_lapic_set_reg(apic, APIC_TMICT, 0); > apic->lapic_timer.period = 0; > apic->lapic_timer.tscdeadline = 0;
On Tue, 24 Mar 2020 at 23:24, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Wanpeng Li <kernellwp@gmail.com> writes: > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > The timer is disarmed when switching between TSC deadline and other modes, > > we should set everything to disarmed state, however, LAPIC timer can be > > emulated by preemption timer, it still works if vmx->hv_deadline_timer is > > not -1. This patch also cancels preemption timer when disarm LAPIC timer. > > > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > --- > > arch/x86/kvm/lapic.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 338de38..a38f1a8 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -1445,6 +1445,8 @@ static void limit_periodic_timer_frequency(struct kvm_lapic *apic) > > } > > } > > > > +static void cancel_hv_timer(struct kvm_lapic *apic); > > + > > Nitpick: cancel_hv_timer() is only 4 lines long so I'd suggest we move > it instead of adding a forward declaration. There are other preemption timer operations like start_hv_timer etc around cancel_hv_timer, so it is not that suitable to move directly. Wanpeng
On 24/03/20 07:32, Wanpeng Li wrote: > hrtimer_cancel(&apic->lapic_timer.timer); > + preempt_disable(); > + if (apic->lapic_timer.hv_timer_in_use) > + cancel_hv_timer(apic); > + preempt_enable(); > kvm_lapic_set_reg(apic, APIC_TMICT, 0); > apic->lapic_timer.period = 0; > apic->lapic_timer.tscdeadline = 0; There are a few other occurrences of hrtimer_cancel, and all of them probably have a similar issue. What about adding a cancel_apic_timer function that contains the combination of hrtimer_cancel/preempt_disable/cancel_hv_timer/preempt_enable? Thanks, Paolo
On Wed, 25 Mar 2020 at 23:55, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 24/03/20 07:32, Wanpeng Li wrote: > > hrtimer_cancel(&apic->lapic_timer.timer); > > + preempt_disable(); > > + if (apic->lapic_timer.hv_timer_in_use) > > + cancel_hv_timer(apic); > > + preempt_enable(); > > kvm_lapic_set_reg(apic, APIC_TMICT, 0); > > apic->lapic_timer.period = 0; > > apic->lapic_timer.tscdeadline = 0; > > There are a few other occurrences of hrtimer_cancel, and all of them > probably have a similar issue. What about adding a cancel_apic_timer Other places are a little different, here we just disarm the timer, other places we will restart the timer just after the disarm except the vCPU reset (fixed in commit 95c065400a1 (KVM: VMX: Stop the preemption timer during vCPU reset)), the restart will override vmx->hv_deadline_tsc. What do you think? I can do it if introduce cancel_apic_timer() is still better. Wanpeng
On 26/03/20 01:20, Wanpeng Li wrote: >> There are a few other occurrences of hrtimer_cancel, and all of them >> probably have a similar issue. What about adding a cancel_apic_timer > Other places are a little different, here we just disarm the timer, > other places we will restart the timer just after the disarm except > the vCPU reset (fixed in commit 95c065400a1 (KVM: VMX: Stop the > preemption timer during vCPU reset)), the restart will override > vmx->hv_deadline_tsc. What do you think? I can do it if introduce > cancel_apic_timer() is still better. At least start_apic_timer() would benefit from adding hrtimer_cancel(), removing it from kvm_set_lapic_tscdeadline_msr and kvm_lapic_reg_write. But you're right that it doesn't benefit from a cancel_apic_timer(), because ultimately they either update the preemption timer or cancel it in start_sw_timer. So I'll apply your patch and send a cleanup myself for start_apic_timer. Thanks, Paolo
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 338de38..a38f1a8 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1445,6 +1445,8 @@ static void limit_periodic_timer_frequency(struct kvm_lapic *apic) } } +static void cancel_hv_timer(struct kvm_lapic *apic); + static void apic_update_lvtt(struct kvm_lapic *apic) { u32 timer_mode = kvm_lapic_get_reg(apic, APIC_LVTT) & @@ -1454,6 +1456,10 @@ static void apic_update_lvtt(struct kvm_lapic *apic) if (apic_lvtt_tscdeadline(apic) != (timer_mode == APIC_LVT_TIMER_TSCDEADLINE)) { hrtimer_cancel(&apic->lapic_timer.timer); + preempt_disable(); + if (apic->lapic_timer.hv_timer_in_use) + cancel_hv_timer(apic); + preempt_enable(); kvm_lapic_set_reg(apic, APIC_TMICT, 0); apic->lapic_timer.period = 0; apic->lapic_timer.tscdeadline = 0;