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

[Gleb Natapov <gleb@redhat.com>]

On Tue, May 28, 2013 at 06:33:39PM +0200, Paolo Bonzini wrote:
> Il 28/05/2013 17:00, Gleb Natapov ha scritto:
> > On Tue, May 28, 2013 at 03:48:58PM +0200, Paolo Bonzini wrote:
> >> 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
> >>
> > 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 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.