| Message ID | 20110803122441.GB10538@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/03/2011 06:24 AM, Michael S. Tsirkin wrote: > 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; > } > Removing the assert appears to work fine. Tested-by: David Ahern <daahern@cisco.com> David
On Wed, Aug 03, 2011 at 07:55:47AM -0600, David Ahern wrote: > Tested-by: David Ahern <daahern@cisco.com> > > David Applied, thanks very much.
On 08/03/2011 09:00 AM, Michael S. Tsirkin wrote: > On Wed, Aug 03, 2011 at 07:55:47AM -0600, David Ahern wrote: >> Tested-by: David Ahern <daahern@cisco.com> >> >> David > > Applied, thanks very much. > I assume this will make 0.15 since it is a regression? haven't seen the patch applied to that branch yet. David
On Thu, Aug 04, 2011 at 12:48:49PM -0600, David Ahern wrote: > > > On 08/03/2011 09:00 AM, Michael S. Tsirkin wrote: > > On Wed, Aug 03, 2011 at 07:55:47AM -0600, David Ahern wrote: > >> Tested-by: David Ahern <daahern@cisco.com> > >> > >> David > > > > Applied, thanks very much. > > > > I assume this will make 0.15 since it is a regression? How is this a regression in 0.15? I thought it's an old bug exposed by the memory API which I expect is not going into 0.15. > haven't seen the > patch applied to that branch yet. > > David My branch will have to get merged in trunk first.
On 08/04/2011 01:17 PM, Michael S. Tsirkin wrote: > On Thu, Aug 04, 2011 at 12:48:49PM -0600, David Ahern wrote: >> >> >> On 08/03/2011 09:00 AM, Michael S. Tsirkin wrote: >>> On Wed, Aug 03, 2011 at 07:55:47AM -0600, David Ahern wrote: >>>> Tested-by: David Ahern <daahern@cisco.com> >>>> >>>> David >>> >>> Applied, thanks very much. >>> >> >> I assume this will make 0.15 since it is a regression? > > How is this a regression in 0.15? I thought it's an old bug > exposed by the memory API which I expect is not going into 0.15. Is qemu-kvm.git not tagged as 0.15 rc? David > >> haven't seen the >> patch applied to that branch yet. >> >> David > > My branch will have to get merged in trunk first. >
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; }