[v2,03/17] memory: add ref/unref calls

Add ref/unref calls at the following places:

- places where memory regions are stashed by a listener and
  used outside the BQL (including in Xen or KVM).

- memory_region_find callsites

- creation of aliases and containers (only the aliased/contained
  region gets a reference to avoid loops)

- around calls to del_subregion/add_subregion, where the region
  could disappear after the first call

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                                |  6 +++++-
 hw/core/loader.c                      |  1 +
 hw/display/exynos4210_fimd.c          |  6 ++++++
 hw/display/framebuffer.c              | 12 +++++++-----
 hw/i386/kvmvapic.c                    |  1 +
 hw/misc/vfio.c                        |  2 ++
 hw/virtio/dataplane/hostmem.c         |  7 +++++++
 hw/virtio/vhost.c                     |  2 ++
 hw/virtio/virtio-balloon.c            |  1 +
 hw/xen/xen_pt.c                       |  4 ++++
 include/hw/virtio/dataplane/hostmem.h |  1 +
 kvm-all.c                             |  2 ++
 memory.c                              | 20 ++++++++++++++++++++
 target-arm/kvm.c                      |  2 ++
 target-sparc/mmu_helper.c             |  1 +
 xen-all.c                             |  2 ++
 16 files changed, 64 insertions(+), 6 deletions(-)

Hi!

I do not know how (yet) but this patch breaks qtest on x86 (I bisected it):


make check-qtest V=1
QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$((RANDOM % 255 + 1))} gtester -k
--verbose -m=quick tests/fdc-test tests/ide-test tests/hd-geo-test
tests/rtc-test tests/i440fx-test tests/fw_cfg-test
TEST: tests/fdc-test... (pid=13049)
Broken pipe
FAIL: tests/fdc-test
TEST: tests/ide-test... (pid=13053)
  /x86_64/ide/identify:
Broken pipe
FAIL
GTester: last random seed: R02S2f8a8fd53ff256765db44cefb0a920ce
(pid=13057)
  /x86_64/ide/bmdma/setup:
Broken pipe
FAIL
GTester: last random seed: R02S0cec5d222cfd196e6e839e06d7ddde89
(pid=13061)
  /x86_64/ide/bmdma/simple_rw:                                         FAIL
GTester: last random seed: R02S46a30a1ccd33dc104919118330810a85
(pid=13062)
  /x86_64/ide/bmdma/short_prdt:                                        FAIL
GTester: last random seed: R02S19fdcc95895b870371ed5ddcc8b77eda
(pid=13063)

[...]


