[Qemu-devel] [PATCH v2 0/5] balloon: fix memleaks, invalid arguments, unplug

[Qemu-devel] [PATCH v2 0/5] balloon: fix memleaks, invalid arguments, unplug

[Amit Shah]

Hello,

This series is on top of the other balloon series for which I sent a
pull request on Tuesday.

This series fixes memleak on exit, unregisters the savevm section on
unplug, disallows negative values as ballooning targets and doesn't
allow multiple balloon device registrations.

v2 contains small tweaks suggested:
 - Error is shown by balloon.c instead of virtio-balloon.c in patch 1
   (mst)
 - Filter out negative input in do_balloon() and add qerror message
   (Markus)
 - Separate out savevm section unregistering from memleak fix


Amit Shah (5):
  balloon: Don't allow multiple balloon handler registrations
  virtio-balloon: Check if balloon registration failed
  balloon: Ignore negative balloon values
  virtio-balloon: Add exit handler, fix memleaks
  virtio-balloon: Unregister savevm section on device unplug

 balloon.c           |   20 +++++++++++++++++---
 balloon.h           |    4 ++--
 hw/virtio-balloon.c |   18 +++++++++++++++++-
 hw/virtio-pci.c     |   14 +++++++++++++-
 hw/virtio.h         |    1 +
 5 files changed, 50 insertions(+), 7 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH v2 1/5] balloon: Don't allow multiple balloon handler registrations

[Amit Shah]

Multiple balloon devices don't make sense; disallow more than one
registration attempt to register handlers.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 balloon.c |   12 ++++++++++--
 balloon.h |    4 ++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/balloon.c b/balloon.c
index a938475..5200565 100644
--- a/balloon.c
+++ b/balloon.c
@@ -36,12 +36,20 @@ static QEMUBalloonEvent *balloon_event_fn;
 static QEMUBalloonStatus *balloon_stat_fn;
 static void *balloon_opaque;
 
-void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
-                              QEMUBalloonStatus *stat_func, void *opaque)
+int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
+                             QEMUBalloonStatus *stat_func, void *opaque)
 {
+    if (balloon_event_fn || balloon_stat_fn || balloon_opaque) {
+        /* We're already registered one balloon handler.  How many can
+         * a guest really have?
+         */
+        error_report("Another balloon device already registered");
+        return -1;
+    }
     balloon_event_fn = event_func;
     balloon_stat_fn = stat_func;
     balloon_opaque = opaque;
+    return 0;
 }
 
 static int qemu_balloon(ram_addr_t target)
diff --git a/balloon.h b/balloon.h
index a6c31d5..3df14e6 100644
--- a/balloon.h
+++ b/balloon.h
@@ -20,8 +20,8 @@ typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
 typedef void (QEMUBalloonStatus)(void *opaque, MonitorCompletion cb,
                                  void *cb_data);
 
-void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
-                              QEMUBalloonStatus *stat_func, void *opaque);
+int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
+			     QEMUBalloonStatus *stat_func, void *opaque);
 
 void monitor_print_balloon(Monitor *mon, const QObject *data);
 int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque);
-- 
1.7.6

[Qemu-devel] [PATCH v2 2/5] virtio-balloon: Check if balloon registration failed

[Amit Shah]

