[Qemu-devel] [PATCH 0/3] balloon: error if guest driver is not initialised

[Qemu-devel] [PATCH 0/3] balloon: error if guest driver is not initialised

[Amit Shah]

Hello,

These patches make qemu generate an error on failure in setting a
balloon value:

(qemu) balloon 400
Device 'balloon' has not been activated

This can happen when the guest hasn't yet loaded a balloon driver.

Previously, such a ballooning attempt didn't produce an error, but the
virtio-balloon driver remembered the value nevertheless, causing a
module load in the guest using the attempted balloon value
originally.  This may not be the desired course of action.

I've re-used the QERR_DEVICE_NOT_ACTIVE error message, I think it fits
in this case too.  If not, let me know and I'll change it to something
else.

Please review and apply.

Amit Shah (3):
  balloon: Make functions return 0 on OK, -1 on error.
  balloon: report error if ballooning operation fails
  virtio-balloon: report error if balloon driver in guest not available

 balloon.c           |   19 ++++++++++++-------
 balloon.h           |    2 +-
 hw/virtio-balloon.c |    6 +++++-
 3 files changed, 18 insertions(+), 9 deletions(-)

-- 
1.7.7.3

[Qemu-devel] [PATCH 1/3] balloon: Make functions return 0 on OK, -1 on error.

[Amit Shah]

Current semantics of 1 on OK and 0 on error are slightly weird.
qemu_balloon() and qemu_balloon_stats() do this.  Other functions in the
file use the standard 0 and -1 return values.  This commit makes the
file consistent in returning such values.

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

