From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing
Date: Thu, 30 May 2013 09:01:50 +0300
Message-ID: <20130530060150.GA28173@redhat.com>
References: <20130526130031.GS4725@redhat.com>
 <51A48D53.7070204@redhat.com>
 <20130528125613.GB3326@redhat.com>
 <51A4B5CA.9070109@redhat.com>
 <20130528150057.GA6891@redhat.com>
 <51A4DC63.2040906@redhat.com>
 <20130530012048.GB20766@redhat.com>
 <51A6E671.8070408@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, Jan Kiszka <jan.kiszka@siemens.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:13478 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S966640Ab3E3GBy (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 30 May 2013 02:01:54 -0400
Content-Disposition: inline
In-Reply-To: <51A6E671.8070408@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Thu, May 30, 2013 at 07:41:05AM +0200, Paolo Bonzini wrote:
> Il 30/05/2013 03:20, Gleb Natapov ha scritto:
> > 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 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.
> 
But vcpu may be in non running state so we will not get here.

--
			Gleb.