Multiple balloon registrations are not allowed; check if the
registration with the qemu balloon api succeeded.  If not, fail the
device init.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-balloon.c |    9 ++++++++-
 hw/virtio-pci.c     |    3 +++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 2ba7e95..26ee364 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -269,6 +269,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
 VirtIODevice *virtio_balloon_init(DeviceState *dev)
 {
     VirtIOBalloon *s;
+    int ret;
 
     s = (VirtIOBalloon *)virtio_common_init("virtio-balloon",
                                             VIRTIO_ID_BALLOON,
@@ -278,12 +279,18 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
     s->vdev.set_config = virtio_balloon_set_config;
     s->vdev.get_features = virtio_balloon_get_features;
 
+    ret = qemu_add_balloon_handler(virtio_balloon_to_target,
+                                   virtio_balloon_stat, s);
+    if (ret < 0) {
+        virtio_cleanup(&s->vdev);
+        return NULL;
+    }
+
     s->ivq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
     s->dvq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
     s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
 
     reset_stats(s);
-    qemu_add_balloon_handler(virtio_balloon_to_target, virtio_balloon_stat, s);
 
     register_savevm(dev, "virtio-balloon", -1, 1,
                     virtio_balloon_save, virtio_balloon_load, s);
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index d685243..ca5f125 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -788,6 +788,9 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
     VirtIODevice *vdev;
 
     vdev = virtio_balloon_init(&pci_dev->qdev);
+    if (!vdev) {
+        return -1;
+    }
     virtio_init_pci(proxy, vdev);
     return 0;
 }
-- 
1.7.6

[Qemu-devel] [PATCH v2 3/5] balloon: Ignore negative balloon values

[Amit Shah]

Negative balloon values don't make sense, ignore them.

Reported-by: Mike Cao <bcao@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 balloon.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/balloon.c b/balloon.c
index 5200565..f56fdc1 100644
--- a/balloon.c
+++ b/balloon.c
@@ -140,6 +140,7 @@ int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque)
 int do_balloon(Monitor *mon, const QDict *params,
 	       MonitorCompletion cb, void *opaque)
 {
+    int64_t target;
     int ret;
 
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
@@ -147,7 +148,12 @@ int do_balloon(Monitor *mon, const QDict *params,
         return -1;
     }
 
-    ret = qemu_balloon(qdict_get_int(params, "value"));
+    target = qdict_get_int(params, "value");
+    if (target <= 0) {
+        qerror_report(QERR_INVALID_PARAMETER_VALUE, "target", "a size");
+        return -1;
+    }
+    ret = qemu_balloon(target);
     if (ret == 0) {
         qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
         return -1;
-- 
1.7.6

Re: [Qemu-devel] [PATCH v2 3/5] balloon: Ignore negative balloon values

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> Negative balloon values don't make sense, ignore them.

Actually, they aren't ignored, they're rejected.

Re: [Qemu-devel] [PATCH v2 3/5] balloon: Ignore negative balloon values

[Amit Shah]

On (Thu) 28 Jul 2011 [09:31:53], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Negative balloon values don't make sense, ignore them.
> 
> Actually, they aren't ignored, they're rejected.

Will update commit log.

		Amit

[Qemu-devel] [PATCH v2 4/5] virtio-balloon: Add exit handler, fix memleaks

[Amit Shah]

Add an exit handler that will free up RAM and unregister the savevm
section after a virtio-balloon device is unplugged.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-balloon.c |    5 +++++
 hw/virtio-pci.c     |   11 ++++++++++-
 hw/virtio.h         |    1 +
 3 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 26ee364..0ce0049 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -297,3 +297,8 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
 
     return &s->vdev;
 }
+
+void virtio_balloon_exit(VirtIODevice *vdev)
+{
+    virtio_cleanup(vdev);
+}
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index ca5f125..316bf92 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -795,6 +795,15 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
     return 0;
 }
 
