Re: [patch 1/3] KVM: x86: disallow multiple KVM_CREATE_IRQCHIP

["Michael S. Tsirkin" <mst@redhat.com>]

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