[PATCH RFC] KVM: Fix race in apic->pending_events processing

Gleb Natapov

author

2013-05-26 13:00 UTC #0 lore raw

RFC PATCH 1/1 KVM: Fix race in apic->pending_events processing

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.

Signed-off-by Gleb Natapov <[email protected]>

arch/x86/kvm/lapic.c +4 -1

index 9d75193..67686b8 100644
@@ -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.

Paolo Bonzini

maintainer

2013-05-28 10:56 UTC #1 lore raw

Il 26/05/2013 15:00, Gleb Natapov ha scritto:

Quoted 12 lines

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

Quoted 5 lines

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:

arch/x86/kvm/lapic.c +5 -1

index e1adbb4..3fe00fc 100644
@@ -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

Quoted 38 lines

Signed-off-by Gleb Natapov <[email protected]>

arch/x86/kvm/lapic.c +4 -2

index 9d75193..67686b8 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Gleb Natapov

author

2013-05-28 12:56 UTC #2 lore raw

Quoted 29 lines · Paolo Bonzini

Il 26/05/2013 15:00, Gleb Natapov ha scritto:
Quoted 12 lines
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
Quoted 5 lines
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:
arch/x86/kvm/lapic.c +0 -0
index e1adbb4..3fe00fc 100644

arch/x86/kvm/lapic.c void kvm_apic_accept_events(struct kvm_vcpu *vcpu) · line 1881

Quoted 10 lines

arch/x86/kvm/lapic.c +5 -1

@@ -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).

Quoted 5 lines

arch/x86/kvm/lapic.c +0 -0
 		/* 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.

Quoted 33 lines

Paolo

Quoted 31 lines

Signed-off-by Gleb Natapov <[email protected]>

arch/x86/kvm/lapic.c +4 -1

index 9d75193..67686b8 100644
@@ -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);
 	}
 }

Quoted 7 lines

Quoted 7 lines
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

Paolo Bonzini

maintainer

2013-05-28 13:49 UTC #3 lore raw

Il 28/05/2013 14:56, Gleb Natapov ha scritto:

Quoted 4 lines

(quoted diff) +0 -1

 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&

Quoted 8 lines

Quoted 6 lines
Review
Quoted 6 lines
(quoted diff) +5 -0
+	/*
+	 * 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).

My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts

INIT-INIT-SIPI-INIT-SIPI

your version would do many SIPIs, while mine would do just one.

Hmm... there is a reference to this in 25.2 "Other causes of VM exits":
"If a logical processor is in the wait-for-SIPI state, INIT signals are blocked. They do not cause VM exits in this case." It is not for the physical processor, but it makes sense to have the same thing. Is this the reason why you did the cmpxchg at the end?

But then, there's another way to mask INITs in the wait-for-SIPI state. Considering that KVM_MP_STATE_INIT_RECEIVED is really a wait-for-SIPI, you can do:

arch/x86/kvm/lapic.c +10 -2

index e1adbb4..36bc308 100644
@@ -720,8 +720,12 @@ out:
 		break;
 
 	case APIC_DM_INIT:
-		if (!trig_mode || level) {
+		if ((!trig_mode || level) &&
+		    vcpu->arch.mp_state != KVM_MP_STATE_INIT_RECEIVED) {
 			result = 1;
+
+			/* check mp_state before writing apic->pending_events */
+			smp_mb();
 			/* assumes that there are only KVM_APIC_INIT/SIPI */
 			apic->pending_events = (1UL << KVM_APIC_INIT);
 			/* make sure pending_events is visible before sending

@@ -1865,13 +1869,17 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 	if (!kvm_vcpu_has_lapic(vcpu))
 		return;
 
-	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
+	if (test_bit(KVM_APIC_INIT, &apic->pending_events)) {
 		kvm_lapic_reset(vcpu);
 		kvm_vcpu_reset(vcpu);
 		if (kvm_vcpu_is_bsp(apic->vcpu))
 			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+
+		/* write mp_state before toggling KVM_APIC_INIT */
+		smp_mb__before_clear_bit();
+		clear_bit(KVM_APIC_INIT, &apic->pending_events);
 	}
 	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
 	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {

I don't have a particular preference.

Paolo

Gleb Natapov

author

2013-05-28 15:01 UTC #4 lore raw

Quoted 14 lines · Paolo Bonzini

Il 28/05/2013 14:56, Gleb Natapov ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/lapic.c +0 -1
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
Quoted 8 lines
Quoted 6 lines
Review
Quoted 6 lines
(quoted diff) +5 -0
+	/*
+	 * 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).

Quoted 6 lines

My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts

You assume that the next processing will actually happen, but this is not necessary the case.

Quoted 10 lines

INIT-INIT-SIPI-INIT-SIPI
your version would do many SIPIs, while mine would do just one.
Hmm... there is a reference to this in 25.2 "Other causes of VM exits":
"If a logical processor is in the wait-for-SIPI state, INIT signals are blocked. They do not cause VM exits in this case." It is not for the physical processor, but it makes sense to have the same thing. Is this the reason why you did the cmpxchg at the end?

No, but if helps us model proper behaviour for nested guest +1 to it :) But until we handle INIT in nested virt it is not something that dictates the solution.

Quoted 4 lines

But then, there's another way to mask INITs in the wait-for-SIPI state. Considering that KVM_MP_STATE_INIT_RECEIVED is really a wait-for-SIPI, you can do:

Haven't checked it for races (especially races between multiple CPUS sending INIT), but looks more complicated to me.

Quoted 38 lines

arch/x86/kvm/lapic.c +10 -2

index e1adbb4..36bc308 100644
@@ -720,8 +720,12 @@ out:
 		break;
 
 	case APIC_DM_INIT:
-		if (!trig_mode || level) {
+		if ((!trig_mode || level) &&
+		    vcpu->arch.mp_state != KVM_MP_STATE_INIT_RECEIVED) {
 			result = 1;
+
+			/* check mp_state before writing apic->pending_events */
+			smp_mb();
 			/* assumes that there are only KVM_APIC_INIT/SIPI */
 			apic->pending_events = (1UL << KVM_APIC_INIT);
 			/* make sure pending_events is visible before sending

@@ -1865,13 +1869,17 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 	if (!kvm_vcpu_has_lapic(vcpu))
 		return;
 
-	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
+	if (test_bit(KVM_APIC_INIT, &apic->pending_events)) {
 		kvm_lapic_reset(vcpu);
 		kvm_vcpu_reset(vcpu);
 		if (kvm_vcpu_is_bsp(apic->vcpu))
 			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+
+		/* write mp_state before toggling KVM_APIC_INIT */
+		smp_mb__before_clear_bit();
+		clear_bit(KVM_APIC_INIT, &apic->pending_events);
 	}
 	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
 	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {

Quoted 3 lines

I don't have a particular preference.
Paolo

--
Gleb.

Paolo Bonzini

maintainer

2013-05-28 16:33 UTC #5 lore raw

Il 28/05/2013 17:00, Gleb Natapov ha scritto:

Quoted 15 lines · Paolo Bonzini

Quoted 14 lines
Il 28/05/2013 14:56, Gleb Natapov ha scritto:
Quoted 12 lines
Quoted 10 lines
Review
Quoted 10 lines
(quoted diff) +5 -1
 		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).