+static int virtio_balloon_exit_pci(PCIDevice *pci_dev)
+{
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
+
+    virtio_pci_stop_ioeventfd(proxy);
+    virtio_balloon_exit(proxy->vdev);
+    return virtio_exit_pci(pci_dev);
+}
+
 static PCIDeviceInfo virtio_info[] = {
     {
         .qdev.name = "virtio-blk-pci",
@@ -869,7 +878,7 @@ static PCIDeviceInfo virtio_info[] = {
         .qdev.alias = "virtio-balloon",
         .qdev.size = sizeof(VirtIOPCIProxy),
         .init      = virtio_balloon_init_pci,
-        .exit      = virtio_exit_pci,
+        .exit      = virtio_balloon_exit_pci,
         .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET,
         .device_id = PCI_DEVICE_ID_VIRTIO_BALLOON,
         .revision  = VIRTIO_PCI_ABI_VERSION,
diff --git a/hw/virtio.h b/hw/virtio.h
index 0fd0bb0..c129264 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -213,6 +213,7 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf);
 void virtio_net_exit(VirtIODevice *vdev);
 void virtio_blk_exit(VirtIODevice *vdev);
 void virtio_serial_exit(VirtIODevice *vdev);
+void virtio_balloon_exit(VirtIODevice *vdev);
 
 #define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
 	DEFINE_PROP_BIT("indirect_desc", _state, _field, \
-- 
1.7.6

Re: [Qemu-devel] [PATCH v2 4/5] virtio-balloon: Add exit handler, fix memleaks

[Amit Shah]

On (Thu) 28 Jul 2011 [11:47:15], Amit Shah wrote:
> Add an exit handler that will free up RAM and unregister the savevm
> section after a virtio-balloon device is unplugged.

This commit message should be changed; I'll do that in the pull
request I send out.

		Amit

Re: [Qemu-devel] [PATCH v2 4/5] virtio-balloon: Add exit handler, fix memleaks

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Thu) 28 Jul 2011 [11:47:15], Amit Shah wrote:
>> Add an exit handler that will free up RAM and unregister the savevm
>> section after a virtio-balloon device is unplugged.
>
> This commit message should be changed; I'll do that in the pull
> request I send out.

You mean drop "unregister the savevm", because it has become PATCH 5/5?

Re: [Qemu-devel] [PATCH v2 4/5] virtio-balloon: Add exit handler, fix memleaks

[Amit Shah]

On (Thu) 28 Jul 2011 [09:39:40], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Thu) 28 Jul 2011 [11:47:15], Amit Shah wrote:
> >> Add an exit handler that will free up RAM and unregister the savevm
> >> section after a virtio-balloon device is unplugged.
> >
> > This commit message should be changed; I'll do that in the pull
> > request I send out.
> 
> You mean drop "unregister the savevm", because it has become PATCH 5/5?

Right.

		Amit

Re: [Qemu-devel] [PATCH v2 4/5] virtio-balloon: Add exit handler, fix memleaks

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> Add an exit handler that will free up RAM and unregister the savevm
> section after a virtio-balloon device is unplugged.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-balloon.c |    5 +++++
>  hw/virtio-pci.c     |   11 ++++++++++-
>  hw/virtio.h         |    1 +
>  3 files changed, 16 insertions(+), 1 deletions(-)
>
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 26ee364..0ce0049 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -297,3 +297,8 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
>  
>      return &s->vdev;
>  }
> +
> +void virtio_balloon_exit(VirtIODevice *vdev)
> +{
> +    virtio_cleanup(vdev);
> +}
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index ca5f125..316bf92 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -795,6 +795,15 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
>      return 0;
>  }
>  
> +static int virtio_balloon_exit_pci(PCIDevice *pci_dev)
> +{
> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> +
> +    virtio_pci_stop_ioeventfd(proxy);
> +    virtio_balloon_exit(proxy->vdev);
> +    return virtio_exit_pci(pci_dev);
> +}
> +

Same code in every other virtio_*_exit_pci().  Suggests there's
something wrong with the generic code.  Outside the scope of this
series, of course.

[...]

[Qemu-devel] [PATCH v2 5/5] virtio-balloon: Unregister savevm section on device unplug

[Amit Shah]

Migrating after unplugging a virtio-balloon device resulted in an error
message on the destination:

Unknown savevm section or instance '0000:00:04.0/virtio-balloon' 0
load of migration failed

Fix this by unregistering the section on device unplug.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-balloon.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 0ce0049..072a88a 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -45,6 +45,7 @@ typedef struct VirtIOBalloon
     size_t stats_vq_offset;
     MonitorCompletion *stats_callback;
     void *stats_opaque_callback_data;
+    DeviceState *qdev;
 } VirtIOBalloon;
 
 static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
@@ -292,6 +293,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
 
     reset_stats(s);
 
+    s->qdev = dev;
     register_savevm(dev, "virtio-balloon", -1, 1,
                     virtio_balloon_save, virtio_balloon_load, s);
 
@@ -300,5 +302,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
 
 void virtio_balloon_exit(VirtIODevice *vdev)
 {
+    VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
+    unregister_savevm(s->qdev, "virtio-balloon", s);
     virtio_cleanup(vdev);
 }
-- 
1.7.6

Re: [Qemu-devel] [PATCH v2 5/5] virtio-balloon: Unregister savevm section on device unplug

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> Migrating after unplugging a virtio-balloon device resulted in an error
> message on the destination:
>
> Unknown savevm section or instance '0000:00:04.0/virtio-balloon' 0
> load of migration failed
>
> Fix this by unregistering the section on device unplug.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-balloon.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 0ce0049..072a88a 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -45,6 +45,7 @@ typedef struct VirtIOBalloon
>      size_t stats_vq_offset;
>      MonitorCompletion *stats_callback;
>      void *stats_opaque_callback_data;
> +    DeviceState *qdev;
>  } VirtIOBalloon;

All the other virtio device structs already have such a pointer back to
the proxy.  Suggests that it should live in VirtIODevice, and be set up
in generic code.  Again, outside the scope of this series.

I hate the virtio pointer thicket.

[...]

Re: [Qemu-devel] [PATCH v2 5/5] virtio-balloon: Unregister savevm section on device unplug

[Amit Shah]

