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: Fri, 31 May 2013 10:48:32 +0200 Message-ID: <51A863E0.9080202@redhat.com> References: <20130530060150.GA28173@redhat.com> <51A6F22F.2000600@redhat.com> <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> 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 mail-ee0-f54.google.com ([74.125.83.54]:44350 "EHLO mail-ee0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754343Ab3EaIsp (ORCPT ); Fri, 31 May 2013 04:48:45 -0400 Received: by mail-ee0-f54.google.com with SMTP id d49so82750eek.27 for ; Fri, 31 May 2013 01:48:43 -0700 (PDT) In-Reply-To: <20130531043643.GA26250@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 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. 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. Setting the mp_state to anything bug SIPI_RECEIVED should clear the SIPI event. Paolo >> event sent event processed pending_events >> INIT INIT >> SIPI INIT|SIPI >> INIT SIPI >> SIPI 0 >> INIT INIT >> SIPI INIT|SIPI >> INIT SIPI >> SIPI 0