On 06/04/2013 10:13 PM, Paolo Bonzini wrote:
> Add ref/unref calls at the following places:
> 
> - places where memory regions are stashed by a listener and
>   used outside the BQL (including in Xen or KVM).
> 
> - memory_region_find callsites
> 
> - creation of aliases and containers (only the aliased/contained
>   region gets a reference to avoid loops)
> 
> - around calls to del_subregion/add_subregion, where the region
>   could disappear after the first call
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                                |  6 +++++-
>  hw/core/loader.c                      |  1 +
>  hw/display/exynos4210_fimd.c          |  6 ++++++
>  hw/display/framebuffer.c              | 12 +++++++-----
>  hw/i386/kvmvapic.c                    |  1 +
>  hw/misc/vfio.c                        |  2 ++
>  hw/virtio/dataplane/hostmem.c         |  7 +++++++
>  hw/virtio/vhost.c                     |  2 ++
>  hw/virtio/virtio-balloon.c            |  1 +
>  hw/xen/xen_pt.c                       |  4 ++++
>  include/hw/virtio/dataplane/hostmem.h |  1 +
>  kvm-all.c                             |  2 ++
>  memory.c                              | 20 ++++++++++++++++++++
>  target-arm/kvm.c                      |  2 ++
>  target-sparc/mmu_helper.c             |  1 +
>  xen-all.c                             |  2 ++
>  16 files changed, 64 insertions(+), 6 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 8909478..ba50e8d 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -815,12 +815,16 @@ static uint16_t phys_section_add(MemoryRegionSection *section)
>                                  phys_sections_nb_alloc);
>      }
>      phys_sections[phys_sections_nb] = *section;
> +    memory_region_ref(section->mr);
>      return phys_sections_nb++;
>  }
>  
>  static void phys_sections_clear(void)
>  {
> -    phys_sections_nb = 0;
> +    while (phys_sections_nb > 0) {
> +        MemoryRegionSection *section = &phys_sections[--phys_sections_nb];
> +        memory_region_unref(section->mr);
> +    }
>  }
>  
>  static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section)
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 3a60cbe..44d8714 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -727,6 +727,7 @@ int rom_load_all(void)
>          addr += rom->romsize;
>          section = memory_region_find(get_system_memory(), rom->addr, 1);
>          rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr);
> +        memory_region_unref(section.mr);
>      }
>      qemu_register_reset(rom_reset, NULL);
>      roms_loaded = 1;
> diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
> index 0da00a9..f44c4a6 100644
> --- a/hw/display/exynos4210_fimd.c
> +++ b/hw/display/exynos4210_fimd.c
> @@ -1126,6 +1126,11 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
>      /* Total number of bytes of virtual screen used by current window */
>      w->fb_len = fb_mapped_len = (w->virtpage_width + w->virtpage_offsize) *
>              (w->rightbot_y - w->lefttop_y + 1);
> +
> +    /* TODO: add .exit and unref the region there.  Not needed yet since sysbus
> +     * does not support hot-unplug.
> +     */
> +    memory_region_unref(w->mem_section.mr);
>      w->mem_section = memory_region_find(sysbus_address_space(&s->busdev),
>              fb_start_addr, w->fb_len);
>      assert(w->mem_section.mr);
> @@ -1154,6 +1159,7 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
>      return;
>  
>  error_return:
> +    memory_region_unref(w->mem_section.mr);
>      w->mem_section.mr = NULL;
>      w->mem_section.size = int128_zero();
>      w->host_fb_addr = NULL;
> diff --git a/hw/display/framebuffer.c b/hw/display/framebuffer.c
> index 49c9e59..4546e42 100644
> --- a/hw/display/framebuffer.c
> +++ b/hw/display/framebuffer.c
> @@ -54,11 +54,11 @@ void framebuffer_update_display(
>      src_len = src_width * rows;
>  
>      mem_section = memory_region_find(address_space, base, src_len);
> +    mem = mem_section.mr;
>      if (int128_get64(mem_section.size) != src_len ||
>              !memory_region_is_ram(mem_section.mr)) {
> -        return;
> +        goto out;
>      }
> -    mem = mem_section.mr;
>      assert(mem);
>      assert(mem_section.offset_within_address_space == base);
>  
> @@ -68,10 +68,10 @@ void framebuffer_update_display(
>         but it's not really worth it as dirty flag tracking will probably
>         already have failed above.  */
>      if (!src_base)
> -        return;
> +        goto out;
>      if (src_len != src_width * rows) {
>          cpu_physical_memory_unmap(src_base, src_len, 0, 0);
> -        return;
> +        goto out;
>      }
>      src = src_base;
>      dest = surface_data(ds);
> @@ -102,10 +102,12 @@ void framebuffer_update_display(
>      }
>      cpu_physical_memory_unmap(src_base, src_len, 0, 0);
>      if (first < 0) {
> -        return;
> +        goto out;
>      }
>      memory_region_reset_dirty(mem, mem_section.offset_within_region, src_len,
>                                DIRTY_MEMORY_VGA);
>      *first_row = first;
>      *last_row = last;
> +out:
> +    memory_region_unref(mem);
>  }
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index 655483b..e375c1c 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -605,6 +605,7 @@ static void vapic_map_rom_writable(VAPICROMState *s)
>                               rom_size);
>      memory_region_add_subregion_overlap(as, rom_paddr, &s->rom, 1000);
>      s->rom_mapped_writable = true;
> +    memory_region_unref(section.mr);
>  }
>  
>  static int vapic_prepare(VAPICROMState *s)
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 52fb036..a1f5803 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -1969,6 +1969,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>      DPRINTF("region_add %"HWADDR_PRIx" - %"HWADDR_PRIx" [%p]\n",
>              iova, end - 1, vaddr);
>  
> +    memory_region_ref(section->mr);
>      ret = vfio_dma_map(container, iova, end - iova, vaddr, section->readonly);
>      if (ret) {
>          error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
> @@ -2010,6 +2011,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
>              iova, end - 1);
>  
>      ret = vfio_dma_unmap(container, iova, end - iova);
> +    memory_region_unref(section->mr);
>      if (ret) {
>          error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
>                       "0x%"HWADDR_PRIx") = %d (%m)",
> diff --git a/hw/virtio/dataplane/hostmem.c b/hw/virtio/dataplane/hostmem.c
> index 7e46723..901d98b 100644
> --- a/hw/virtio/dataplane/hostmem.c
> +++ b/hw/virtio/dataplane/hostmem.c
> @@ -64,8 +64,12 @@ out:
>  static void hostmem_listener_commit(MemoryListener *listener)
>  {
>      HostMem *hostmem = container_of(listener, HostMem, listener);
> +    int i;
>  
>      qemu_mutex_lock(&hostmem->current_regions_lock);
> +    for (i = 0; i < hostmem->num_current_regions; i++) {
> +        memory_region_unref(hostmem->current_regions[i].mr);
> +    }
>      g_free(hostmem->current_regions);
>      hostmem->current_regions = hostmem->new_regions;
>      hostmem->num_current_regions = hostmem->num_new_regions;
> @@ -92,8 +96,11 @@ static void hostmem_append_new_region(HostMem *hostmem,
>          .guest_addr = section->offset_within_address_space,
>          .size = int128_get64(section->size),
>          .readonly = section->readonly,
> +        .mr = section->mr,
>      };
>      hostmem->num_new_regions++;
> +
> +    memory_region_ref(section->mr);
>  }
>  
>  static void hostmem_listener_append_region(MemoryListener *listener,
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index baf84ea..96ab625 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -497,6 +497,7 @@ static void vhost_region_add(MemoryListener *listener,
>      dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
>                                  dev->n_mem_sections);
>      dev->mem_sections[dev->n_mem_sections - 1] = *section;
> +    memory_region_ref(section->mr);
>      vhost_set_memory(listener, section, true);
>  }
>  
> @@ -512,6 +513,7 @@ static void vhost_region_del(MemoryListener *listener,
>      }
>  
>      vhost_set_memory(listener, section, false);
> +    memory_region_unref(section->mr);
>      for (i = 0; i < dev->n_mem_sections; ++i) {
>          if (dev->mem_sections[i].offset_within_address_space
>              == section->offset_within_address_space) {
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a27051c..3fa72a9 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -205,6 +205,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>              addr = section.offset_within_region;
>              balloon_page(memory_region_get_ram_ptr(section.mr) + addr,
>                           !!(vq == s->dvq));
> +            memory_region_unref(section.mr);
>          }
>  
>          virtqueue_push(vq, &elem, offset);
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index c199818..be1fd52 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -606,6 +606,7 @@ static void xen_pt_region_add(MemoryListener *l, MemoryRegionSection *sec)
>      XenPCIPassthroughState *s = container_of(l, XenPCIPassthroughState,
>                                               memory_listener);
>  
> +    memory_region_ref(sec->mr);
>      xen_pt_region_update(s, sec, true);
>  }
>  
> @@ -615,6 +616,7 @@ static void xen_pt_region_del(MemoryListener *l, MemoryRegionSection *sec)
>                                               memory_listener);
>  
>      xen_pt_region_update(s, sec, false);
> +    memory_region_unref(sec->mr);
>  }
>  
>  static void xen_pt_io_region_add(MemoryListener *l, MemoryRegionSection *sec)
> @@ -622,6 +624,7 @@ static void xen_pt_io_region_add(MemoryListener *l, MemoryRegionSection *sec)
>      XenPCIPassthroughState *s = container_of(l, XenPCIPassthroughState,
>                                               io_listener);
>  
> +    memory_region_ref(sec->mr);
>      xen_pt_region_update(s, sec, true);
>  }
>  
> @@ -631,6 +634,7 @@ static void xen_pt_io_region_del(MemoryListener *l, MemoryRegionSection *sec)
>                                               io_listener);
>  
>      xen_pt_region_update(s, sec, false);
> +    memory_region_unref(sec->mr);
>  }
>  
>  static const MemoryListener xen_pt_memory_listener = {
> diff --git a/include/hw/virtio/dataplane/hostmem.h b/include/hw/virtio/dataplane/hostmem.h
> index b2cf093..2810f4b 100644
> --- a/include/hw/virtio/dataplane/hostmem.h
> +++ b/include/hw/virtio/dataplane/hostmem.h
> @@ -18,6 +18,7 @@
>  #include "qemu/thread.h"
>  
>  typedef struct {
> +    MemoryRegion *mr;
>      void *host_addr;
>      hwaddr guest_addr;
>      uint64_t size;
> diff --git a/kvm-all.c b/kvm-all.c
> index e6b262f..91aa7ff 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -788,6 +788,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
>  static void kvm_region_add(MemoryListener *listener,
>                             MemoryRegionSection *section)
>  {
> +    memory_region_ref(section->mr);
>      kvm_set_phys_mem(section, true);
>  }
>  
> @@ -795,6 +796,7 @@ static void kvm_region_del(MemoryListener *listener,
>                             MemoryRegionSection *section)
>  {
>      kvm_set_phys_mem(section, false);
> +    memory_region_unref(section->mr);
>  }
>  
>  static void kvm_log_sync(MemoryListener *listener,
> diff --git a/memory.c b/memory.c
> index a136a77..cfce42b 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -147,6 +147,7 @@ static bool memory_listener_match(MemoryListener *listener,
>          }                                                               \
>      } while (0)
>  
> +/* No need to ref/unref .mr, the FlatRange keeps it alive.  */
>  #define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback)            \
>      MEMORY_LISTENER_CALL(callback, dir, (&(MemoryRegionSection) {       \
>          .mr = (fr)->mr,                                                 \
> @@ -262,11 +263,17 @@ static void flatview_insert(FlatView *view, unsigned pos, FlatRange *range)
>      memmove(view->ranges + pos + 1, view->ranges + pos,
>              (view->nr - pos) * sizeof(FlatRange));
>      view->ranges[pos] = *range;
> +    memory_region_ref(range->mr);
>      ++view->nr;
>  }
>  
>  static void flatview_destroy(FlatView *view)
>  {
> +    int i;
> +
> +    for (i = 0; i < view->nr; i++) {
> +        memory_region_unref(view->ranges[i].mr);
> +    }
>      g_free(view->ranges);
>  }
>  
> @@ -796,6 +803,11 @@ static void memory_region_destructor_ram(MemoryRegion *mr)
>      qemu_ram_free(mr->ram_addr);
>  }
>  
> +static void memory_region_destructor_alias(MemoryRegion *mr)
> +{
> +    memory_region_unref(mr->alias);
> +}
> +
>  static void memory_region_destructor_ram_from_ptr(MemoryRegion *mr)
>  {
>      qemu_ram_free_from_ptr(mr->ram_addr);
> @@ -1043,6 +1055,8 @@ void memory_region_init_alias(MemoryRegion *mr,
>                                uint64_t size)
>  {
>      memory_region_init(mr, name, size);
> +    memory_region_ref(orig);
> +    mr->destructor = memory_region_destructor_alias;
>      mr->alias = orig;
>      mr->alias_offset = offset;
>  }
> @@ -1457,6 +1471,7 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
>      memory_region_transaction_begin();
>  
>      assert(!subregion->parent);
> +    memory_region_ref(subregion);
>      subregion->parent = mr;
>      subregion->addr = offset;
>      QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
> @@ -1519,6 +1534,7 @@ void memory_region_del_subregion(MemoryRegion *mr,
>      assert(subregion->parent == mr);
>      subregion->parent = NULL;
>      QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
> +    memory_region_unref(subregion);
>      memory_region_update_pending |= mr->enabled && subregion->enabled;
>      memory_region_transaction_commit();
>  }
> @@ -1546,12 +1562,14 @@ void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
>      }
>  
>      memory_region_transaction_begin();
> +    memory_region_ref(mr);
>      memory_region_del_subregion(parent, mr);
>      if (may_overlap) {
>          memory_region_add_subregion_overlap(parent, addr, mr, priority);
>      } else {
>          memory_region_add_subregion(parent, addr, mr);
>      }
> +    memory_region_unref(mr);
>      memory_region_transaction_commit();
>  }
>  
> @@ -1629,6 +1647,8 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
>      ret.size = range.size;
>      ret.offset_within_address_space = int128_get64(range.start);
>      ret.readonly = fr->readonly;
> +    memory_region_ref(ret.mr);
> +
>      return ret;
>  }
>  
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index b7bdc03..b9051a4 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -127,6 +127,7 @@ static void kvm_arm_machine_init_done(Notifier *notifier, void *data)
>                  abort();
>              }
>          }
> +        memory_region_unref(kd->mr);
>          g_free(kd);
>      }
>  }
> @@ -152,6 +153,7 @@ void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid)
>      kd->kda.id = devid;
>      kd->kda.addr = -1;
>      QSLIST_INSERT_HEAD(&kvm_devices_head, kd, entries);
> +    memory_region_ref(kd->mr);
>  }
>  
>  typedef struct Reg {
> diff --git a/target-sparc/mmu_helper.c b/target-sparc/mmu_helper.c
> index 3983c96..740cbe8 100644
> --- a/target-sparc/mmu_helper.c
> +++ b/target-sparc/mmu_helper.c
> @@ -845,6 +845,7 @@ hwaddr cpu_get_phys_page_debug(CPUSPARCState *env, target_ulong addr)
>          }
>      }
>      section = memory_region_find(get_system_memory(), phys_addr, 1);
> +    memory_region_unref(section.mr);
>      if (!int128_nz(section.size)) {
>          return -1;
>      }
> diff --git a/xen-all.c b/xen-all.c
> index cd520b1..764741a 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -459,6 +459,7 @@ static void xen_set_memory(struct MemoryListener *listener,
>  static void xen_region_add(MemoryListener *listener,
>                             MemoryRegionSection *section)
>  {
> +    memory_region_ref(section->mr);
>      xen_set_memory(listener, section, true);
>  }
>  
> @@ -466,6 +467,7 @@ static void xen_region_del(MemoryListener *listener,
>                             MemoryRegionSection *section)
>  {
>      xen_set_memory(listener, section, false);
> +    memory_region_unref(section->mr);
>  }
>  
>  static void xen_sync_dirty_bitmap(XenIOState *state,
>

Fails on qtest_init() in tests/libqtest.c, "Broken pipe". I cannot easily
see what is wrong here with this patch but it is 100% reproducible on x86_64
 :(


On 06/13/2013 04:28 PM, Alexey Kardashevskiy wrote:
> Hi!
> 
> I do not know how (yet) but this patch breaks qtest on x86 (I bisected it):
> 
> 
> make check-qtest V=1
> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$((RANDOM % 255 + 1))} gtester -k
> --verbose -m=quick tests/fdc-test tests/ide-test tests/hd-geo-test
> tests/rtc-test tests/i440fx-test tests/fw_cfg-test
> TEST: tests/fdc-test... (pid=13049)
> Broken pipe
> FAIL: tests/fdc-test
> TEST: tests/ide-test... (pid=13053)
>   /x86_64/ide/identify:
> Broken pipe
> FAIL
> GTester: last random seed: R02S2f8a8fd53ff256765db44cefb0a920ce
> (pid=13057)
>   /x86_64/ide/bmdma/setup:
> Broken pipe
> FAIL
> GTester: last random seed: R02S0cec5d222cfd196e6e839e06d7ddde89
> (pid=13061)
>   /x86_64/ide/bmdma/simple_rw:                                         FAIL
> GTester: last random seed: R02S46a30a1ccd33dc104919118330810a85
> (pid=13062)
>   /x86_64/ide/bmdma/short_prdt:                                        FAIL
> GTester: last random seed: R02S19fdcc95895b870371ed5ddcc8b77eda
> (pid=13063)
> 
> [...]
> 
> 
> On 06/04/2013 10:13 PM, Paolo Bonzini wrote:
>> Add ref/unref calls at the following places:
>>
>> - places where memory regions are stashed by a listener and
>>   used outside the BQL (including in Xen or KVM).
>>
>> - memory_region_find callsites
>>
>> - creation of aliases and containers (only the aliased/contained
>>   region gets a reference to avoid loops)
>>
>> - around calls to del_subregion/add_subregion, where the region
>>   could disappear after the first call
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  exec.c                                |  6 +++++-
>>  hw/core/loader.c                      |  1 +
>>  hw/display/exynos4210_fimd.c          |  6 ++++++
>>  hw/display/framebuffer.c              | 12 +++++++-----
>>  hw/i386/kvmvapic.c                    |  1 +
>>  hw/misc/vfio.c                        |  2 ++
>>  hw/virtio/dataplane/hostmem.c         |  7 +++++++
>>  hw/virtio/vhost.c                     |  2 ++
>>  hw/virtio/virtio-balloon.c            |  1 +
>>  hw/xen/xen_pt.c                       |  4 ++++
>>  include/hw/virtio/dataplane/hostmem.h |  1 +
>>  kvm-all.c                             |  2 ++
>>  memory.c                              | 20 ++++++++++++++++++++
>>  target-arm/kvm.c                      |  2 ++
>>  target-sparc/mmu_helper.c             |  1 +
>>  xen-all.c                             |  2 ++
>>  16 files changed, 64 insertions(+), 6 deletions(-)

Hi.

Ok. Back to the bug with this patch. The initial problem with this patch is
that "make check" fails.

Please help with subpages.

It turned out that tests use MALLOC_PERTURB_ which is normally off. Who
does not know - this is a way to tell glibc to fill released memory with
some value and then debug accesses to released memory. Some bright mind
made it random what confuses a lot (and btw valgrind found nothing :-/ ).
So I spend some time before figured out how to reproduce it outside of the
qtest thingy.

The tree is qemu.org/master "bd5c51e Michael Roth qemu-char: don't issue
CHR_EVENT_OPEN in a BH" + replayed patches till the one from $subj on top
of it. QEMU is configured as "configure --target-list=x86_64-softmmu".

The magic is:

export MALLOC_PERTURB_=123
nc -l -U ~/qtest-16318.sock &
nc -l -U ~/qtest-16318.qmp &
x86_64-softmmu/qemu-system-x86_64  \
	-qtest unix:/home/alexey/qtest-16318.sock,nowait \
	-qtest-log /dev/null \
	-qmp unix:/home/alexey/qtest-16318.qmp,nowait \
	-pidfile ~/qtest-16318.pid -machine accel=qtest -vnc none

Immediate crash at (the very last backtrace in this mail is that crash).

x86_cpu_apic_realize() creates a subpage for IO:


#0  aik_dbg_start (f=f@entry=0x5555558c4b41 "subpage_init",
l=l@entry=0x6a0, mr=mr@entry=0x555556556d30)
    at /home/alexey/pcipassthru/qemu-impreza/hw/i386/pc.c:1297
#1  0x0000555555774299 in subpage_init (base=0x0, as=0x5555564a9260) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:1696
#2  register_subpage (d=d@entry=0x555556523d00,
section=section@entry=0x7fffffffd620)
    at /home/alexey/pcipassthru/qemu-impreza/exec.c:845
