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@xxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
