| Message ID | 1622710841-76604-1-git-send-email-wanpengli@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer | expand |
On Thu, Jun 03, 2021, Wanpeng Li wrote: > From: Wanpeng Li <wanpengli@tencent.com> > > According to the SDM 10.5.4.1: > > A write of 0 to the initial-count register effectively stops the local > APIC timer, in both one-shot and periodic mode. > > The lapic timer oneshot/periodic mode which is emulated by vmx-preemption > timer doesn't stop since vmx->hv_deadline_tsc is still set. But the VMX preemption timer is only used for deadline, never for oneshot or periodic. Am I missing something? static bool start_hv_timer(struct kvm_lapic *apic) { struct kvm_timer *ktimer = &apic->lapic_timer; struct kvm_vcpu *vcpu = apic->vcpu; bool expired; WARN_ON(preemptible()); if (!kvm_can_use_hv_timer(vcpu)) return false; if (!ktimer->tscdeadline) <------- return false; ... }
On Thu, Jun 3, 2021 at 2:04 AM Wanpeng Li <kernellwp@gmail.com> wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > According to the SDM 10.5.4.1: > > A write of 0 to the initial-count register effectively stops the local > APIC timer, in both one-shot and periodic mode. If KVM is not correctly emulating this behavior then could you also add a kvm-unit-test to test for the correct behavior? > > The lapic timer oneshot/periodic mode which is emulated by vmx-preemption > timer doesn't stop since vmx->hv_deadline_tsc is still set. This patch > fixes it by also cancel vmx-preemption timer when writing 0 to initial-count > register. > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > --- > arch/x86/kvm/lapic.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 8120e86..20dd2ae 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1494,6 +1494,15 @@ static void limit_periodic_timer_frequency(struct kvm_lapic *apic) > > static void cancel_hv_timer(struct kvm_lapic *apic); > > +static void cancel_timer(struct kvm_lapic *apic) > +{ > + hrtimer_cancel(&apic->lapic_timer.timer); > + preempt_disable(); > + if (apic->lapic_timer.hv_timer_in_use) > + cancel_hv_timer(apic); > + preempt_enable(); > +} > + > static void apic_update_lvtt(struct kvm_lapic *apic) > { > u32 timer_mode = kvm_lapic_get_reg(apic, APIC_LVTT) & > @@ -1502,11 +1511,7 @@ static void apic_update_lvtt(struct kvm_lapic *apic) > if (apic->lapic_timer.timer_mode != timer_mode) { > 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(); > + cancel_timer(apic); > kvm_lapic_set_reg(apic, APIC_TMICT, 0); > apic->lapic_timer.period = 0; > apic->lapic_timer.tscdeadline = 0; > @@ -2092,7 +2097,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) > if (apic_lvtt_tscdeadline(apic)) > break; > > - hrtimer_cancel(&apic->lapic_timer.timer); > + cancel_timer(apic); > kvm_lapic_set_reg(apic, APIC_TMICT, val); > start_apic_timer(apic); > break; > -- > 2.7.4 >
On Fri, 4 Jun 2021 at 07:02, David Matlack <dmatlack@google.com> wrote: > > On Thu, Jun 3, 2021 at 2:04 AM Wanpeng Li <kernellwp@gmail.com> wrote: > > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > According to the SDM 10.5.4.1: > > > > A write of 0 to the initial-count register effectively stops the local > > APIC timer, in both one-shot and periodic mode. > > If KVM is not correctly emulating this behavior then could you also > add a kvm-unit-test to test for the correct behavior? A simple test here, the test will hang after the patch since it will not receive the spurious interrupt any more. diff --git a/x86/apic.c b/x86/apic.c index a7681fe..947d018 100644 --- a/x86/apic.c +++ b/x86/apic.c @@ -488,6 +488,14 @@ static void test_apic_timer_one_shot(void) */ report((lvtt_counter == 1) && (tsc2 - tsc1 >= interval), "APIC LVT timer one shot"); + + lvtt_counter = 0; + apic_write(APIC_TMICT, interval); + apic_write(APIC_TMICT, 0); + while (!lvtt_counter); + + report((lvtt_counter == 1), + "APIC LVT timer one shot spurious interrupt"); }
On Thu, 3 Jun 2021 at 23:20, Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Jun 03, 2021, Wanpeng Li wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > > > According to the SDM 10.5.4.1: > > > > A write of 0 to the initial-count register effectively stops the local > > APIC timer, in both one-shot and periodic mode. > > > > The lapic timer oneshot/periodic mode which is emulated by vmx-preemption > > timer doesn't stop since vmx->hv_deadline_tsc is still set. > > But the VMX preemption timer is only used for deadline, never for oneshot or > periodic. Am I missing something? Yes, it is upstream. https://lore.kernel.org/kvm/1477304593-3453-1-git-send-email-wanpeng.li@hotmail.com/ Wanpeng
On Fri, Jun 04, 2021, Wanpeng Li wrote: > On Thu, 3 Jun 2021 at 23:20, Sean Christopherson <seanjc@google.com> wrote: > > > > On Thu, Jun 03, 2021, Wanpeng Li wrote: > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > > > According to the SDM 10.5.4.1: > > > > > > A write of 0 to the initial-count register effectively stops the local > > > APIC timer, in both one-shot and periodic mode. > > > > > > The lapic timer oneshot/periodic mode which is emulated by vmx-preemption > > > timer doesn't stop since vmx->hv_deadline_tsc is still set. > > > > But the VMX preemption timer is only used for deadline, never for oneshot or > > periodic. Am I missing something? > > Yes, it is upstream. Huh. I always thought 'tscdeadline' alluded to the timer being in deadline mode and never looked closely at the arming code. Thanks! Maybe name the new helper cancel_apic_timer() to align with start_apic_timer() and restart_apic_timer()? With that: Reviewed-by: Sean Christopherson <seanjc@google.com>
On Thu, Jun 3, 2021 at 5:33 PM Wanpeng Li <kernellwp@gmail.com> wrote: > > On Fri, 4 Jun 2021 at 07:02, David Matlack <dmatlack@google.com> wrote: > > > > On Thu, Jun 3, 2021 at 2:04 AM Wanpeng Li <kernellwp@gmail.com> wrote: > > > > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > > > According to the SDM 10.5.4.1: > > > > > > A write of 0 to the initial-count register effectively stops the local > > > APIC timer, in both one-shot and periodic mode. > > > > If KVM is not correctly emulating this behavior then could you also > > add a kvm-unit-test to test for the correct behavior? > > A simple test here, the test will hang after the patch since it will > not receive the spurious interrupt any more. Thanks. Can you send this as a [PATCH]? I think it would be worthwhile so have a regression test for this bug. > > diff --git a/x86/apic.c b/x86/apic.c > index a7681fe..947d018 100644 > --- a/x86/apic.c > +++ b/x86/apic.c > @@ -488,6 +488,14 @@ static void test_apic_timer_one_shot(void) > */ > report((lvtt_counter == 1) && (tsc2 - tsc1 >= interval), > "APIC LVT timer one shot"); > + > + lvtt_counter = 0; > + apic_write(APIC_TMICT, interval); > + apic_write(APIC_TMICT, 0); > + while (!lvtt_counter); > + > + report((lvtt_counter == 1), > + "APIC LVT timer one shot spurious interrupt"); > }
On Fri, 4 Jun 2021 at 23:37, Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Jun 04, 2021, Wanpeng Li wrote: > > On Thu, 3 Jun 2021 at 23:20, Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Thu, Jun 03, 2021, Wanpeng Li wrote: > > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > > > > > According to the SDM 10.5.4.1: > > > > > > > > A write of 0 to the initial-count register effectively stops the local > > > > APIC timer, in both one-shot and periodic mode. > > > > > > > > The lapic timer oneshot/periodic mode which is emulated by vmx-preemption > > > > timer doesn't stop since vmx->hv_deadline_tsc is still set. > > > > > > But the VMX preemption timer is only used for deadline, never for oneshot or > > > periodic. Am I missing something? > > > > Yes, it is upstream. > > Huh. I always thought 'tscdeadline' alluded to the timer being in deadline mode > and never looked closely at the arming code. Thanks! > > Maybe name the new helper cancel_apic_timer() to align with start_apic_timer() > and restart_apic_timer()? With that: > > Reviewed-by: Sean Christopherson <seanjc@google.com> Do it in v2, thanks. Wanpeng
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 8120e86..20dd2ae 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1494,6 +1494,15 @@ static void limit_periodic_timer_frequency(struct kvm_lapic *apic) static void cancel_hv_timer(struct kvm_lapic *apic); +static void cancel_timer(struct kvm_lapic *apic) +{ + hrtimer_cancel(&apic->lapic_timer.timer); + preempt_disable(); + if (apic->lapic_timer.hv_timer_in_use) + cancel_hv_timer(apic); + preempt_enable(); +} + static void apic_update_lvtt(struct kvm_lapic *apic) { u32 timer_mode = kvm_lapic_get_reg(apic, APIC_LVTT) & @@ -1502,11 +1511,7 @@ static void apic_update_lvtt(struct kvm_lapic *apic) if (apic->lapic_timer.timer_mode != timer_mode) { 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(); + cancel_timer(apic); kvm_lapic_set_reg(apic, APIC_TMICT, 0); apic->lapic_timer.period = 0; apic->lapic_timer.tscdeadline = 0; @@ -2092,7 +2097,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) if (apic_lvtt_tscdeadline(apic)) break; - hrtimer_cancel(&apic->lapic_timer.timer); + cancel_timer(apic); kvm_lapic_set_reg(apic, APIC_TMICT, val); start_apic_timer(apic); break;