#3  0x000055555577447d in mem_add (listener=0x555556523d08,
section=<optimized out>)
    at /home/alexey/pcipassthru/qemu-impreza/exec.c:881
#4  0x00005555557c2d69 in address_space_update_topology_pass
(as=as@entry=0x5555564a9260, adding=adding@entry=0x1, old_view=...,
    new_view=...) at /home/alexey/pcipassthru/qemu-impreza/memory.c:751
#5  0x00005555557c64b8 in address_space_update_topology (as=0x5555564a9260)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:766
#6  memory_region_transaction_commit () at
/home/alexey/pcipassthru/qemu-impreza/memory.c:790
#7  0x00005555557c79cd in memory_region_add_subregion_common
(mr=0x555556523c30, offset=offset@entry=0x7e,
    subregion=subregion@entry=0x555556550a28) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:1518
#8  0x00005555557c7ae8 in memory_region_add_subregion (mr=<optimized out>,
offset=offset@entry=0x7e,
    subregion=subregion@entry=0x555556550a28) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:1527
#9  0x0000555555663995 in sysbus_add_io (dev=dev@entry=0x55555654e700,
addr=addr@entry=0x7e, mem=mem@entry=0x555556550a28)
    at /home/alexey/pcipassthru/qemu-impreza/hw/core/sysbus.c:242