On (Thu) 28 Jul 2011 [09:45:44], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Migrating after unplugging a virtio-balloon device resulted in an error
> > message on the destination:
> >
> > Unknown savevm section or instance '0000:00:04.0/virtio-balloon' 0
> > load of migration failed
> >
> > Fix this by unregistering the section on device unplug.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/virtio-balloon.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> > index 0ce0049..072a88a 100644
> > --- a/hw/virtio-balloon.c
> > +++ b/hw/virtio-balloon.c
> > @@ -45,6 +45,7 @@ typedef struct VirtIOBalloon
> >      size_t stats_vq_offset;
> >      MonitorCompletion *stats_callback;
> >      void *stats_opaque_callback_data;
> > +    DeviceState *qdev;
> >  } VirtIOBalloon;
> 
> All the other virtio device structs already have such a pointer back to
> the proxy.  Suggests that it should live in VirtIODevice, and be set up
> in generic code.  Again, outside the scope of this series.
> 
> I hate the virtio pointer thicket.

Yep :-(

		Amit

Re: [Qemu-devel] [PATCH v2 5/5] virtio-balloon: Unregister savevm section on device unplug

[Michael S. Tsirkin]

On Thu, Jul 28, 2011 at 09:45:44AM +0200, Markus Armbruster wrote:
> I hate the virtio pointer thicket.

The problem is that each device is both a virtio pci
device and a virtio net device.

-- 
MST

Re: [Qemu-devel] [PATCH v2 5/5] virtio-balloon: Unregister savevm section on device unplug

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Jul 28, 2011 at 09:45:44AM +0200, Markus Armbruster wrote:
>> I hate the virtio pointer thicket.
>
> The problem is that each device is both a virtio pci
> device and a virtio net device.

In my possibly naive opinion, virtio-FOO-pci is a PCI device, and has a
common virtio part.

Re: [Qemu-devel] [PATCH v2 5/5] virtio-balloon: Unregister savevm section on device unplug

[Michael S. Tsirkin]

On Thu, Jul 28, 2011 at 02:21:32PM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Jul 28, 2011 at 09:45:44AM +0200, Markus Armbruster wrote:
> >> I hate the virtio pointer thicket.
> >
> > The problem is that each device is both a virtio pci
> > device and a virtio net device.
> 
> In my possibly naive opinion, virtio-FOO-pci is a PCI device, and has a
> common virtio part.

Yes, but it also has a comon virtio-pci part with other
virtio-FOO-pci devices, and a common virtio-net
part with virtio-net-XXXX.

-- 
MST

Re: [Qemu-devel] [PATCH v2 0/5] balloon: fix memleaks, invalid arguments, unplug

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> Hello,
>
> This series is on top of the other balloon series for which I sent a
> pull request on Tuesday.
>
> This series fixes memleak on exit, unregisters the savevm section on
> unplug, disallows negative values as ballooning targets and doesn't
> allow multiple balloon device registrations.
>
> v2 contains small tweaks suggested:
>  - Error is shown by balloon.c instead of virtio-balloon.c in patch 1
>    (mst)
>  - Filter out negative input in do_balloon() and add qerror message
>    (Markus)
>  - Separate out savevm section unregistering from memleak fix

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/5] balloon: fix memleaks, invalid arguments, unplug

[Michael S. Tsirkin]

On Thu, Jul 28, 2011 at 11:47:11AM +0530, Amit Shah wrote:
> Hello,
> 
> This series is on top of the other balloon series for which I sent a
> pull request on Tuesday.
> 
> This series fixes memleak on exit, unregisters the savevm section on
> unplug, disallows negative values as ballooning targets and doesn't
> allow multiple balloon device registrations.
> 
> v2 contains small tweaks suggested:
>  - Error is shown by balloon.c instead of virtio-balloon.c in patch 1
>    (mst)
>  - Filter out negative input in do_balloon() and add qerror message
>    (Markus)
>  - Separate out savevm section unregistering from memleak fix
> 
> 
> Amit Shah (5):
>   balloon: Don't allow multiple balloon handler registrations
>   virtio-balloon: Check if balloon registration failed
>   balloon: Ignore negative balloon values
>   virtio-balloon: Add exit handler, fix memleaks
>   virtio-balloon: Unregister savevm section on device unplug


Acked-by: Michael S. Tsirkin <mst@redhat.com>

>  balloon.c           |   20 +++++++++++++++++---
>  balloon.h           |    4 ++--
>  hw/virtio-balloon.c |   18 +++++++++++++++++-
>  hw/virtio-pci.c     |   14 +++++++++++++-
>  hw/virtio.h         |    1 +
>  5 files changed, 50 insertions(+), 7 deletions(-)
> 
> -- 
> 1.7.6