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 <pbonz...@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,
>
--
Alexey