#10 0x000055555579cfce in vapic_init (dev=0x55555654e700) at
/home/alexey/pcipassthru/qemu-impreza/hw/i386/kvmvapic.c:707
#11 0x0000555555661651 in device_realize (dev=0x55555654e700,
err=0x7fffffffda40)
    at /home/alexey/pcipassthru/qemu-impreza/hw/core/qdev.c:178
#12 0x0000555555662cf3 in device_set_realized (obj=0x55555654e700,
value=0x1, err=0x7fffffffdb50)
    at /home/alexey/pcipassthru/qemu-impreza/hw/core/qdev.c:699
#13 0x000055555573358e in property_set_bool (obj=0x55555654e700,
v=<optimized out>, opaque=0x55555653c1f0, name=<optimized out>,
    errp=0x7fffffffdb50) at
/home/alexey/pcipassthru/qemu-impreza/qom/object.c:1301
#14 0x0000555555736445 in object_property_set_qobject (obj=0x55555654e700,
value=<optimized out>, name=0x555555896553 "realized",
    errp=0x7fffffffdb50) at
/home/alexey/pcipassthru/qemu-impreza/qom/qom-qobject.c:24
#15 0x000055555573525e in object_property_set_bool
(obj=obj@entry=0x55555654e700, value=value@entry=0x1,
    name=name@entry=0x555555896553 "realized", errp=errp@entry=0x7fffffffdb50)
    at /home/alexey/pcipassthru/qemu-impreza/qom/object.c:852