Quoted 8 lines

Quoted 6 lines
My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts
You assume that the next processing will actually happen, but this is not necessary the case.

Why not? The INIT and SIPI that have just been sent have kicked the VCPU again.

Quoted 6 lines

Quoted 4 lines
But then, there's another way to mask INITs in the wait-for-SIPI state. Considering that KVM_MP_STATE_INIT_RECEIVED is really a wait-for-SIPI, you can do:
Haven't checked it for races (especially races between multiple CPUS sending INIT), but looks more complicated to me.

Ok, let's go with yours.

Paolo

Gleb Natapov

author

2013-05-30 01:20 UTC #6 lore raw

Quoted 22 lines · Paolo Bonzini

Il 28/05/2013 17:00, Gleb Natapov ha scritto:
Quoted 21 lines · Paolo Bonzini
Quoted 20 lines
Il 28/05/2013 14:56, Gleb Natapov ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/lapic.c +0 -1
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
Quoted 8 lines
Quoted 6 lines
Review
Quoted 6 lines
(quoted diff) +5 -0
+	/*
+	 * 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).
My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts

Quoted 6 lines

Quoted 2 lines
You assume that the next processing will actually happen, but this is not necessary the case.
Why not? The INIT and SIPI that have just been sent have kicked the VCPU again.

kick is a nop if vcpu thread is not in a halt or in a guest.

Quoted 10 lines

Quoted 6 lines
Quoted 4 lines
But then, there's another way to mask INITs in the wait-for-SIPI state. Considering that KVM_MP_STATE_INIT_RECEIVED is really a wait-for-SIPI, you can do:
Haven't checked it for races (especially races between multiple CPUS sending INIT), but looks more complicated to me.
Ok, let's go with yours.
Paolo

--
Gleb.

Paolo Bonzini

maintainer

2013-05-30 05:41 UTC #7 lore raw

Il 30/05/2013 03:20, Gleb Natapov ha scritto:

Quoted 23 lines · Paolo Bonzini

Quoted 22 lines
Il 28/05/2013 17:00, Gleb Natapov ha scritto:
Quoted 21 lines · Paolo Bonzini
Quoted 20 lines
Il 28/05/2013 14:56, Gleb Natapov ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/lapic.c +0 -1
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
Quoted 8 lines
Quoted 6 lines
Review
Quoted 6 lines
(quoted diff) +5 -0
+	/*
+	 * 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).
My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
You assume that the next processing will actually happen, but this is not necessary the case.
Why not? The INIT and SIPI that have just been sent have kicked the VCPU again.
kick is a nop if vcpu thread is not in a halt or in a guest.

But the KVM_REQ_EVENT request will be caught at:

        if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
            || need_resched() || signal_pending(current)) {
                vcpu->mode = OUTSIDE_GUEST_MODE;
                smp_wmb();
                local_irq_enable();
                preempt_enable();
                r = 1;
                goto cancel_injection;
        }

and the entry will be canceled.

Paolo

Gleb Natapov

author

2013-05-30 06:01 UTC #8 lore raw

Quoted 44 lines · Paolo Bonzini

Il 30/05/2013 03:20, Gleb Natapov ha scritto:
Quoted 23 lines · Paolo Bonzini
Quoted 22 lines
Il 28/05/2013 17:00, Gleb Natapov ha scritto:
Quoted 21 lines · Paolo Bonzini
Quoted 20 lines
Il 28/05/2013 14:56, Gleb Natapov ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/lapic.c +0 -1
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
Quoted 8 lines
Quoted 6 lines
Review
Quoted 6 lines
(quoted diff) +5 -0
+	/*
+	 * 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).
My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
You assume that the next processing will actually happen, but this is not necessary the case.
Why not? The INIT and SIPI that have just been sent have kicked the VCPU again.
kick is a nop if vcpu thread is not in a halt or in a guest.
But the KVM_REQ_EVENT request will be caught at:
        if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
            || need_resched() || signal_pending(current)) {
                vcpu->mode = OUTSIDE_GUEST_MODE;
                smp_wmb();
                local_irq_enable();
                preempt_enable();
                r = 1;
                goto cancel_injection;
        }

Quoted 2 lines

and the entry will be canceled.

But vcpu may be in non running state so we will not get here.

--
Gleb.

Paolo Bonzini

maintainer

2013-05-30 06:31 UTC #9 lore raw

Il 30/05/2013 08:01, Gleb Natapov ha scritto:

Quoted 45 lines · Paolo Bonzini

Quoted 44 lines
Il 30/05/2013 03:20, Gleb Natapov ha scritto:
Quoted 23 lines · Paolo Bonzini
Quoted 22 lines
Il 28/05/2013 17:00, Gleb Natapov ha scritto:
Quoted 21 lines · Paolo Bonzini
Quoted 20 lines
Il 28/05/2013 14:56, Gleb Natapov ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/lapic.c +0 -1
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
Quoted 8 lines
Quoted 6 lines
Review
Quoted 6 lines
(quoted diff) +5 -0
+	/*
+	 * 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).
My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
You assume that the next processing will actually happen, but this is not necessary the case.
Why not? The INIT and SIPI that have just been sent have kicked the VCPU again.
kick is a nop if vcpu thread is not in a halt or in a guest.
But the KVM_REQ_EVENT request will be caught at:
        if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
            || need_resched() || signal_pending(current)) {
                vcpu->mode = OUTSIDE_GUEST_MODE;
                smp_wmb();
                local_irq_enable();
                preempt_enable();
                r = 1;
                goto cancel_injection;
        }

Quoted 1 line

Quoted 1 line
and the entry will be canceled.

I was wrong: we exit immediately because state is
KVM_MP_STATE_INIT_RECEIVED. But then...

Quoted 1 line

But vcpu may be in non running state so we will not get here.

... vcpu_enter_guest will return 1 and __vcpu_run goes around the while loop once more (modulo pending signals of course).

On the next iteration __vcpu_run will call kvm_vcpu_block, which calls kvm_arch_vcpu_runnable. kvm_arch_vcpu_runnable returns true because kvm_apic_has_events(vcpu) is also true. This will set KVM_REQ_UNHALT, call kvm_apic_accept_events again and do the INIT+SIPI.

Paolo

Gleb Natapov

author

2013-05-30 07:09 UTC #10 lore raw

Quoted 53 lines · Paolo Bonzini

Il 30/05/2013 08:01, Gleb Natapov ha scritto:
Quoted 45 lines · Paolo Bonzini
Quoted 44 lines
Il 30/05/2013 03:20, Gleb Natapov ha scritto:
Quoted 23 lines · Paolo Bonzini
Quoted 22 lines
Il 28/05/2013 17:00, Gleb Natapov ha scritto:
Quoted 21 lines · Paolo Bonzini
Quoted 20 lines
Il 28/05/2013 14:56, Gleb Natapov ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -1
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
Quoted 8 lines
Quoted 6 lines
Review
Quoted 6 lines
(quoted diff) +5 -0
+	/*
+	 * 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).
My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
You assume that the next processing will actually happen, but this is not necessary the case.
Why not? The INIT and SIPI that have just been sent have kicked the VCPU again.
kick is a nop if vcpu thread is not in a halt or in a guest.
But the KVM_REQ_EVENT request will be caught at:
        if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
            || need_resched() || signal_pending(current)) {
                vcpu->mode = OUTSIDE_GUEST_MODE;
                smp_wmb();
                local_irq_enable();
                preempt_enable();
                r = 1;
                goto cancel_injection;
        }
Quoted 1 line
Quoted 1 line
and the entry will be canceled.
I was wrong: we exit immediately because state is
KVM_MP_STATE_INIT_RECEIVED. But then...
Quoted 1 line
But vcpu may be in non running state so we will not get here.

Quoted 8 lines

... vcpu_enter_guest will return 1 and __vcpu_run goes around the while loop once more (modulo pending signals of course).
On the next iteration __vcpu_run will call kvm_vcpu_block, which calls kvm_arch_vcpu_runnable. kvm_arch_vcpu_runnable returns true because kvm_apic_has_events(vcpu) is also true. This will set KVM_REQ_UNHALT, call kvm_apic_accept_events again and do the INIT+SIPI.

Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not lose the event.

--
Gleb.

Paolo Bonzini

maintainer

2013-05-30 07:30 UTC #11 lore raw

Il 30/05/2013 09:09, Gleb Natapov ha scritto:

Quoted 57 lines · Paolo Bonzini

Quoted 56 lines
Il 30/05/2013 08:01, Gleb Natapov ha scritto:
Quoted 45 lines · Paolo Bonzini
Quoted 44 lines
Il 30/05/2013 03:20, Gleb Natapov ha scritto:
Quoted 23 lines · Paolo Bonzini
Quoted 22 lines
Il 28/05/2013 17:00, Gleb Natapov ha scritto:
Quoted 21 lines · Paolo Bonzini
Quoted 20 lines
Il 28/05/2013 14:56, Gleb Natapov ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -1
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
Quoted 8 lines
Quoted 6 lines
Review
Quoted 6 lines
(quoted diff) +5 -0
+	/*
+	 * 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).
My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
You assume that the next processing will actually happen, but this is not necessary the case.
Why not? The INIT and SIPI that have just been sent have kicked the VCPU again.
kick is a nop if vcpu thread is not in a halt or in a guest.
But the KVM_REQ_EVENT request will be caught at:
        if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
            || need_resched() || signal_pending(current)) {
                vcpu->mode = OUTSIDE_GUEST_MODE;
                smp_wmb();
                local_irq_enable();
                preempt_enable();
                r = 1;
                goto cancel_injection;
        }
Quoted 1 line
Quoted 1 line
and the entry will be canceled.
I was wrong: we exit immediately because state is
KVM_MP_STATE_INIT_RECEIVED. But then...
Quoted 1 line
But vcpu may be in non running state so we will not get here.
... vcpu_enter_guest will return 1 and __vcpu_run goes around the while loop once more (modulo pending signals of course).

Quoted 7 lines

Quoted 4 lines
On the next iteration __vcpu_run will call kvm_vcpu_block, which calls kvm_arch_vcpu_runnable. kvm_arch_vcpu_runnable returns true because kvm_apic_has_events(vcpu) is also true. This will set KVM_REQ_UNHALT, call kvm_apic_accept_events again and do the INIT+SIPI.
Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not lose the event.

Ok, then I'd prefer to have the cmpxchg directly in the if, as in
http://article.gmane.org/gmane.comp.emulators.kvm.devel/110505

Thanks for the discussion!

Paolo

Gleb Natapov

author

2013-05-30 12:34 UTC #12 lore raw

Quoted 63 lines · Paolo Bonzini

Il 30/05/2013 09:09, Gleb Natapov ha scritto:
Quoted 62 lines · Paolo Bonzini
Quoted 60 lines
Il 30/05/2013 08:01, Gleb Natapov ha scritto:
Quoted 45 lines · Paolo Bonzini
Quoted 44 lines
Il 30/05/2013 03:20, Gleb Natapov ha scritto:
Quoted 23 lines · Paolo Bonzini
Quoted 22 lines
Il 28/05/2013 17:00, Gleb Natapov ha scritto:
Quoted 21 lines · Paolo Bonzini
Quoted 20 lines
Il 28/05/2013 14:56, Gleb Natapov ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -1
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
Quoted 8 lines
Quoted 6 lines
Review
Quoted 6 lines
(quoted diff) +5 -0
+	/*
+	 * 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).
My patch does "do another INIT (which will have no effect) and do SIPI after that INIT", which is different but has almost the same effect. If pending_events is INIT/SIPI, it ignores the SIPI for now and lets the next iteration of kvm_apic_accept_events do both. The difference would be that in a carefully-timed sequence of interrupts
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
You assume that the next processing will actually happen, but this is not necessary the case.
Why not? The INIT and SIPI that have just been sent have kicked the VCPU again.
kick is a nop if vcpu thread is not in a halt or in a guest.
But the KVM_REQ_EVENT request will be caught at:
        if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
            || need_resched() || signal_pending(current)) {
                vcpu->mode = OUTSIDE_GUEST_MODE;
                smp_wmb();
                local_irq_enable();
                preempt_enable();
                r = 1;
                goto cancel_injection;
        }
Quoted 1 line
Quoted 1 line
and the entry will be canceled.
I was wrong: we exit immediately because state is
KVM_MP_STATE_INIT_RECEIVED. But then...
Quoted 1 line
But vcpu may be in non running state so we will not get here.
... vcpu_enter_guest will return 1 and __vcpu_run goes around the while loop once more (modulo pending signals of course).
On the next iteration __vcpu_run will call kvm_vcpu_block, which calls kvm_arch_vcpu_runnable. kvm_arch_vcpu_runnable returns true because kvm_apic_has_events(vcpu) is also true. This will set KVM_REQ_UNHALT, call kvm_apic_accept_events again and do the INIT+SIPI.

Quoted 6 lines

Quoted 2 lines
Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not lose the event.
Ok, then I'd prefer to have the cmpxchg directly in the if, as in
http://article.gmane.org/gmane.comp.emulators.kvm.devel/110505

I still do not. Both of them are tricky, mine does not coalesce events needlessly.

Quoted 3 lines

Thanks for the discussion!
Paolo

--
Gleb.

Paolo Bonzini

maintainer

2013-05-30 12:58 UTC #13 lore raw

Il 30/05/2013 14:34, Gleb Natapov ha scritto:

Quoted 9 lines

Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not lose the event.
Ok, then I'd prefer to have the cmpxchg directly in the if, as in
http://article.gmane.org/gmane.comp.emulators.kvm.devel/110505
I still do not. Both of them are tricky, mine does not coalesce events needlessly.

Agreed that both are tricky, but I don't think my patch is coalescing events. If you have

INIT SIPI INIT SIPI
^ ^
INIT bit cleared here SIPI bit checked here

my patch KVM sees apic_events = INIT | SIPI and deduces that the SIPI bit was set by the second SIPI, not by the first. In fact the first SIPI was cancelled by the second INIT, and thus should not be processed at all.

Instead, with your patch KVM will service all four events; strictly speaking it is wrong to service the first SIPI, which is why I prefer having the cmpxchg in the beginning.

Paolo

Gleb Natapov

author

2013-05-30 13:10 UTC #14 lore raw

Quoted 11 lines · Paolo Bonzini

Il 30/05/2013 14:34, Gleb Natapov ha scritto:
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not lose the event.
Ok, then I'd prefer to have the cmpxchg directly in the if, as in
http://article.gmane.org/gmane.comp.emulators.kvm.devel/110505
I still do not. Both of them are tricky, mine does not coalesce events needlessly.

Quoted 7 lines

Agreed that both are tricky, but I don't think my patch is coalescing events. If you have
INIT SIPI INIT SIPI
^ ^
INIT bit cleared here SIPI bit checked here

Not sure I understand what you are trying to say here.

Quoted 4 lines

my patch KVM sees apic_events = INIT | SIPI and deduces that the SIPI bit was set by the second SIPI, not by the first. In fact the first SIPI was cancelled by the second INIT, and thus should not be processed at all.

That is called coalesced.

Quoted 5 lines

Instead, with your patch KVM will service all four events; strictly speaking it is wrong to service the first SIPI, which is why I prefer having the cmpxchg in the beginning.
y

Why is it wrong?

I do not see what are you arguing about.

--
Gleb.

Paolo Bonzini

maintainer

2013-05-30 13:23 UTC #15 lore raw

Il 30/05/2013 15:10, Gleb Natapov ha scritto:

Quoted 12 lines · Paolo Bonzini

Quoted 11 lines
Il 30/05/2013 14:34, Gleb Natapov ha scritto:
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not lose the event.
Ok, then I'd prefer to have the cmpxchg directly in the if, as in
http://article.gmane.org/gmane.comp.emulators.kvm.devel/110505
I still do not. Both of them are tricky, mine does not coalesce events needlessly.

Quoted 8 lines

Quoted 7 lines
Agreed that both are tricky, but I don't think my patch is coalescing events. If you have
INIT SIPI INIT SIPI
^ ^
INIT bit cleared here SIPI bit checked here
Not sure I understand what you are trying to say here.

I'll redo the picture below.

Quoted 5 lines

Quoted 4 lines
my patch KVM sees apic_events = INIT | SIPI and deduces that the SIPI bit was set by the second SIPI, not by the first. In fact the first SIPI was cancelled by the second INIT, and thus should not be processed at all.
That is called coalesced.

Coalescing would be something like INIT SIPI SIPI -> INIT SIPI. This is not coalescing, it is proper detection of a cancelled SIPI. We have:

event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
XX INIT INIT

    SIPI                                               INIT|SIPI
YY                           SIPI                      INIT|SIPI
                        failed cmpxchg                 INIT|SIPI

INIT SIPI
SIPI 0

At the line I marked with YY, you're processing an interrupt that is not anymore in the pending_events. It was dropped at point XX.

With my patch it is:

event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
XX INIT INIT
SIPI INIT|SIPI
failed cmpxchg INIT|SIPI
INIT SIPI
SIPI 0

Quoted 5 lines

Quoted 3 lines
Instead, with your patch KVM will service all four events; strictly speaking it is wrong to service the first SIPI, which is why I prefer having the cmpxchg in the beginning.
Why is it wrong?

Because the first SIPI was dropped atomically with the triggering of the second INIT, it's as if you were handling it twice.

Paolo

Gleb Natapov

author

2013-05-30 13:35 UTC #16 lore raw

Quoted 38 lines · Paolo Bonzini

Il 30/05/2013 15:10, Gleb Natapov ha scritto:
Quoted 12 lines · Paolo Bonzini
Quoted 11 lines
Il 30/05/2013 14:34, Gleb Natapov ha scritto:
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not lose the event.
Ok, then I'd prefer to have the cmpxchg directly in the if, as in
http://article.gmane.org/gmane.comp.emulators.kvm.devel/110505
I still do not. Both of them are tricky, mine does not coalesce events needlessly.
Quoted 8 lines
Quoted 7 lines
Agreed that both are tricky, but I don't think my patch is coalescing events. If you have
INIT SIPI INIT SIPI
^ ^
INIT bit cleared here SIPI bit checked here
Not sure I understand what you are trying to say here.
I'll redo the picture below.
Quoted 5 lines
Quoted 4 lines
my patch KVM sees apic_events = INIT | SIPI and deduces that the SIPI bit was set by the second SIPI, not by the first. In fact the first SIPI was cancelled by the second INIT, and thus should not be processed at all.
That is called coalesced.
Coalescing would be something like INIT SIPI SIPI -> INIT SIPI. This is not coalescing, it is proper detection of a cancelled SIPI. We have:
event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
XX INIT INIT

Quoted 6 lines

    SIPI                                               INIT|SIPI
YY                           SIPI                      INIT|SIPI
                        failed cmpxchg                 INIT|SIPI

INIT SIPI
SIPI 0

At this point I am not even sure that you understand what problem the patch is fixing, because the bug is not event triggered by above sequence.

Quoted 16 lines

At the line I marked with YY, you're processing an interrupt that is not anymore in the pending_events. It was dropped at point XX.
With my patch it is:
event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
XX INIT INIT
SIPI INIT|SIPI
failed cmpxchg INIT|SIPI
INIT SIPI
SIPI 0

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
Instead, with your patch KVM will service all four events; strictly speaking it is wrong to service the first SIPI, which is why I prefer having the cmpxchg in the beginning.
Why is it wrong?
Because the first SIPI was dropped atomically with the triggering of the second INIT, it's as if you were handling it twice.

No, you were slow to process first SIPI, so second INIT was sent because vcpu appears to be dead, so instead of processing both you process last. This is exactly what coalescing means, you are just trying to present is as something desirable.

--
Gleb.

Paolo Bonzini

maintainer

2013-05-30 14:15 UTC #17 lore raw

Il 30/05/2013 15:35, Gleb Natapov ha scritto:

Quoted 45 lines · Paolo Bonzini

Quoted 38 lines
Il 30/05/2013 15:10, Gleb Natapov ha scritto:
Quoted 12 lines · Paolo Bonzini
Quoted 11 lines
Il 30/05/2013 14:34, Gleb Natapov ha scritto:
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not lose the event.
Ok, then I'd prefer to have the cmpxchg directly in the if, as in
http://article.gmane.org/gmane.comp.emulators.kvm.devel/110505
I still do not. Both of them are tricky, mine does not coalesce events needlessly.
Quoted 8 lines
Quoted 7 lines
Agreed that both are tricky, but I don't think my patch is coalescing events. If you have
INIT SIPI INIT SIPI
^ ^
INIT bit cleared here SIPI bit checked here
Not sure I understand what you are trying to say here.
I'll redo the picture below.
Quoted 5 lines
Quoted 4 lines
my patch KVM sees apic_events = INIT | SIPI and deduces that the SIPI bit was set by the second SIPI, not by the first. In fact the first SIPI was cancelled by the second INIT, and thus should not be processed at all.
That is called coalesced.
Coalescing would be something like INIT SIPI SIPI -> INIT SIPI. This is not coalescing, it is proper detection of a cancelled SIPI. We have:
event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
XX INIT INIT
Quoted 6 lines
    SIPI                                               INIT|SIPI
YY                           SIPI                      INIT|SIPI
                        failed cmpxchg                 INIT|SIPI
INIT SIPI
SIPI 0

Quoted 2 lines

At this point I am not even sure that you understand what problem the patch is fixing, because the bug is not event triggered by above sequence.

Maybe.

Quoted 5 lines

Quoted 3 lines
Because the first SIPI was dropped atomically with the triggering of the second INIT, it's as if you were handling it twice.
No, you were slow to process first SIPI, so second INIT was sent because vcpu appears to be dead, so instead of processing both you process last.

Can you draw the events that happen?

What I drew above is based on the commit message. Instead what I understand from this explanation is:

event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
SIPI 0
INIT INIT
SIPI INIT|SIPI
INIT SIPI
SIPI 0

Then my patch has absolutely no effect, the cmpxchg succeeds. With your patch instead I get:

event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
SIPI ...
INIT INIT
SIPI INIT|SIPI
failed cmpxchg INIT|SIPI
INIT SIPI
SIPI ...
successful cmpxchg 0

But there is no difference in the actual set of events that was processed.

Paolo

Gleb Natapov

author

2013-05-31 04:36 UTC #18 lore raw

Quoted 51 lines · Paolo Bonzini

Il 30/05/2013 15:35, Gleb Natapov ha scritto:
Quoted 45 lines · Paolo Bonzini
Quoted 38 lines
Il 30/05/2013 15:10, Gleb Natapov ha scritto:
Quoted 12 lines · Paolo Bonzini
Quoted 11 lines
Il 30/05/2013 14:34, Gleb Natapov ha scritto:
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not lose the event.
Ok, then I'd prefer to have the cmpxchg directly in the if, as in
http://article.gmane.org/gmane.comp.emulators.kvm.devel/110505
I still do not. Both of them are tricky, mine does not coalesce events needlessly.
Quoted 8 lines
Quoted 7 lines
Agreed that both are tricky, but I don't think my patch is coalescing events. If you have
INIT SIPI INIT SIPI
^ ^
INIT bit cleared here SIPI bit checked here
Not sure I understand what you are trying to say here.
I'll redo the picture below.
Quoted 5 lines
Quoted 4 lines
my patch KVM sees apic_events = INIT | SIPI and deduces that the SIPI bit was set by the second SIPI, not by the first. In fact the first SIPI was cancelled by the second INIT, and thus should not be processed at all.
That is called coalesced.
Coalescing would be something like INIT SIPI SIPI -> INIT SIPI. This is not coalescing, it is proper detection of a cancelled SIPI. We have:
event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
XX INIT INIT
Quoted 6 lines
    SIPI                                               INIT|SIPI
YY                           SIPI                      INIT|SIPI
                        failed cmpxchg                 INIT|SIPI
INIT SIPI
SIPI 0
Quoted 2 lines
At this point I am not even sure that you understand what problem the patch is fixing, because the bug is not event triggered by above sequence.
Maybe.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Because the first SIPI was dropped atomically with the triggering of the second INIT, it's as if you were handling it twice.
No, you were slow to process first SIPI, so second INIT was sent because vcpu appears to be dead, so instead of processing both you process last.
Can you draw the events that happen?

I did, in commit message.

Quoted 3 lines

What I drew above is based on the commit message. Instead what I understand from this explanation is:

It is definitely not based on my commit message :)

In my commit message there is two INITs in a row:
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

Two INITs before SIPI are essential to trigger the bug and coincidentally this is what spec advices to do.

Quoted 26 lines

event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
SIPI 0
INIT INIT
SIPI INIT|SIPI
INIT SIPI
SIPI 0
Then my patch has absolutely no effect, the cmpxchg succeeds. With your patch instead I get:
event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
SIPI ...
INIT INIT
SIPI INIT|SIPI
failed cmpxchg INIT|SIPI
INIT SIPI
SIPI ...
successful cmpxchg 0

Quoted 2 lines

But there is no difference in the actual set of events that was processed.

I do not get what you are trying to tell with this. The scenario you are repeatedly describing works with your path, with my patch and without any patch at all.

--
Gleb.

Paolo Bonzini

maintainer

2013-05-31 08:48 UTC #19 lore raw

Il 31/05/2013 06:36, Gleb Natapov ha scritto:

Quoted 11 lines

In my commit message there is two INITs in a row:
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
Two INITs before SIPI are essential to trigger the bug

I see now. Let's draw pending_events as well:

event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
SIPI INIT
INIT 0

Events are reordered, there is indeed a bug if the second INIT comes at just the right time. With your patch:

event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
SIPI, failed cmpxchg INIT|SIPI
INIT SIPI
SIPI SIPI

The patch introduces a spurious SIPI, that's worse than coalescing. With my patch:

event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
(failed cmpxchg) INIT|SIPI
INIT SIPI
SIPI 0

My patch looks better to me for this scenario.

Quoted 1 line

and coincidentally this is what spec advices to do.

The spec advises INIT-SIPI-SIPI, not INIT-INIT-SIPI. This is because the first INIT may have been processed late, and SIPIs are masked if not in wait-for-SIPI state. You need to send the second just in case. It is not needed in KVM because INITs effectively unmask the SIPI immediately, even though the INIT may take place a bit later.

The INIT-SIPI-SIPI sequence is handled correctly by KVM thanks to the check on the mp-state. But your patch breaks another corner case:

event sent event processed pending_events
INIT INIT
INIT 0
SIPI SIPI
SIPI 0
SIPI SIPI
ignored SIPI SIPI

set_mp_state(INIT_RECEIVED) SIPI
SIPI 0

With my patch, or no patch at all:

event sent event processed pending_events
INIT INIT
INIT 0
SIPI SIPI
SIPI 0
SIPI SIPI
ignored SIPI 0
set_mp_state(INIT_RECEIVED) 0

Though perhaps the real bug here is in kvm_arch_vcpu_ioctl_setmp_state. Setting the mp_state to anything bug SIPI_RECEIVED should clear the SIPI event.

Paolo

Quoted 9 lines

Quoted 9 lines
event sent event processed pending_events
INIT INIT
SIPI INIT|SIPI
INIT SIPI
SIPI 0
INIT INIT
SIPI INIT|SIPI
INIT SIPI
SIPI 0

Gleb Natapov

author

2013-05-31 09:18 UTC #20 lore raw

Quoted 26 lines · Paolo Bonzini

Il 31/05/2013 06:36, Gleb Natapov ha scritto:
Quoted 11 lines
In my commit message there is two INITs in a row:
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
Two INITs before SIPI are essential to trigger the bug
I see now. Let's draw pending_events as well:
event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
SIPI INIT
INIT 0
Events are reordered, there is indeed a bug if the second INIT comes at just the right time. With your patch:

Quoted 9 lines

event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
SIPI, failed cmpxchg INIT|SIPI
INIT SIPI
SIPI SIPI

This is incorrect. cmpxchg will fail only if another INIT cames after SIPI. Why would it fail?

Quoted 14 lines

The patch introduces a spurious SIPI, that's worse than coalescing. With my patch:
event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
(failed cmpxchg) INIT|SIPI
INIT SIPI
SIPI 0
My patch looks better to me for this scenario.

Quoted 8 lines

Quoted 1 line
and coincidentally this is what spec advices to do.
The spec advises INIT-SIPI-SIPI, not INIT-INIT-SIPI. This is because the first INIT may have been processed late, and SIPIs are masked if not in wait-for-SIPI state. You need to send the second just in case. It is not needed in KVM because INITs effectively unmask the SIPI immediately, even though the INIT may take place a bit later.

OK, I said this from memory since I cannot check the spec now. In this case we need to fix unit test too.

Quoted 25 lines

The INIT-SIPI-SIPI sequence is handled correctly by KVM thanks to the check on the mp-state. But your patch breaks another corner case:
event sent event processed pending_events
INIT INIT
INIT 0
SIPI SIPI
SIPI 0
SIPI SIPI
ignored SIPI SIPI
set_mp_state(INIT_RECEIVED) SIPI
SIPI 0
With my patch, or no patch at all:
event sent event processed pending_events
INIT INIT
INIT 0
SIPI SIPI
SIPI 0
SIPI SIPI
ignored SIPI 0
set_mp_state(INIT_RECEIVED) 0

Quoted 1 line

Though perhaps the real bug here is in kvm_arch_vcpu_ioctl_setmp_state.

Looks like it, also in my patch we can always call cmpxchg to clear SIPI.

Paolo Bonzini

maintainer

2013-05-31 09:48 UTC #21 lore raw

Il 31/05/2013 11:18, Gleb Natapov ha scritto:

Quoted 36 lines · Paolo Bonzini

Quoted 26 lines
Il 31/05/2013 06:36, Gleb Natapov ha scritto:
Quoted 11 lines
In my commit message there is two INITs in a row:
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
Two INITs before SIPI are essential to trigger the bug
I see now. Let's draw pending_events as well:
event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
SIPI INIT
INIT 0
Events are reordered, there is indeed a bug if the second INIT comes at just the right time. With your patch:
Quoted 8 lines
event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
SIPI, failed cmpxchg INIT|SIPI
INIT SIPI
SIPI SIPI

Quoted 2 lines

This is incorrect. cmpxchg will fail only if another INIT cames after SIPI. Why would it fail?

You're right.

Can you show what is the case in my patch where you have coalescing? I still prefer it because it is a smaller change, it keeps the "clear a bit before processing" idea that you find almost everywhere. Changing it to "clear a bit after processing" is a bigger and more surprising change, though both are indeed tricky.

Paolo

Gleb Natapov

author

2013-06-02 13:14 UTC #22 lore raw

Quoted 37 lines · Paolo Bonzini

Il 31/05/2013 11:18, Gleb Natapov ha scritto:
Quoted 36 lines · Paolo Bonzini
Quoted 34 lines
Il 31/05/2013 06:36, Gleb Natapov ha scritto:
Quoted 11 lines
In my commit message there is two INITs in a row:
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
Two INITs before SIPI are essential to trigger the bug
I see now. Let's draw pending_events as well:
event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
SIPI INIT
INIT 0
Events are reordered, there is indeed a bug if the second INIT comes at just the right time. With your patch:
event sent event processed pending_events
INIT INIT
INIT 0
INIT INIT
SIPI INIT|SIPI
SIPI, failed cmpxchg INIT|SIPI
INIT SIPI
SIPI SIPI

Quoted 6 lines

Quoted 2 lines
This is incorrect. cmpxchg will fail only if another INIT cames after SIPI. Why would it fail?
You're right.
Can you show what is the case in my patch where you have coalescing? I

You'ev said it in some of your emails. Quoting:
" INIT-INIT-SIPI-INIT-SIPI

your version would do many SIPIs, while mine would do just one."

Quoted 5 lines

still prefer it because it is a smaller change, it keeps the "clear a bit before processing" idea that you find almost everywhere. Changing it to "clear a bit after processing" is a bigger and more surprising change, though both are indeed tricky.

There is nothing "surprising" in it for me. Really it is so subjection that arguing about it is waste of everybody time and energy. So if we want to continue have fun arguing about it lets move to some real patch problems/benefits. So what I didn't like from the start about pending_events is that it introduces two locked instruction on each interrupt injection path, your patch makes it worse by change one of those locked instruction to cmpxchg, while mine actually removes one. But I think we can do even better and get rid of both of them for common case and do only one locked inst while there are events pending, but this is slow path so less important:

arch/x86/kvm/lapic.c +6 -3

index 9d75193..3e0e85a 100644
@@ -1850,11 +1850,14 @@ 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))
+	if (!kvm_vcpu_has_lapic(vcpu) || !apic->pending_events)
 		return;
 
-	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
+	pe = xchg(&apic->pending_events, 0);
+
+	if (test_bit(KVM_APIC_INIT, &pe)) {
 		kvm_lapic_reset(vcpu);
 		kvm_vcpu_reset(vcpu);
 		if (kvm_vcpu_is_bsp(apic->vcpu))

@@ -1862,7 +1865,7 @@ 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) &&
+	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();

--
Gleb.

Paolo Bonzini

maintainer

2013-06-02 14:32 UTC #23 lore raw

Il 02/06/2013 15:14, Gleb Natapov ha scritto:

Quoted 5 lines

Quoted 1 line
Can you show what is the case in my patch where you have coalescing? I
You'ev said it in some of your emails. Quoting:
" INIT-INIT-SIPI-INIT-SIPI
your version would do many SIPIs, while mine would do just one."

Cancelling is very different from coalescing. In the implementation we chose, we have "two wires" to the processor; an INIT interrupt means "bring SIPI wire to 0 and INIT to 1", while a SIPI means "leave INIT wire aside and bring the SIPI wire to 1". If the target processor does not sense in time that the SIPI wire is high, the signal is lost.

Coalescing means that INIT-INIT or SIPI-SIPI becomes a single SIPI. That is happening no matter what, because we have two bits not a queue.

So let's settle this as a simple disagreement on terms and move on.

Quoted 4 lines

There is nothing "surprising" in it for me. Really it is so subjection that arguing about it is waste of everybody time and energy. So if we want to continue have fun arguing about it lets move to some real patch problems/benefits.

Good idea. :)

Quoted 4 lines

So what I didn't like from the start about
pending_events is that it introduces two locked instruction on each interrupt injection path, your patch makes it worse by change one of those locked instruction to cmpxchg, while mine actually removes one.

A cmpxchg is not more expensive than a test_and_clear_bit. A cmpxchg loop would be worse, of course.

Quoted 3 lines

But I think we can do even better and get rid of both of them for common case and do only one locked inst while there are events pending, but this is slow path so less important:

It looks indeed better than both alternatives. It doesn't do the coalescing that worries you, and I can understand it relatively easily as "latching" the contents of pending_events at the beginning of kvm_apic_accept_events. Very good idea!

Quoted 6 lines

arch/x86/kvm/lapic.c +0 -0
index 9d75193..3e0e85a 100644

arch/x86/kvm/lapic.c void kvm_apic_accept_events(struct kvm_vcpu *vcpu) · line 1856

Quoted 8 lines

arch/x86/kvm/lapic.c +2 -1

@@ -1850,11 +1850,14 @@ 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))
+	if (!kvm_vcpu_has_lapic(vcpu) || !apic->pending_events)
 		return;

FWIW, this optimization is independent of the other change. It would work even on top of the current code. But of course there is no need to split it into a separate patch.

Paolo

Quoted 23 lines

arch/x86/kvm/lapic.c +4 -3

-	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
+	pe = xchg(&apic->pending_events, 0);
+
+	if (test_bit(KVM_APIC_INIT, &pe)) {
 		kvm_lapic_reset(vcpu);
 		kvm_vcpu_reset(vcpu);
 		if (kvm_vcpu_is_bsp(apic->vcpu))

@@ -1862,7 +1865,7 @@ 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) &&
+	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();
--

Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Gleb Natapov

author

2013-06-02 17:33 UTC #24 lore raw

Quoted 8 lines · Paolo Bonzini

Quoted 4 lines
So what I didn't like from the start about
pending_events is that it introduces two locked instruction on each interrupt injection path, your patch makes it worse by change one of those locked instruction to cmpxchg, while mine actually removes one.
A cmpxchg is not more expensive than a test_and_clear_bit. A cmpxchg loop would be worse, of course.

Not sure about it. cmpxchg is regarded to be expensive AFAIK, but of course how expensive depends on particular cpu.

Quoted 9 lines

Quoted 3 lines
But I think we can do even better and get rid of both of them for common case and do only one locked inst while there are events pending, but this is slow path so less important:
It looks indeed better than both alternatives. It doesn't do the coalescing that worries you, and I can understand it relatively easily as "latching" the contents of pending_events at the beginning of kvm_apic_accept_events. Very good idea!

We can do the same for event processing during vcpu entry and remove a lot of locking instruction from there.

Quoted 11 lines

arch/x86/kvm/lapic.c +1 -0

index 9d75193..3e0e85a 100644
@@ -1850,11 +1850,14 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	unsigned int sipi_vector;
+	unsigned long pe;

Quoted 8 lines

arch/x86/kvm/lapic.c void kvm_apic_accept_events(struct kvm_vcpu *vcpu) · line 1856
Quoted 3 lines
arch/x86/kvm/lapic.c +1 -1
-	if (!kvm_vcpu_has_lapic(vcpu))
+	if (!kvm_vcpu_has_lapic(vcpu) || !apic->pending_events)
 		return;
FWIW, this optimization is independent of the other change. It would work even on top of the current code. But of course there is no need to split it into a separate patch.

Agree.

Quoted 17 lines

Paolo

Quoted 15 lines

arch/x86/kvm/lapic.c +4 -2

-	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
+	pe = xchg(&apic->pending_events, 0);
+
+	if (test_bit(KVM_APIC_INIT, &pe)) {
 		kvm_lapic_reset(vcpu);
 		kvm_vcpu_reset(vcpu);
 		if (kvm_vcpu_is_bsp(apic->vcpu))

@@ -1862,7 +1865,7 @@ 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) &&
+	if (test_bit(KVM_APIC_SIPI, &pe) &&
 	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
 		/* evaluate pending_events before reading the vector */

Quoted 8 lines

Quoted 8 lines
arch/x86/kvm/lapic.c +0 -1
 		smp_rmb();
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.