| Message ID | 51A48D53.7070204@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <gleb@redhat.com> > > 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. -- 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
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) { /* evaluate pending_events before reading the vector */ smp_rmb();