Re: qemu-kvm aborts - vhost_dev_unassign_memory: Assertion `to >= 0' failed.

Michael S. Tsirkin
2011-08-03T12:24:41Z

On Wed, Aug 03, 2011 at 02:48:05PM +0300, Avi Kivity wrote:
> On 08/01/2011 08:44 PM, David Ahern wrote:
> >qemu-kvm.git as of:
> >
> >commit dacdc4b10bafbb21120e1c24a9665444768ef999
> >Merge: 7b69d4f 0af4922
> >Author: Avi Kivity<avi@redhat.com>
> >Date: Sun Jul 31 11:42:26 2011 +0300
> >
> > Merge branch 'upstream-merge' into next
> >
> >is aborting with the error:
> >
> >qemu-kvm: qemu-kvm.git/hw/vhost.c:123: vhost_dev_unassign_memory:
> >Assertion `to>= 0' failed.
> >Aborted
> >
>
> It's a bug in vhost:
>
> /* Assign/unassign. Keep an unsorted array of non-overlapping
> * memory regions in dev->mem. */
> static void vhost_dev_unassign_memory(struct vhost_dev *dev,
> uint64_t start_addr,
> uint64_t size)
> {
> int from, to, n = dev->mem->nregions;
> /* Track overlapping/split regions for sanity checking. */
> int overlap_start = 0, overlap_end = 0, overlap_middle = 0, split = 0;
>
> for (from = 0, to = 0; from < n; ++from, ++to) {
> struct vhost_memory_region *reg = dev->mem->regions + to;
> uint64_t reglast;
> uint64_t memlast;
> uint64_t change;
>
> /* clone old region */
> if (to != from) {
> memcpy(reg, dev->mem->regions + from, sizeof *reg);
> }
>
> /* No overlap is simple */
> if (!ranges_overlap(reg->guest_phys_addr, reg->memory_size,
> start_addr, size)) {
> continue;
> }
>
> /* Split only happens if supplied region
> * is in the middle of an existing one. Thus it can not
> * overlap with any other existing region. */
> assert(!split);
>
> reglast = range_get_last(reg->guest_phys_addr, reg->memory_size);
> memlast = range_get_last(start_addr, size);
>
> /* Remove whole region */
> if (start_addr <= reg->guest_phys_addr && memlast >= reglast) {
> --dev->mem->nregions;
> --to;
> assert(to >= 0);
> ++overlap_middle;
> continue;
> }
>
>
> We're removing the first region, and 'to' goes negative. Michael?

Yes, that assert is wrong.

--->
Subject: vhost: remove an incorrect assert

The 'to' can go negative when the first region gets removed
(it gets incremented by to 0 immediately afterward), which
makes the assertion fail. Nothing breaks if
to < 0 here so just remove the assert.

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

----

diff --git a/hw/vhost.c b/hw/vhost.c
index c3d8821..19e7255 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -120,7 +120,6 @@ static void vhost_dev_unassign_memory(struct vhost_dev *dev,
if (start_addr <= reg->guest_phys_addr && memlast >= reglast) {
--dev->mem->nregions;
--to;
- assert(to >= 0);
++overlap_middle;
continue;
}