#16 0x0000555555661c3a in qdev_init (dev=dev@entry=0x55555654e700) at
/home/alexey/pcipassthru/qemu-impreza/hw/core/qdev.c:163
#17 0x0000555555661e91 in qdev_init_nofail (dev=dev@entry=0x55555654e700)
    at /home/alexey/pcipassthru/qemu-impreza/hw/core/qdev.c:277
#18 0x0000555555663789 in sysbus_create_varargs
(name=name@entry=0x5555558c73a1 "kvmvapic", addr=addr@entry=0xffffffffffffffff)
    at /home/alexey/pcipassthru/qemu-impreza/hw/core/sysbus.c:157
#19 0x00005555557a4ead in sysbus_create_simple (irq=0x0,
addr=0xffffffffffffffff, name=0x5555558c73a1 "kvmvapic")
    at /home/alexey/pcipassthru/qemu-impreza/include/hw/sysbus.h:75
#20 apic_init_common (dev=0x555556535350) at
/home/alexey/pcipassthru/qemu-impreza/hw/intc/apic_common.c:311
#21 0x0000555555790fb6 in icc_device_realize (dev=0x555556535350,
errp=0x7fffffffdc80)
    at /home/alexey/pcipassthru/qemu-impreza/hw/cpu/icc_bus.c:50
