From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [patch 1/3] KVM: x86: disallow multiple KVM_CREATE_IRQCHIP
Date: Thu, 29 Oct 2009 11:32:48 +0200
Message-ID: <20091029093246.GA4140@redhat.com>
References: <20091028204237.663479892@amt.cnet> <20091028204359.575032781@amt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: avi@redhat.com, kvm@vger.kernel.org, gleb@redhat.com
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:30696 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751746AbZJ2JfE (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 29 Oct 2009 05:35:04 -0400
Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n9T9Z8BM012319
	for <kvm@vger.kernel.org>; Thu, 29 Oct 2009 05:35:09 -0400
Content-Disposition: inline
In-Reply-To: <20091028204359.575032781@amt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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 <mtosatti@redhat.com>
> 
> 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?

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;
>