From: Alexey Kardashevskiy <aik@ozlabs.ru>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2 03/17] memory: add ref/unref calls
Date: Thu, 13 Jun 2013 16:28:06 +1000 [thread overview]
Message-ID: <51B96676.9010707@ozlabs.ru> (raw)
In-Reply-To: <1370348041-6768-4-git-send-email-pbonzini@redhat.com>
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,
>
--
Alexey
next prev parent reply other threads:[~2013-06-13 6:28 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-04 12:13 [Qemu-devel] [PATCH v2 00/17] Memory/IOMMU patches part 4: region ownership Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 01/17] memory: add getter/setter for owner Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 02/17] memory: add ref/unref Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 03/17] memory: add ref/unref calls Paolo Bonzini
2013-06-13 6:28 ` Alexey Kardashevskiy [this message]
2013-06-13 9:02 ` Alexey Kardashevskiy
2013-06-14 10:09 ` Alexey Kardashevskiy
2013-06-14 13:56 ` Paolo Bonzini
2013-06-14 15:04 ` Alexey Kardashevskiy
2013-06-25 8:49 ` Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 04/17] exec: add a reference to the region returned by address_space_translate Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 05/17] pci: set owner for BARs Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio Paolo Bonzini
2013-06-04 12:24 ` Peter Maydell
2013-06-04 12:31 ` Paolo Bonzini
2013-06-04 12:36 ` Peter Maydell
2013-06-04 13:24 ` Paolo Bonzini
2013-06-04 14:11 ` Peter Maydell
2013-06-04 14:27 ` Paolo Bonzini
2013-06-04 14:56 ` Peter Maydell
2013-06-04 15:06 ` Paolo Bonzini
2013-06-04 16:50 ` Paolo Bonzini
2013-06-04 17:47 ` Alex Williamson
2013-06-04 19:11 ` Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 07/17] sysbus: set owner for MMIO regions Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 08/17] acpi: add memory_region_set_owner calls Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 09/17] misc: " Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 10/17] isa/portio: allow setting an owner Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 11/17] vga: add memory_region_set_owner calls Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 12/17] pci-assign: " Paolo Bonzini
2013-06-04 16:42 ` Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 13/17] vfio: " Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 14/17] exec: check MRU in qemu_ram_addr_from_host Paolo Bonzini
2013-06-04 12:13 ` [Qemu-devel] [PATCH v2 15/17] exec: move qemu_ram_addr_from_host_nofail to cputlb.c Paolo Bonzini
2013-06-04 12:14 ` [Qemu-devel] [PATCH v2 16/17] memory: return MemoryRegion from qemu_ram_addr_from_host Paolo Bonzini
2013-06-04 12:14 ` [Qemu-devel] [PATCH v2 17/17] memory: ref/unref memory across address_space_map/unmap Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51B96676.9010707@ozlabs.ru \
--to=aik@ozlabs.ru \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.