From patchwork Tue May 28 10:56:19 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 2624021 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id ACAC3DFB78 for ; Tue, 28 May 2013 10:56:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758193Ab3E1K4b (ORCPT ); Tue, 28 May 2013 06:56:31 -0400 Received: from mail-qe0-f49.google.com ([209.85.128.49]:49757 "EHLO mail-qe0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757623Ab3E1K4a (ORCPT ); Tue, 28 May 2013 06:56:30 -0400 Received: by mail-qe0-f49.google.com with SMTP id a11so4188739qen.36 for ; Tue, 28 May 2013 03:56:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:x-enigmail-version:content-type :content-transfer-encoding; bh=CaU3uPDcTb8nCQ8pfOAQ+t3pbfEyzAKGcdki2IeGTPw=; b=M0k+l7F3jBIFNLjYxLVvbf5D7xSlxqNGGZ9Fe5qFpkZQ5cK3oztPAkvfq+xARCE9Jc hkw0GBnM1zRNjFk3rdo/U12snciRHwH3tO/DTqEjyUHZrfuLTPPqzgeKzrpOXkzJUJB8 WkGJ/fjQxt9sHxKAVuZmQLwUHKA886uv2i93ZB2jrzWD7zJyE2O+DK9Sn5kLriSBBpOs jge3eCDftEN3zAzDCCho4OK2WK4rgJy4eKU37b8e3IzG56mZTGihXIqOKLDaBWhd6XFC URtxIatZnBR21WYr6lRQrwIfRYg299UJ43iS+DBvpBkGkG59vPeZpxpl0rP0fWoIwblc 5/zg== X-Received: by 10.224.120.2 with SMTP id b2mr31081174qar.67.1369738589711; Tue, 28 May 2013 03:56:29 -0700 (PDT) Received: from yakj.usersys.redhat.com (net-37-117-138-128.cust.dsl.vodafone.it. [37.117.138.128]) by mx.google.com with ESMTPSA id j10sm26455544qeh.0.2013.05.28.03.56.27 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Tue, 28 May 2013 03:56:28 -0700 (PDT) Message-ID: <51A48D53.7070204@redhat.com> Date: Tue, 28 May 2013 12:56:19 +0200 From: Paolo Bonzini User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 MIME-Version: 1.0 To: Gleb Natapov CC: kvm@vger.kernel.org, Jan Kiszka Subject: Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing References: <20130526130031.GS4725@redhat.com> In-Reply-To: <20130526130031.GS4725@redhat.com> X-Enigmail-Version: 1.5.1 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Il 26/05/2013 15:00, Gleb Natapov ha scritto: > apic->pending_events processing has a race that may cause INIT and SIPI > processing to be reordered: > > 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 > > At the and INIT is left pending in pending_events. The following patch > tries to fix this using the fact that if INIT comes after SIPI it drops > SIPI from the pending_events, so if pending_events is different after > SIPI is processed it means that INIT was issued after SIPI otherwise > all pending event are processed and pending_events can be reset to zero. The patch looks correct, but is there any reason to do the cmpxchg at the end? That is, would this have any problem? It seems a bit simpler: (Even if we go with your patch, it deserves a comment in the code). Paolo > Signed-off-by: Gleb Natapov > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 9d75193..67686b8 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1850,6 +1850,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu) > { > struct kvm_lapic *apic = vcpu->arch.apic; > unsigned int sipi_vector; > + unsigned long pe; > > if (!kvm_vcpu_has_lapic(vcpu)) > return; > @@ -1862,7 +1863,8 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu) > else > vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED; > } > - if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) && > + pe = apic->pending_events; > + if (test_bit(KVM_APIC_SIPI, &pe) && > vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) { > /* evaluate pending_events before reading the vector */ > smp_rmb(); > @@ -1871,6 +1873,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu) > vcpu->vcpu_id, sipi_vector); > kvm_vcpu_deliver_sipi_vector(vcpu, sipi_vector); > vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE; > + cmpxchg(&apic->pending_events, pe, 0); > } > } > > -- > Gleb. > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index e1adbb4..3fe00fc 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1873,7 +1873,11 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu) 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) { /* evaluate pending_events before reading the vector */ smp_rmb();