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) {
/* evaluate pending_events before reading the vector */
smp_rmb();
(Even if we go with your patch, it deserves a comment in the code).
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
>
--
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
