* [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer
@ 2020-03-24 6:32 Wanpeng Li
2020-03-24 15:24 ` Vitaly Kuznetsov
2020-03-25 15:55 ` Paolo Bonzini
0 siblings, 2 replies; 6+ messages in thread
From: Wanpeng Li @ 2020-03-24 6:32 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel
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);
+
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;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer
2020-03-24 6:32 [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer Wanpeng Li
@ 2020-03-24 15:24 ` Vitaly Kuznetsov
2020-03-25 0:16 ` Wanpeng Li
2020-03-25 15:55 ` Paolo Bonzini
1 sibling, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2020-03-24 15:24 UTC (permalink / raw)
To: Wanpeng Li, linux-kernel, kvm
Cc: Paolo Bonzini, Sean Christopherson, Wanpeng Li, Jim Mattson,
Joerg Roedel
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;
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer
2020-03-24 15:24 ` Vitaly Kuznetsov
@ 2020-03-25 0:16 ` Wanpeng Li
0 siblings, 0 replies; 6+ messages in thread
From: Wanpeng Li @ 2020-03-25 0:16 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: LKML, kvm, Paolo Bonzini, Sean Christopherson, Wanpeng Li,
Jim Mattson, Joerg Roedel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer
2020-03-24 6:32 [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer Wanpeng Li
2020-03-24 15:24 ` Vitaly Kuznetsov
@ 2020-03-25 15:55 ` Paolo Bonzini
2020-03-26 0:20 ` Wanpeng Li
1 sibling, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2020-03-25 15:55 UTC (permalink / raw)
To: Wanpeng Li, linux-kernel, kvm
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer
2020-03-25 15:55 ` Paolo Bonzini
@ 2020-03-26 0:20 ` Wanpeng Li
2020-03-26 0:28 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Wanpeng Li @ 2020-03-26 0:20 UTC (permalink / raw)
To: Paolo Bonzini
Cc: LKML, kvm, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer
2020-03-26 0:20 ` Wanpeng Li
@ 2020-03-26 0:28 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-03-26 0:28 UTC (permalink / raw)
To: Wanpeng Li
Cc: LKML, kvm, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-26 0:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 6:32 [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm LAPIC timer Wanpeng Li
2020-03-24 15:24 ` Vitaly Kuznetsov
2020-03-25 0:16 ` Wanpeng Li
2020-03-25 15:55 ` Paolo Bonzini
2020-03-26 0:20 ` Wanpeng Li
2020-03-26 0:28 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).