From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [patch 1/3] KVM: x86: disallow multiple KVM_CREATE_IRQCHIP Date: Thu, 29 Oct 2009 12:10:30 -0200 Message-ID: <20091029141030.GA3158@amt.cnet> References: <20091028204237.663479892@amt.cnet> <20091028204359.575032781@amt.cnet> <20091029093246.GA4140@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: avi@redhat.com, kvm@vger.kernel.org, gleb@redhat.com To: "Michael S. Tsirkin" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:55612 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754387AbZJ2OLv (ORCPT ); Thu, 29 Oct 2009 10:11:51 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n9TEBuqC008854 for ; Thu, 29 Oct 2009 10:11:56 -0400 Content-Disposition: inline In-Reply-To: <20091029093246.GA4140@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Oct 29, 2009 at 11:32:48AM +0200, Michael S. Tsirkin wrote: > On Wed, Oct 28, 2009 at 06:42:38PM -0200, Marcelo Tosatti wrote: > > Otherwise kvm will leak memory on multiple KVM_CREATE_IRQCHIP. > > Also serialize multiple accesses with kvm->lock. > > > > Signed-off-by: Marcelo Tosatti > > > > Index: kvm/arch/x86/kvm/x86.c > > =================================================================== > > --- kvm.orig/arch/x86/kvm/x86.c > > +++ kvm/arch/x86/kvm/x86.c > > @@ -2362,25 +2362,38 @@ long kvm_arch_vm_ioctl(struct file *filp > > if (r) > > goto out; > > break; > > - case KVM_CREATE_IRQCHIP: > > + case KVM_CREATE_IRQCHIP: { > > + struct kvm_pic *vpic; > > + > > + mutex_lock(&kvm->lock); > > + r = -EEXIST; > > + if (kvm->arch.vpic) > > + goto create_irqchip_unlock; > > r = -ENOMEM; > > - kvm->arch.vpic = kvm_create_pic(kvm); > > - if (kvm->arch.vpic) { > > + vpic = kvm_create_pic(kvm); > > + if (vpic) { > > r = kvm_ioapic_init(kvm); > > if (r) { > > - kfree(kvm->arch.vpic); > > - kvm->arch.vpic = NULL; > > - goto out; > > + kfree(vpic); > > + goto create_irqchip_unlock; > > } > > } else > > - goto out; > > + goto create_irqchip_unlock; > > + kvm->arch.vpic = vpic; > > + smp_wmb(); > > Hmm, I think we want the reverse order: > smp_wmb(); > kvm->arch.vpic = vpic; > > The point is preventing vpic pointer > from being written to memory before vpic data itself. > Right? Point is preventing arch.vpic assignment (and everything before) from reordering with kvm_setup_default_irq_routing (you cannot have a present irq route without arch.vioapic or arch.vpic set, since kvm_set_irq assumes they are present). But, now that you say, we also want a smp_wmb before vpic assignment so the irqchip_in_kernel() test is safe (that is, everything including vioapic is in place before irqchip_in_kernel() can succeed). I'll resend, thanks. > BTW, the reason that we have wmb here but no read barrier anywhere is > that this code is x86 specific and reads are not reordered across a > dependency on x86. Writes are also not reordered on x86, so this really > acts as a compiler barrier, but I agree it's better to be portable. So > maybe, for documentation purposes, we should add read_barrier_depends > within irqchip_in_kernel()? > > > r = kvm_setup_default_irq_routing(kvm); > > if (r) { > > + mutex_lock(&kvm->irq_lock); > > kfree(kvm->arch.vpic); > > kfree(kvm->arch.vioapic); > > - goto out; > > + kvm->arch.vpic = NULL; > > + kvm->arch.vioapic = NULL; > > + mutex_unlock(&kvm->irq_lock); > > } > > + create_irqchip_unlock: > > + mutex_unlock(&kvm->lock); > > break; > > + } > > case KVM_CREATE_PIT: > > u.pit_config.flags = KVM_PIT_SPEAKER_DUMMY; > > goto create_pit; > >