#22 0x0000555555662cf3 in device_set_realized (obj=0x555556535350,
value=0x1, err=0x7fffffffdd90)
    at /home/alexey/pcipassthru/qemu-impreza/hw/core/qdev.c:699
#23 0x000055555573358e in property_set_bool (obj=0x555556535350,
v=<optimized out>, opaque=0x555556535610, name=<optimized out>,
    errp=0x7fffffffdd90) at
/home/alexey/pcipassthru/qemu-impreza/qom/object.c:1301
#24 0x0000555555736445 in object_property_set_qobject (obj=0x555556535350,
value=<optimized out>, name=0x555555896553 "realized",
    errp=0x7fffffffdd90) at
/home/alexey/pcipassthru/qemu-impreza/qom/qom-qobject.c:24
#25 0x000055555573525e in object_property_set_bool
(obj=obj@entry=0x555556535350, value=value@entry=0x1,
    name=name@entry=0x555555896553 "realized", errp=errp@entry=0x7fffffffdd90)
    at /home/alexey/pcipassthru/qemu-impreza/qom/object.c:852
#26 0x0000555555661c3a in qdev_init (dev=0x555556535350) at
/home/alexey/pcipassthru/qemu-impreza/hw/core/qdev.c:163
#27 0x00005555557d9a7c in x86_cpu_apic_realize (errp=0x7fffffffddd0,
cpu=0x55555653df50)
    at /home/alexey/pcipassthru/qemu-impreza/target-i386/cpu.c:2327
#28 x86_cpu_realizefn (dev=0x55555653df50, errp=0x7fffffffde20) at
/home/alexey/pcipassthru/qemu-impreza/target-i386/cpu.c:2397
#29 0x0000555555662cf3 in device_set_realized (obj=0x55555653df50,
value=0x1, err=0x7fffffffdf30)
    at /home/alexey/pcipassthru/qemu-impreza/hw/core/qdev.c:699
#30 0x000055555573358e in property_set_bool (obj=0x55555653df50,
v=<optimized out>, opaque=0x55555652e390, name=<optimized out>,
    errp=0x7fffffffdf30) at
/home/alexey/pcipassthru/qemu-impreza/qom/object.c:1301
---Type <return> to continue, or q <return> to quit---
#31 0x0000555555736445 in object_property_set_qobject (obj=0x55555653df50,
value=<optimized out>, name=0x555555896553 "realized",
    errp=0x7fffffffdf30) at
/home/alexey/pcipassthru/qemu-impreza/qom/qom-qobject.c:24
#32 0x000055555573525e in object_property_set_bool (obj=0x55555653df50,
value=<optimized out>, name=0x555555896553 "realized",
    errp=0x7fffffffdf30) at
/home/alexey/pcipassthru/qemu-impreza/qom/object.c:852
#33 0x000055555579f3b0 in pc_new_cpu (cpu_model=<optimized out>,
apic_id=0x0, icc_bridge=<optimized out>, errp=0x7fffffffdf70)
    at /home/alexey/pcipassthru/qemu-impreza/hw/i386/pc.c:911
#34 0x00005555557a0fc1 in pc_cpus_init (cpu_model=0x5555558c7a2d "qemu64",
cpu_model@entry=0x0,
    icc_bridge=icc_bridge@entry=0x55555652b420) at
/home/alexey/pcipassthru/qemu-impreza/hw/i386/pc.c:964
#35 0x00005555557a129f in pc_init1 (system_memory=0x555556522e60,
system_io=0x555556523c30, ram_size=ram_size@entry=0x8000000,
    boot_device=boot_device@entry=0x555555891aaa "cad",
kernel_filename=kernel_filename@entry=0x0,
    kernel_cmdline=kernel_cmdline@entry=0x5555558d8fb6 "",
initrd_filename=initrd_filename@entry=0x0,
    cpu_model=cpu_model@entry=0x0, pci_enabled=pci_enabled@entry=0x1,
