On Wed, 2013-10-30 at 12:40 +0200, Gleb Natapov wrote:
> On Tue, Oct 29, 2013 at 10:13:22AM -0600, Alex Williamson wrote:
> > The KVM device interface allocates a struct kvm_device and calls
> > kvm_device_ops.create on it from KVM VM ioctl KVM_CREATE_DEVICE.
> > This returns a file descriptor to the user for them to set/get/check
> > further attributes. On closing the file descriptor, one would assume
> > that kvm_device_ops.destroy is called and all traces of the device
> > would go away. One would be wrong, it actually does nothing more
> > than release the struct kvm reference, waiting until the VM is
> > destroyed before doing more. This leaves devices that only want a
> > single instance of themselves per VM in a tough spot.
> >
> This is by design. Otherwise locking will be needed on each device access
> and for interrupt controllers this is unnecessary serialization and
> overhead. Device API is not designed for devices that can go away while
> machine is running anyway, so after creation device is only destroyed
> during VM destruction.
Hmm, ok. In that case I can drop this patch and I think the rest just
boils down to userspace use of the device. I had been close()'ing the
kvm device fd when all QEMU vfio devices are detached, but I can just as
easily leave it open in case a new device is added later. I'll send out
a new series after doing some more review and testing. Do you have any
comments on the rest of the series? Thanks,
Alex
> > To fix this, do full cleanup on release of the device file descriptor.
> > It's also non-symmetric that one of the existing devices frees the
> > struct kvm_device from it's .destroy function, while the other
> > doesn't. KVM-core allocates the structure and should therefore be
> > responsible for freeing it. Finally, add a missing kfree for the
> > device creation error path.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > ---
> > arch/powerpc/kvm/book3s_xics.c | 1 -
> > virt/kvm/kvm_main.c | 5 +++++
> > 2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c
> > index a3a5cb8..9a82426 100644
> > --- a/arch/powerpc/kvm/book3s_xics.c
> > +++ b/arch/powerpc/kvm/book3s_xics.c
> > @@ -1220,7 +1220,6 @@ static void kvmppc_xics_free(struct kvm_device *dev)
> > for (i = 0; i <= xics->max_icsid; i++)
> > kfree(xics->ics[i]);
> > kfree(xics);
> > - kfree(dev);
> > }
> >
> > static int kvmppc_xics_create(struct kvm_device *dev, u32 type)
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a9dd682..fec8320 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -572,6 +572,7 @@ static void kvm_destroy_devices(struct kvm *kvm)
> >
> > list_del(node);
> > dev->ops->destroy(dev);
> > + kfree(dev);
> > }
> > }
> >
> > @@ -2231,6 +2232,9 @@ static int kvm_device_release(struct inode *inode, struct file *filp)
> > struct kvm_device *dev = filp->private_data;
> > struct kvm *kvm = dev->kvm;
> >
> > + list_del(&dev->vm_node);
> > + dev->ops->destroy(dev);
> > + kfree(dev);
> > kvm_put_kvm(kvm);
> > return 0;
> > }
> > @@ -2294,6 +2298,7 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
> > ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR | O_CLOEXEC);
> > if (ret < 0) {
> > ops->destroy(dev);
> > + kfree(dev);
> > return ret;
> > }
> >
>
> --
> Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
