| Message ID | 1495337552-78885-1-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21/05/2017 05:32, Wanpeng Li wrote: > vmx_cancel_hv_timer > vCPU0's vmx->hv_deadline_tsc = -1 > > preempt occur > > clear preemption timer field in CPU1's active vmcs > vCPU0's apic_timer.hv_timer_in_use = false > vmx_vcpu_run(vCPU0) > vmx_arm_hv_timer > if (vmx->hv_deadline_tsc == -1) > nothing change > > handle_preemption_timer(vCPU0) > kvm_lapic_expired_hv_timer > WARN_ON(!apic->lapic_timer.hv_timer_in_use); > > Preemption can occur during cancel preemption timer, and there will be inconsistent > status in lapic, vmx and vmcs field. This patch fixes it by disable preemption for > cancelling preemption timer. I see, so the purpose is to serialize against kvm_arch_vcpu_load. Nice catch, I've queued the patch for kvm/master. Paolo > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > --- > arch/x86/kvm/lapic.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index c329d28..6e6f345 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1495,8 +1495,10 @@ EXPORT_SYMBOL_GPL(kvm_lapic_hv_timer_in_use); > > static void cancel_hv_timer(struct kvm_lapic *apic) > { > + preempt_disable(); > kvm_x86_ops->cancel_hv_timer(apic->vcpu); > apic->lapic_timer.hv_timer_in_use = false; > + preempt_enable(); > } > > static bool start_hv_timer(struct kvm_lapic *apic) > --
On 21/05/2017 05:32, Wanpeng Li wrote: > CPU0 CPU1 > > vmx_cancel_hv_timer > vCPU0's vmx->hv_deadline_tsc = -1 > > preempt occur > > clear preemption timer field in CPU1's active vmcs > vCPU0's apic_timer.hv_timer_in_use = false > vmx_vcpu_run(vCPU0) > vmx_arm_hv_timer > if (vmx->hv_deadline_tsc == -1) > nothing change > > handle_preemption_timer(vCPU0) > kvm_lapic_expired_hv_timer > WARN_ON(!apic->lapic_timer.hv_timer_in_use); I think it's more like this, what do you think? CPU0 CPU1 preemption timer vmexit handle_preemption_timer(vCPU0) kvm_lapic_expired_hv_timer vmx_cancel_hv_timer vmx->hv_deadline_tsc = -1 vmcs_clear_bits /* hv_timer_in_use still true */ sched_out sched_in kvm_arch_vcpu_load vmx_set_hv_timer write vmx->hv_deadline_tsc vmcs_set_bits /* back in kvm_lapic_expired_hv_timer */ hv_timer_in_use = false ... vmx_vcpu_run vmx_arm_hv_run write preemption timer deadline spurious preemption timer vmexit handle_preemption_timer(vCPU0) kvm_lapic_expired_hv_timer WARN_ON(!apic->lapic_timer.hv_timer_in_use); Thanks, Paolo
2017-05-26 23:57 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>: > > > On 21/05/2017 05:32, Wanpeng Li wrote: >> CPU0 CPU1 >> >> vmx_cancel_hv_timer >> vCPU0's vmx->hv_deadline_tsc = -1 >> >> preempt occur >> >> clear preemption timer field in CPU1's active vmcs >> vCPU0's apic_timer.hv_timer_in_use = false >> vmx_vcpu_run(vCPU0) >> vmx_arm_hv_timer >> if (vmx->hv_deadline_tsc == -1) >> nothing change >> >> handle_preemption_timer(vCPU0) >> kvm_lapic_expired_hv_timer >> WARN_ON(!apic->lapic_timer.hv_timer_in_use); > > > I think it's more like this, what do you think? > > CPU0 CPU1 > > preemption timer vmexit > handle_preemption_timer(vCPU0) > kvm_lapic_expired_hv_timer > vmx_cancel_hv_timer > vmx->hv_deadline_tsc = -1 > vmcs_clear_bits > /* hv_timer_in_use still true */ > sched_out > sched_in > kvm_arch_vcpu_load > vmx_set_hv_timer > write vmx->hv_deadline_tsc > vmcs_set_bits > /* back in kvm_lapic_expired_hv_timer */ > hv_timer_in_use = false > ... > vmx_vcpu_run > vmx_arm_hv_run > write preemption timer deadline > spurious preemption timer vmexit > handle_preemption_timer(vCPU0) > kvm_lapic_expired_hv_timer > WARN_ON(!apic->lapic_timer.hv_timer_in_use); Looks good to me, thanks for your help, Paolo. :) Regards, Wanpeng Li
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index c329d28..6e6f345 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1495,8 +1495,10 @@ EXPORT_SYMBOL_GPL(kvm_lapic_hv_timer_in_use); static void cancel_hv_timer(struct kvm_lapic *apic) { + preempt_disable(); kvm_x86_ops->cancel_hv_timer(apic->vcpu); apic->lapic_timer.hv_timer_in_use = false; + preempt_enable(); } static bool start_hv_timer(struct kvm_lapic *apic)