From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing Date: Fri, 31 May 2013 12:18:35 +0300 Message-ID: <20130531091835.GA467@redhat.com> References: <20130530070906.GA29815@redhat.com> <51A70021.1040809@redhat.com> <20130530123454.GA4845@redhat.com> <51A74CE1.1000700@redhat.com> <20130530131054.GB5495@redhat.com> <51A752D7.1020809@redhat.com> <20130530133534.GC5495@redhat.com> <51A75F07.805@redhat.com> <20130531043643.GA26250@redhat.com> <51A863E0.9080202@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Jan Kiszka To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:7923 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754477Ab3EaJSi (ORCPT ); Fri, 31 May 2013 05:18:38 -0400 Content-Disposition: inline In-Reply-To: <51A863E0.9080202@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, May 31, 2013 at 10:48:32AM +0200, Paolo Bonzini wrote: > Il 31/05/2013 06:36, Gleb Natapov ha scritto: > > 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 > This is incorrect. cmpxchg will fail only if another INIT cames after SIPI. Why would it fail? > 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. > > > 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. > 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. Looks like it, also in my patch we can always call cmpxchg to clear SIPI.