From: Wanpeng Li <[email protected]>
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 <[email protected]>
---
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
Wanpeng Li <[email protected]> writes:
> From: Wanpeng Li <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Tue, 24 Mar 2020 at 23:24, Vitaly Kuznetsov <[email protected]> wrote:
>
> Wanpeng Li <[email protected]> writes:
>
> > From: Wanpeng Li <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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