From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing Date: Tue, 28 May 2013 18:33:39 +0200 Message-ID: <51A4DC63.2040906@redhat.com> References: <20130526130031.GS4725@redhat.com> <51A48D53.7070204@redhat.com> <20130528125613.GB3326@redhat.com> <51A4B5CA.9070109@redhat.com> <20130528150057.GA6891@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Jan Kiszka To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43353 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934692Ab3E1Qdv (ORCPT ); Tue, 28 May 2013 12:33:51 -0400 In-Reply-To: <20130528150057.GA6891@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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. >> 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