kvmclock_enabled=kvmclock_enabled@entry=0x1)
    at /home/alexey/pcipassthru/qemu-impreza/hw/i386/pc_piix.c:98
#36 0x00005555557a1aea in pc_init_pci (args=<optimized out>) at
/home/alexey/pcipassthru/qemu-impreza/hw/i386/pc_piix.c:242
#37 0x00005555555dcea0 in main (argc=<optimized out>, argv=<optimized out>,
envp=<optimized out>)
    at /home/alexey/pcipassthru/qemu-impreza/vl.c:4307




This subpage is released later due to some magic which I do not understand:


(gdb) bt
#0  aik_dbg (f=f@entry=0x5555558c4c20 "destroy_page_desc", l=l@entry=0x305)
    at /home/alexey/pcipassthru/qemu-impreza/hw/i386/pc.c:1284
#1  0x0000555555773d48 in destroy_page_desc (section_index=<optimized out>)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:773
#2  destroy_l2_mapping (level=0x0, lp=0x555556559e10) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:791
#3  destroy_l2_mapping (lp=0x555556559e10, level=0x0) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:777
#4  0x0000555555773c88 in destroy_l2_mapping (level=0x1, lp=0x555556559610)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:789
#5  destroy_l2_mapping (lp=0x555556559610, level=0x1) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:777
#6  0x0000555555773c88 in destroy_l2_mapping (level=0x2, lp=0x555556558e10)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:789
#7  destroy_l2_mapping (lp=0x555556558e10, level=0x2) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:777
#8  0x0000555555773c88 in destroy_l2_mapping (level=0x3, lp=0x555556523d00)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:789
#9  destroy_l2_mapping (lp=0x555556523d00, level=0x3) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:777
#10 0x0000555555773df8 in destroy_all_mappings (d=0x555556523d00) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:800
#11 mem_begin (listener=0x555556523d08) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:1732
#12 0x00005555557c6168 in memory_region_transaction_commit () at
/home/alexey/pcipassthru/qemu-impreza/memory.c:787
#13 0x00005555557c79cd in memory_region_add_subregion_common
(mr=mr@entry=0x555556522e60, offset=offset@entry=0xfee00000,
    subregion=subregion@entry=0x55555652d7b8) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:1518
#14 0x00005555557c7a72 in memory_region_add_subregion_overlap
(mr=0x555556522e60, offset=0xfee00000, subregion=0x55555652d7b8,
    priority=<optimized out>) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:1537
#15 0x00005555557a1038 in pc_cpus_init (cpu_model=0x5555558c7a2d "qemu64",
cpu_model@entry=0x0,
    icc_bridge=icc_bridge@entry=0x55555652b420) at
/home/alexey/pcipassthru/qemu-impreza/hw/i386/pc.c:976
#16 0x00005555557a129f in pc_init1 (system_memory=0x555556522e60,
system_io=0x555556523c30, ram_size=ram_size@entry=0x8000000,
    boot_device=boot_device@entry=0x555555891aaa "cad",
kernel_filename=kernel_filename@entry=0x0,
    kernel_cmdline=kernel_cmdline@entry=0x5555558d8fb6 "",
initrd_filename=initrd_filename@entry=0x0,
    cpu_model=cpu_model@entry=0x0, pci_enabled=pci_enabled@entry=0x1,
kvmclock_enabled=kvmclock_enabled@entry=0x1)
    at /home/alexey/pcipassthru/qemu-impreza/hw/i386/pc_piix.c:98
#17 0x00005555557a1aea in pc_init_pci (args=<optimized out>) at
/home/alexey/pcipassthru/qemu-impreza/hw/i386/pc_piix.c:242
#18 0x00005555555dcea0 in main (argc=<optimized out>, argv=<optimized out>,
envp=<optimized out>)
    at /home/alexey/pcipassthru/qemu-impreza/vl.c:4307
(gdb)



And - crash:


#0  object_unref (obj=0xa7a7a7a7a7a7a7a7) at
/home/alexey/pcipassthru/qemu-impreza/qom/object.c:691
#1  0x00005555557c505c in memory_region_unref (mr=<optimized out>) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:1172
#2  0x0000555555775953 in phys_sections_clear () at
/home/alexey/pcipassthru/qemu-impreza/exec.c:826
#3  0x0000555555775999 in core_begin (listener=<optimized out>) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:1738
#4  0x00005555557c6168 in memory_region_transaction_commit () at
/home/alexey/pcipassthru/qemu-impreza/memory.c:787
#5  0x00005555557c79cd in memory_region_add_subregion_common
(mr=mr@entry=0x555556522e60, offset=offset@entry=0xfee00000,
    subregion=subregion@entry=0x55555652d7b8) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:1518
#6  0x00005555557c7a72 in memory_region_add_subregion_overlap
(mr=0x555556522e60, offset=0xfee00000, subregion=0x55555652d7b8,
    priority=<optimized out>) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:1537
#7  0x00005555557a1038 in pc_cpus_init (cpu_model=0x5555558c7a2d "qemu64",
cpu_model@entry=0x0,
    icc_bridge=icc_bridge@entry=0x55555652b420) at
/home/alexey/pcipassthru/qemu-impreza/hw/i386/pc.c:976
#8  0x00005555557a129f in pc_init1 (system_memory=0x555556522e60,
system_io=0x555556523c30, ram_size=ram_size@entry=0x8000000,
    boot_device=boot_device@entry=0x555555891aaa "cad",
