From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing Date: Tue, 28 May 2013 15:56:13 +0300 Message-ID: <20130528125613.GB3326@redhat.com> References: <20130526130031.GS4725@redhat.com> <51A48D53.7070204@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Jan Kiszka To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39171 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933922Ab3E1M4S (ORCPT ); Tue, 28 May 2013 08:56:18 -0400 Content-Disposition: inline In-Reply-To: <51A48D53.7070204@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, May 28, 2013 at 12:56:19PM +0200, Paolo Bonzini wrote: > Il 26/05/2013 15:00, Gleb Natapov ha scritto: > > apic->pending_events processing has a race that may cause INIT and SIPI > > processing to be reordered: > > > > vpu0: vcpu1: > > set INIT > > test_and_clear_bit(KVM_APIC_INIT) > > process INIT > > set INIT > > set SIPI > > test_and_clear_bit(KVM_APIC_SIPI) > > process SIPI > > > > At the and INIT is left pending in pending_events. The following patch > > tries to fix this using the fact that if INIT comes after SIPI it drops > > SIPI from the pending_events, so if pending_events is different after > > SIPI is processed it means that INIT was issued after SIPI otherwise > > all pending event are processed and pending_events can be reset to zero. > > The patch looks correct, but is there any reason to do the cmpxchg at > the end? > > That is, would this have any problem? It seems a bit simpler: > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index e1adbb4..3fe00fc 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1873,7 +1873,11 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu) > else > vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED; > } > - if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) && > + /* > + * Note that we may get another INIT+SIPI sequence right here; process > + * the INIT first. Assumes that there are only KVM_APIC_INIT/SIPI. > + */ > + if (cmpxchg(&apic->pending_events, KVM_APIC_SIPI, 0) == KVM_APIC_SIPI && > vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) { Because pending_events can be INIT/SIPI at this point and it should be interpreted as: do SIPI and ignore INIT (atomically). > /* evaluate pending_events before reading the vector */ > smp_rmb(); > > (Even if we go with your patch, it deserves a comment in the code). > We can move explanation from the commit message to a coment. > Paolo > > > Signed-off-by: Gleb Natapov > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 9d75193..67686b8 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -1850,6 +1850,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu) > > { > > struct kvm_lapic *apic = vcpu->arch.apic; > > unsigned int sipi_vector; > > + unsigned long pe; > > > > if (!kvm_vcpu_has_lapic(vcpu)) > > return; > > @@ -1862,7 +1863,8 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu) > > else > > vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED; > > } > > - if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) && > > + pe = apic->pending_events; > > + if (test_bit(KVM_APIC_SIPI, &pe) && > > vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) { > > /* evaluate pending_events before reading the vector */ > > smp_rmb(); > > @@ -1871,6 +1873,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu) > > vcpu->vcpu_id, sipi_vector); > > kvm_vcpu_deliver_sipi_vector(vcpu, sipi_vector); > > vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE; > > + cmpxchg(&apic->pending_events, pe, 0); > > } > > } > > > > -- > > Gleb. > > -- > > To unsubscribe from this list: send the line "unsubscribe kvm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- Gleb.