From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Subject: Re: [PATCH v4 1/6] kvm: add device control API
Date: Thu, 25 Apr 2013 13:59:20 -0500
Message-ID: <1366916360.30341.7@snotra>
References: <20130425182204.GH16740@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed
Content-Transfer-Encoding: 8BIT
Cc: Alexander Graf <agraf@suse.de>, <kvm-ppc@vger.kernel.org>,
	<kvm@vger.kernel.org>, <paulus@samba.org>
To: Gleb Natapov <gleb@redhat.com>
Return-path: <kvm-ppc-owner@vger.kernel.org>
In-Reply-To: <20130425182204.GH16740@redhat.com> (from gleb@redhat.com on
	Thu Apr 25 13:22:04 2013)
Content-Disposition: inline
Sender: kvm-ppc-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org

On 04/25/2013 01:22:04 PM, Gleb Natapov wrote:
> On Thu, Apr 25, 2013 at 11:51:08AM -0500, Scott Wood wrote:
> > On 04/25/2013 05:47:39 AM, Alexander Graf wrote:
> > >
> > >On 25.04.2013, at 11:43, Gleb Natapov wrote:
> > >
> > >>> +void kvm_device_put(struct kvm_device *dev)
> > >>> +{
> > >>> +	if (atomic_dec_and_test(&dev->users))
> > >>> +		dev->ops->destroy(dev);
> > >>> +}
> > >>> +
> > >>> +static int kvm_device_release(struct inode *inode, struct file
> > >*filp)
> > >>> +{
> > >>> +	struct kvm_device *dev = filp->private_data;
> > >>> +	struct kvm *kvm = dev->kvm;
> > >>> +
> > >>> +	kvm_device_put(dev);
> > >>> +	kvm_put_kvm(kvm);
> > >> We may put kvm only if users goes to zero, otherwise kvm can be
> > >> freed while something holds a reference to a device. Why not make
> > >> kvm_device_put() do it?
> > >
> > >Nice catch. I'll change the patch so it does the kvm_put_kvm
> > >inside kvm_device_put's destroy branch.
> >
> > No, please don't.  The KVM reference being "put" here is associated
> > with the file descriptor, not with the MPIC object.
> Is it so? Device holds a pointer to kvm, so it increments kvm  
> reference
> to make sure the pointer is valid. What prevents kvm from been  
> destroyed
> while device is still in use in current code?

Where will that kvm pointer be used, after all the file descriptors go  
away and the vcpus stop running?  mmio_mapped guards against unmapping  
the MMIO if it's already been unmapped due to KVM destruction.  We  
don't have any timers or other delayed work.

Well, I do see one place, that Alex added -- the NULLing out of  
dev->kvm->arch.mpic, which didn't exist in my patchset.

> > that change I think you'll have circular references and thus a
> > memory leak, because the vcpus can hold a reference to the MPIC
> > object.
> >
> How circular reference can be created?

MPIC holds reference on KVM, vcpu holds reference on MPIC, and vcpu is  
not destroyed until KVM is destroyed.

-Scott