vhost-net: Move asserts to after check for end < start

Submitted by Bruce Rogers on Dec. 16, 2011, 8:33 p.m.

Details

Message ID 1324067607-17055-1-git-send-email-brogers@suse.com
State New
Headers show

Commit Message

Bruce Rogers Dec. 16, 2011, 8:33 p.m.
When migrating a vm using vhost-net we hit the following assertion:

qemu-kvm: /usr/src/packages/BUILD/qemu-kvm-0.15.1/hw/vhost.c:30:
vhost_dev_sync_region: Assertion `start / (0x1000 * (8 *
sizeof(vhost_log_chunk_t))) < dev->log_size' failed.

The cases which the end < start check is intended to catch, such as
for vga video memory, will also likely trigger the assertion.
Reorder the code to handle this correctly.

Signed-off-by: Bruce Rogers <brogers@suse.com>
---
 hw/vhost.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Comments

Josh Durgin March 31, 2012, 2:21 a.m.
On 12/16/2011 12:33 PM, Bruce Rogers wrote:
> When migrating a vm using vhost-net we hit the following assertion:
>
> qemu-kvm: /usr/src/packages/BUILD/qemu-kvm-0.15.1/hw/vhost.c:30:
> vhost_dev_sync_region: Assertion `start / (0x1000 * (8 *
> sizeof(vhost_log_chunk_t)))<  dev->log_size' failed.

I consistently hit this assert while testing live migration with qemu 
1.0.1 and the master branch. Applying this patch allowed live migration 
to complete successfully. Maybe it could be reviewed and merged?

> The cases which the end<  start check is intended to catch, such as
> for vga video memory, will also likely trigger the assertion.
> Reorder the code to handle this correctly.
>
> Signed-off-by: Bruce Rogers<brogers@suse.com>
> ---
>   hw/vhost.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/vhost.c b/hw/vhost.c
> index 0870cb7..7309f71 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -26,11 +26,11 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
>       vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
>       uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
>
> -    assert(end / VHOST_LOG_CHUNK<  dev->log_size);
> -    assert(start / VHOST_LOG_CHUNK<  dev->log_size);
>       if (end<  start) {
>           return;
>       }
> +    assert(end / VHOST_LOG_CHUNK<  dev->log_size);
> +    assert(start / VHOST_LOG_CHUNK<  dev->log_size);
>       for (;from<  to; ++from) {
>           vhost_log_chunk_t log;
>           int bit;
Michael S. Tsirkin April 1, 2012, 8:54 a.m.
On Fri, Mar 30, 2012 at 07:21:22PM -0700, Josh Durgin wrote:
> On 12/16/2011 12:33 PM, Bruce Rogers wrote:
> >When migrating a vm using vhost-net we hit the following assertion:
> >
> >qemu-kvm: /usr/src/packages/BUILD/qemu-kvm-0.15.1/hw/vhost.c:30:
> >vhost_dev_sync_region: Assertion `start / (0x1000 * (8 *
> >sizeof(vhost_log_chunk_t)))<  dev->log_size' failed.
> 
> I consistently hit this assert while testing live migration with
> qemu 1.0.1 and the master branch. Applying this patch allowed live
> migration to complete successfully. Maybe it could be reviewed and
> merged?

Yes, thanks for the reminder. I've applied a patch by Alex Williamson that
addresses this and other crashes.

> >The cases which the end<  start check is intended to catch, such as
> >for vga video memory, will also likely trigger the assertion.
> >Reorder the code to handle this correctly.
> >
> >Signed-off-by: Bruce Rogers<brogers@suse.com>
> >---
> >  hw/vhost.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/vhost.c b/hw/vhost.c
> >index 0870cb7..7309f71 100644
> >--- a/hw/vhost.c
> >+++ b/hw/vhost.c
> >@@ -26,11 +26,11 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
> >      vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
> >      uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
> >
> >-    assert(end / VHOST_LOG_CHUNK<  dev->log_size);
> >-    assert(start / VHOST_LOG_CHUNK<  dev->log_size);
> >      if (end<  start) {
> >          return;
> >      }
> >+    assert(end / VHOST_LOG_CHUNK<  dev->log_size);
> >+    assert(start / VHOST_LOG_CHUNK<  dev->log_size);
> >      for (;from<  to; ++from) {
> >          vhost_log_chunk_t log;
> >          int bit;

Patch hide | download patch | download mbox

diff --git a/hw/vhost.c b/hw/vhost.c
index 0870cb7..7309f71 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -26,11 +26,11 @@  static void vhost_dev_sync_region(struct vhost_dev *dev,
     vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
     uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
 
-    assert(end / VHOST_LOG_CHUNK < dev->log_size);
-    assert(start / VHOST_LOG_CHUNK < dev->log_size);
     if (end < start) {
         return;
     }
+    assert(end / VHOST_LOG_CHUNK < dev->log_size);
+    assert(start / VHOST_LOG_CHUNK < dev->log_size);
     for (;from < to; ++from) {
         vhost_log_chunk_t log;
         int bit;