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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 28/05/2013 14:56, Gleb Natapov ha scritto:
>> >  		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).

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:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index e1adbb4..36bc308 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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