diff --git a/balloon.c b/balloon.c
index e1cd5fa..2b7131a 100644
--- a/balloon.c
+++ b/balloon.c
@@ -64,25 +64,26 @@ void qemu_remove_balloon_handler(void *opaque)
 static int qemu_balloon(ram_addr_t target)
 {
     if (!balloon_event_fn) {
-        return 0;
+        return -1;
     }
     trace_balloon_event(balloon_opaque, target);
     balloon_event_fn(balloon_opaque, target);
-    return 1;
+    return 0;
 }
 
 static int qemu_balloon_status(BalloonInfo *info)
 {
     if (!balloon_stat_fn) {
-        return 0;
+        return -1;
     }
     balloon_stat_fn(balloon_opaque, info);
-    return 1;
+    return 0;
 }
 
 BalloonInfo *qmp_query_balloon(Error **errp)
 {
     BalloonInfo *info;
+    int ret;
 
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
         error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
@@ -91,7 +92,8 @@ BalloonInfo *qmp_query_balloon(Error **errp)
 
     info = g_malloc0(sizeof(*info));
 
-    if (qemu_balloon_status(info) == 0) {
+    ret = qemu_balloon_status(info);
+    if (ret < 0) {
         error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
         qapi_free_BalloonInfo(info);
         return NULL;
@@ -120,7 +122,7 @@ int do_balloon(Monitor *mon, const QDict *params,
         return -1;
     }
     ret = qemu_balloon(target);
-    if (ret == 0) {
+    if (ret < 0) {
         qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
         return -1;
     }
-- 
1.7.7.3

[Qemu-devel] [PATCH 2/3] balloon: report error if ballooning operation fails

[Amit Shah]

Ballooning operations can fail (e.g. driver in guest not available).
Let the user know of such an error condition instead of silently
ignoring errors.

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

diff --git a/balloon.c b/balloon.c
index 2b7131a..b05c9e2 100644
--- a/balloon.c
+++ b/balloon.c
@@ -63,12 +63,15 @@ void qemu_remove_balloon_handler(void *opaque)
 
 static int qemu_balloon(ram_addr_t target)
 {
+    int ret;
+
     if (!balloon_event_fn) {
         return -1;
     }
     trace_balloon_event(balloon_opaque, target);
-    balloon_event_fn(balloon_opaque, target);
-    return 0;
+    ret = balloon_event_fn(balloon_opaque, target);
+
+    return ret < 0 ? -1 : 0;
 }
 
 static int qemu_balloon_status(BalloonInfo *info)
diff --git a/balloon.h b/balloon.h
index b36abea..23b43bd 100644
--- a/balloon.h
+++ b/balloon.h
@@ -17,7 +17,7 @@
 #include "monitor.h"
 #include "qapi-types.h"
 
-typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
+typedef int (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
 typedef void (QEMUBalloonStatus)(void *opaque, BalloonInfo *info);
 
 int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index e24a2bf..a8ecb51 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -180,7 +180,7 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
                                VIRTIO_BALLOON_PFN_SHIFT);
 }
 
-static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
+static int virtio_balloon_to_target(void *opaque, ram_addr_t target)
 {
     VirtIOBalloon *dev = opaque;
 
@@ -191,6 +191,7 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
         dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
         virtio_notify_config(&dev->vdev);
     }
+    return 0;
 }
 
 static void virtio_balloon_save(QEMUFile *f, void *opaque)
-- 
1.7.7.3

[Qemu-devel] [PATCH 3/3] virtio-balloon: report error if balloon driver in guest not available

[Amit Shah]

The guest may not have the balloon driver initialised or may have it
disabled.  Report an error to the user in such a case when a request for
ballooning arrives.

This also solves another issue where a previous request for ballooning
failed (e.g., for the reason mentioned above), but the config was still
updated, resulting in the ballooning value being applied to the guest
after the guest loads the balloon module.  This may not be the desirable
thing to do at this later stage.

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

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index a8ecb51..d692813 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -184,6 +184,9 @@ static int virtio_balloon_to_target(void *opaque, ram_addr_t target)
 {
     VirtIOBalloon *dev = opaque;
 
+    if (!(dev->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        return -ENODEV;
+    }
     if (target > ram_size) {
         target = ram_size;
     }
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH 0/3] balloon: error if guest driver is not initialised

[Daniel P. Berrange]

On Fri, Dec 09, 2011 at 05:19:35PM +0530, Amit Shah wrote:
> Hello,
> 
> These patches make qemu generate an error on failure in setting a
> balloon value:
> 
> (qemu) balloon 400
> Device 'balloon' has not been activated
> 
> This can happen when the guest hasn't yet loaded a balloon driver.

This is a pretty significant change in behaviour/semantics of the
balloon driver interface.

eg, when libvirt starts a guest, it launches QEMU paused, and
immediately sets a balloon target, which may be lower than the
memory limit, and then starts CPUS to let the guest boot. The
guest will utilize this target when it loads the balloon driver
during bootup.

With this proposed change, libvirt will immediately get
an error, and have to guess when the balloon driver might be
loaded by the guest, in order to set the limit post-boot.



> Previously, such a ballooning attempt didn't produce an error, but the
> virtio-balloon driver remembered the value nevertheless, causing a
> module load in the guest using the attempted balloon value
> originally.  This may not be the desired course of action.

This is *exactly* desired behaviour. Please don't change this.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 0/3] balloon: error if guest driver is not initialised

[Amit Shah]

On (Fri) 09 Dec 2011 [11:55:21], Daniel P. Berrange wrote:
> On Fri, Dec 09, 2011 at 05:19:35PM +0530, Amit Shah wrote:
> > Hello,
> > 
> > These patches make qemu generate an error on failure in setting a
> > balloon value:
> > 
> > (qemu) balloon 400
> > Device 'balloon' has not been activated
> > 
> > This can happen when the guest hasn't yet loaded a balloon driver.
> 
> This is a pretty significant change in behaviour/semantics of the
> balloon driver interface.
> 
> eg, when libvirt starts a guest, it launches QEMU paused, and
> immediately sets a balloon target, which may be lower than the
> memory limit, and then starts CPUS to let the guest boot. The
> guest will utilize this target when it loads the balloon driver
> during bootup.
> 
> With this proposed change, libvirt will immediately get
> an error, and have to guess when the balloon driver might be
> loaded by the guest, in order to set the limit post-boot.
> 
> > Previously, such a ballooning attempt didn't produce an error, but the
> > virtio-balloon driver remembered the value nevertheless, causing a
> > module load in the guest using the attempted balloon value
> > originally.  This may not be the desired course of action.
> 
> This is *exactly* desired behaviour. Please don't change this.

OK, thanks.

I'll retract this patch series.

		Amit