kernel_filename=kernel_filename@entry=0x0,
    kernel_cmdline=kernel_cmdline@entry=0x5555558d8fb6 "",
initrd_filename=initrd_filename@entry=0x0,
    cpu_model=cpu_model@entry=0x0, pci_enabled=pci_enabled@entry=0x1,
kvmclock_enabled=kvmclock_enabled@entry=0x1)
    at /home/alexey/pcipassthru/qemu-impreza/hw/i386/pc_piix.c:98
#9  0x00005555557a1aea in pc_init_pci (args=<optimized out>) at
/home/alexey/pcipassthru/qemu-impreza/hw/i386/pc_piix.c:242
#10 0x00005555555dcea0 in main (argc=<optimized out>, argv=<optimized out>,
envp=<optimized out>)
    at /home/alexey/pcipassthru/qemu-impreza/vl.c:4307
(gdb)





On 06/13/2013 07:02 PM, Alexey Kardashevskiy wrote:
> Fails on qtest_init() in tests/libqtest.c, "Broken pipe". I cannot easily
> see what is wrong here with this patch but it is 100% reproducible on x86_64
>  :(
> 
> 
> On 06/13/2013 04:28 PM, Alexey Kardashevskiy wrote:
>> Hi!
>>
>> I do not know how (yet) but this patch breaks qtest on x86 (I bisected it):
>>
>>
>> make check-qtest V=1
>> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
>> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$((RANDOM % 255 + 1))} gtester -k
>> --verbose -m=quick tests/fdc-test tests/ide-test tests/hd-geo-test
>> tests/rtc-test tests/i440fx-test tests/fw_cfg-test
>> TEST: tests/fdc-test... (pid=13049)
>> Broken pipe
>> FAIL: tests/fdc-test
>> TEST: tests/ide-test... (pid=13053)
>>   /x86_64/ide/identify:
>> Broken pipe
>> FAIL
>> GTester: last random seed: R02S2f8a8fd53ff256765db44cefb0a920ce
>> (pid=13057)
>>   /x86_64/ide/bmdma/setup:
>> Broken pipe
>> FAIL
>> GTester: last random seed: R02S0cec5d222cfd196e6e839e06d7ddde89
>> (pid=13061)
>>   /x86_64/ide/bmdma/simple_rw:                                         FAIL
>> GTester: last random seed: R02S46a30a1ccd33dc104919118330810a85
>> (pid=13062)
>>   /x86_64/ide/bmdma/short_prdt:                                        FAIL
>> GTester: last random seed: R02S19fdcc95895b870371ed5ddcc8b77eda
>> (pid=13063)
>>
>> [...]
>>
>>
>> On 06/04/2013 10:13 PM, Paolo Bonzini wrote:
>>> Add ref/unref calls at the following places:
>>>
>>> - places where memory regions are stashed by a listener and
>>>   used outside the BQL (including in Xen or KVM).
>>>
>>> - memory_region_find callsites
>>>
>>> - creation of aliases and containers (only the aliased/contained
>>>   region gets a reference to avoid loops)
>>>
>>> - around calls to del_subregion/add_subregion, where the region
>>>   could disappear after the first call
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  exec.c                                |  6 +++++-
>>>  hw/core/loader.c                      |  1 +
>>>  hw/display/exynos4210_fimd.c          |  6 ++++++
>>>  hw/display/framebuffer.c              | 12 +++++++-----
>>>  hw/i386/kvmvapic.c                    |  1 +
>>>  hw/misc/vfio.c                        |  2 ++
>>>  hw/virtio/dataplane/hostmem.c         |  7 +++++++
>>>  hw/virtio/vhost.c                     |  2 ++
>>>  hw/virtio/virtio-balloon.c            |  1 +
>>>  hw/xen/xen_pt.c                       |  4 ++++
>>>  include/hw/virtio/dataplane/hostmem.h |  1 +
>>>  kvm-all.c                             |  2 ++
>>>  memory.c                              | 20 ++++++++++++++++++++
>>>  target-arm/kvm.c                      |  2 ++
>>>  target-sparc/mmu_helper.c             |  1 +
>>>  xen-all.c                             |  2 ++
>>>  16 files changed, 64 insertions(+), 6 deletions(-)
> 
>

Il 14/06/2013 06:09, Alexey Kardashevskiy ha scritto:
> And - crash:
> 
> 
> #0  object_unref (obj=0xa7a7a7a7a7a7a7a7) at
> /home/alexey/pcipassthru/qemu-impreza/qom/object.c:691

Dangling pointer.  One ref, two unrefs probably.

I'm redoing the series according to Peter's request, it could fix it
automatically.

Paolo

On 06/14/2013 11:56 PM, Paolo Bonzini wrote:
> Il 14/06/2013 06:09, Alexey Kardashevskiy ha scritto:
>> And - crash:
>>
>>
>> #0  object_unref (obj=0xa7a7a7a7a7a7a7a7) at
>> /home/alexey/pcipassthru/qemu-impreza/qom/object.c:691
> 
> Dangling pointer.  One ref, two unrefs probably.

No, subpages (and nested MRs) are just g_free'd, it is not a result of
unreferencing, that's the point.


> I'm redoing the series according to Peter's request, it could fix it
> automatically.


When is it expected to be pushed to github?