[Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Paolo Bonzini]

Now that the DMA APIs are unified, we move closer and closer to breaking
memory access from the BQL dependency.  This series adds an API to
reference/unreference memory regions, which is not really needed only
for BQL-less memory access: the big lock can already be dropped between
address_space_map and the corresponding unmap, and the last patch fixes
potential problems in this area if the DMA destination is hot-unplugged.
Lockless memory access only makes things a bit worse.

Reference counting the region piggybacks on reference counting of a QOM
object, the "owner" of the region.  The owner API is designed so that
it will be called as little as possible.  Unowned subregions will get a
region if memory_region_set_owner is called after the subregion is added.
This is in general the common case already; often setting the owner can
be delegated to a bus-specific API that already takes a DeviceState
(for example pci_register_bar or sysbus_init_mmio).

I'll be leaving soon for a few days of vacation, hence I'm not
sending a pull request for the previous part yet.

Paolo

Paolo Bonzini (15):
  memory: add getter/setter for owner
  memory: add ref/unref
  memory: add ref/unref calls
  exec: add a reference to the region returned by address_space_translate
  pci: set owner for BARs
  sysbus: set owner for MMIO regions
  acpi: add memory_region_set_owner calls
  misc: add memory_region_set_owner calls
  isa/portio: allow setting an owner
  vga: add memory_region_set_owner calls
  pci-assign: add memory_region_set_owner calls
  vfio: add memory_region_set_owner calls
  exec: check MRU in qemu_ram_addr_from_host
  memory: return MemoryRegion from qemu_ram_addr_from_host
  memory: ref/unref memory across address_space_map/unmap

 exec.c                                | 79 ++++++++++++++++++++++++++---------
 hw/acpi/ich9.c                        |  1 +
 hw/acpi/piix4.c                       |  5 +++
 hw/char/serial-pci.c                  |  1 +
 hw/core/loader.c                      |  1 +
 hw/core/sysbus.c                      |  2 +
 hw/display/cirrus_vga.c               | 19 ++++++---
 hw/display/exynos4210_fimd.c          |  6 +++
 hw/display/framebuffer.c              | 12 +++---
 hw/display/qxl.c                      |  6 ++-
 hw/display/vga-isa-mm.c               |  2 +-
 hw/display/vga-isa.c                  |  4 +-
 hw/display/vga-pci.c                  |  5 ++-
 hw/display/vga.c                      | 19 ++++++---
 hw/display/vga_int.h                  |  9 ++--
 hw/display/vmware_vga.c               |  4 +-
 hw/i386/kvm/pci-assign.c              | 11 +++++
 hw/i386/kvmvapic.c                    |  1 +
 hw/isa/apm.c                          |  1 +
 hw/isa/isa-bus.c                      |  2 +
 hw/misc/pc-testdev.c                  |  7 ++++
 hw/misc/vfio.c                        | 10 +++++
 hw/pci/pci.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/exec/cpu-common.h             |  2 +-
 include/exec/ioport.h                 |  3 ++
 include/exec/memory.h                 | 51 +++++++++++++++++++++-
 include/hw/virtio/dataplane/hostmem.h |  1 +
 ioport.c                              | 10 +++++
 kvm-all.c                             |  2 +
 memory.c                              | 51 ++++++++++++++++++++++
 target-arm/kvm.c                      |  2 +
 target-i386/kvm.c                     |  4 +-
 target-sparc/mmu_helper.c             |  1 +
 xen-all.c                             |  2 +
 38 files changed, 301 insertions(+), 51 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 01/15] memory: add getter/setter for owner

[Paolo Bonzini]

Whenever memory regions are accessed outside the BQL, they need to be
preserved against hot-unplug.  MemoryRegions actually do not have their
own reference count; they piggyback on a QOM object, their "owner".
Add two functions to retrieve and specify the owner.

The setter function will affect the owner recursively on a whole tree
of contained regions, but without crossing (a) aliases (b) regions that
are already owned by another device.  This is so that a device can create
a complex tree of regions and a single call to memory_region_set_owner
(perhaps even within a bus-specific function, e.g. pci_register_bar) will
affect the entire tree.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h | 18 ++++++++++++++++++
 memory.c              | 21 +++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3598c4f..457a53c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -150,6 +150,7 @@ struct MemoryRegion {
     const MemoryRegionIOMMUOps *iommu_ops;
     void *opaque;
     MemoryRegion *parent;
+    struct Object *owner;
     Int128 size;
     hwaddr addr;
     void (*destructor)(MemoryRegion *mr);
@@ -388,6 +389,23 @@ void memory_region_init_iommu(MemoryRegion *mr,
 void memory_region_destroy(MemoryRegion *mr);
 
 /**
+ * memory_region_owner: get a memory region's owner.
+ *
+ * @mr: the memory region being queried.
+ */
+struct Object *memory_region_owner(MemoryRegion *mr);
+
+/**
+ * memory_region_set_owner: set the owner for a memory region and all
+ * the unowned regions below it.
+ *
+ * @mr: the memory region being set.
+ * @owner: the object that acts as the owner
+ */
+void memory_region_set_owner(MemoryRegion *mr,
+                             struct Object *owner);
+
+/**
  * memory_region_size: get a memory region's size.
  *
  * @mr: the memory region being queried.
diff --git a/memory.c b/memory.c
index 49371ee..e855105 100644
--- a/memory.c
+++ b/memory.c
@@ -826,6 +826,7 @@ void memory_region_init(MemoryRegion *mr,
     mr->opaque = NULL;
     mr->iommu_ops = NULL;
     mr->parent = NULL;
+    mr->owner = NULL;
     mr->size = int128_make64(size);
     if (size == UINT64_MAX) {
         mr->size = int128_2_64();
@@ -1092,6 +1093,26 @@ void memory_region_destroy(MemoryRegion *mr)
     g_free(mr->ioeventfds);
 }
 
+Object *memory_region_owner(MemoryRegion *mr)
+{
+    return mr->owner;
+}
+
+void memory_region_set_owner(MemoryRegion *mr,
+                             Object *owner)
+{
+    MemoryRegion *child;
+
+    assert(mr->owner == NULL || mr->owner == owner);
+    mr->owner = owner;
+
+    QTAILQ_FOREACH(child, &mr->subregions, subregions_link) {
+        if (child->owner == NULL || child->owner == owner) {
+            memory_region_set_owner(child, owner);
+        }
+    }
+}
+
 uint64_t memory_region_size(MemoryRegion *mr)
 {
     if (int128_eq(mr->size, int128_2_64())) {
-- 
1.8.1.4

[Qemu-devel] [PATCH 02/15] memory: add ref/unref

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h | 30 ++++++++++++++++++++++++++++++
 memory.c              | 14 ++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 457a53c..71df1e6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -268,6 +268,36 @@ struct MemoryListener {
 void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size);
+
+/**
+ * memory_region_ref: Add 1 to a memory region's reference count
+ *
+ * Whenever memory regions are accessed outside the BQL, they need to be
+ * preserved against hot-unplug.  MemoryRegions actually do not have their
+ * own reference count; they piggyback on a QOM object, their "owner".
+ * This function adds a reference to the owner.
+ *
+ * All MemoryRegions must have an owner if they can disappear, even if the
+ * device they belong to operates exclusively under the BQL.  This is because
+ * the region could be returned at any time by memory_region_find, and this
+ * is usually under guest control.
+ *
+ * @mr: the #MemoryRegion
+ */
+void memory_region_ref(MemoryRegion *mr);
+
+/**
+ * memory_region_unref: Remove 1 to a memory region's reference count
+ *
+ * Whenever memory regions are accessed outside the BQL, they need to be
+ * preserved against hot-unplug.  MemoryRegions actually do not have their
+ * own reference count; they piggyback on a QOM object, their "owner".
+ * This function removes a reference to the owner and possibly destroys it.
+ *
+ * @mr: the #MemoryRegion
+ */
+void memory_region_unref(MemoryRegion *mr);
+
 /**
  * memory_region_init_io: Initialize an I/O memory region.
  *
diff --git a/memory.c b/memory.c
index e855105..28613d6 100644
--- a/memory.c
+++ b/memory.c
@@ -1113,6 +1113,20 @@ void memory_region_set_owner(MemoryRegion *mr,
     }
 }
 
+void memory_region_ref(MemoryRegion *mr)
+{
+    if (mr && mr->owner) {
+        object_ref(mr->owner);
+    }
+}
+
+void memory_region_unref(MemoryRegion *mr)
+{
+    if (mr && mr->owner) {
+        object_unref(mr->owner);
+    }
+}
+
 uint64_t memory_region_size(MemoryRegion *mr)
 {
     if (int128_eq(mr->size, int128_2_64())) {
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 02/15] memory: add ref/unref

[Peter Maydell]

On 2 June 2013 16:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
> +/**
> + * memory_region_ref: Add 1 to a memory region's reference count
> + *
> + * Whenever memory regions are accessed outside the BQL, they need to be
> + * preserved against hot-unplug.  MemoryRegions actually do not have their
> + * own reference count; they piggyback on a QOM object, their "owner".
> + * This function adds a reference to the owner.

It doesn't make much sense to describe the function as
"add 1 to a memory region's reference count" and then
say that memory regions don't have reference counts...

-- PMM

Re: [Qemu-devel] [PATCH 02/15] memory: add ref/unref

[Paolo Bonzini]

Il 02/06/2013 17:58, Peter Maydell ha scritto:
>> > + * memory_region_ref: Add 1 to a memory region's reference count
>> > + *
>> > + * Whenever memory regions are accessed outside the BQL, they need to be
>> > + * preserved against hot-unplug.  MemoryRegions actually do not have their
>> > + * own reference count; they piggyback on a QOM object, their "owner".
>> > + * This function adds a reference to the owner.
> It doesn't make much sense to describe the function as
> "add 1 to a memory region's reference count" and then
> say that memory regions don't have reference counts...

The fact that the reference count is the owner's is really an
implementation detail.  The reference count is used entirely as if it
were the region's.

So the summary says it is the region's, the long description says what
happens under the hood.

Suggestions on rephrasing the comments are welcome.

Paolo

[Qemu-devel] [PATCH 03/15] memory: add ref/unref calls

[Paolo Bonzini]

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

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                              | 16 ++++++++++++++++
 target-arm/kvm.c                      |  2 ++
 target-sparc/mmu_helper.c             |  1 +
 xen-all.c                             |  2 ++
 16 files changed, 60 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 97a3128..f8d33b2 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -787,6 +787,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);
 }
 
@@ -794,6 +795,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 28613d6..fdf0a0b 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);
 }
 
@@ -799,6 +806,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);
@@ -1046,6 +1058,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;
 }
@@ -1608,6 +1622,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,
-- 
1.8.1.4

[Qemu-devel] [PATCH 04/15] exec: add a reference to the region returned by address_space_translate

[Paolo Bonzini]

Once address_space_translate will be callable out of the big QEMU
lock, the returned MemoryRegion will be unprotected as soon as
address_space_translate terminates. Avoid this by adding a reference to
the region, and dropping it in the caller of address_space_translate.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                | 16 ++++++++++++++--
 include/exec/memory.h |  3 ++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/exec.c b/exec.c
index ba50e8d..bf287cb 100644
--- a/exec.c
+++ b/exec.c
@@ -292,6 +292,7 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
 
     *plen = len;
     *xlat = addr;
+    memory_region_ref(mr);
     return mr;
 }
 
@@ -1994,6 +1995,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                 memcpy(buf, ptr, l);
             }
         }
+        memory_region_unref(mr);
         len -= l;
         buf += l;
         addr += l;
@@ -2159,8 +2161,10 @@ void *address_space_map(AddressSpace *as,
             raddr = memory_region_get_ram_addr(mr) + xlat;
         } else {
             if (memory_region_get_ram_addr(mr) + xlat != raddr + todo) {
+                memory_region_unref(mr);
                 break;
             }
+            memory_region_unref(mr);
         }
 
         len -= l;
@@ -2260,6 +2264,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
             break;
         }
     }
+    memory_region_unref(mr);
     return val;
 }
 
@@ -2319,6 +2324,7 @@ static inline uint64_t ldq_phys_internal(hwaddr addr,
             break;
         }
     }
+    memory_region_unref(mr);
     return val;
 }
 
@@ -2386,6 +2392,7 @@ static inline uint32_t lduw_phys_internal(hwaddr addr,
             break;
         }
     }
+    memory_region_unref(mr);
     return val;
 }
 
@@ -2433,6 +2440,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)
             }
         }
     }
+    memory_region_unref(mr);
 }
 
 /* warning: addr must be aligned */
@@ -2474,6 +2482,7 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,
         }
         invalidate_and_set_dirty(addr1, 4);
     }
+    memory_region_unref(mr);
 }
 
 void stl_phys(hwaddr addr, uint32_t val)
@@ -2537,6 +2546,7 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,
         }
         invalidate_and_set_dirty(addr1, 2);
     }
+    memory_region_unref(mr);
 }
 
 void stw_phys(hwaddr addr, uint32_t val)
@@ -2626,11 +2636,13 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr)
 {
     MemoryRegion*mr;
     hwaddr l = 1;
+    bool res;
 
     mr = address_space_translate(&address_space_memory,
                                  phys_addr, &phys_addr, &l, false);
 
-    return !(memory_region_is_ram(mr) ||
-             memory_region_is_romd(mr));
+    res = !(memory_region_is_ram(mr) || memory_region_is_romd(mr));
+    memory_region_unref(mr);
+    return res;
 }
 #endif
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 71df1e6..d3a2888 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -997,7 +997,8 @@ bool address_space_write(AddressSpace *as, hwaddr addr,
 bool address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int len);
 
 /* address_space_translate: translate an address range into an address space
- * into a MemoryRegion and an address range into that section
+ * into a MemoryRegion and an address range into that section.  Add a reference
+ * to that region.
  *
  * @as: #AddressSpace to be accessed
  * @addr: address within that address space
-- 
1.8.1.4

[Qemu-devel] [PATCH 05/15] pci: set owner for BARs

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index fa30110..2456075 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -914,6 +914,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     uint64_t wmask;
     pcibus_t size = memory_region_size(memory);
 
+    memory_region_set_owner(memory, OBJECT(pci_dev));
+
     assert(region_num >= 0);
     assert(region_num < PCI_NUM_REGIONS);
     if (size & (size-1)) {
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/15] sysbus: set owner for MMIO regions

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/sysbus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 9004d8c..788696b 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -115,6 +115,8 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
     n = dev->num_mmio++;
     dev->mmio[n].addr = -1;
     dev->mmio[n].memory = memory;
+
+    memory_region_set_owner(dev->mmio[n].memory, OBJECT(dev));
 }
 
 MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n)
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/15] acpi: add memory_region_set_owner calls

[Paolo Bonzini]

ACPI regions are added directly to the I/O address space, without
going through BARs.  Thus they need the owner to be set directly.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/acpi/ich9.c  | 1 +
 hw/acpi/piix4.c | 5 +++++
 hw/isa/apm.c    | 1 +
 3 files changed, 7 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 4a17f32..0b19864 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -223,6 +223,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
                           8);
     memory_region_add_subregion(&pm->io, ICH9_PMIO_SMI_EN, &pm->io_smi);
 
+    memory_region_set_owner(&pm->io, OBJECT(lpc_pci));
     pm->irq = sci_irq;
     qemu_register_reset(pm_reset, pm);
     pm->powerdown_notifier.notify = pm_powerdown_req;
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index c4af1cc..d097592 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -433,6 +433,8 @@ static int piix4_pm_initfn(PCIDevice *dev)
     acpi_pm1_cnt_init(&s->ar, &s->io, s->s4_val);
     acpi_gpe_init(&s->ar, GPE_LEN);
 
+    memory_region_set_owner(&s->io, OBJECT(s));
+
     s->powerdown_notifier.notify = piix4_pm_powerdown_req;
     qemu_register_powerdown_notifier(&s->powerdown_notifier);
 
@@ -672,10 +674,12 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
 {
     memory_region_init_io(&s->io_gpe, &piix4_gpe_ops, s, "apci-gpe0",
                           GPE_LEN);
+    memory_region_set_owner(&s->io_gpe, OBJECT(s));
     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
 
     memory_region_init_io(&s->io_pci, &piix4_pci_ops, s, "apci-pci-hotplug",
                           PCI_HOTPLUG_SIZE);
+    memory_region_set_owner(&s->io_pci, OBJECT(s));
     memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
                                 &s->io_pci);
     pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
@@ -683,6 +687,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
     qemu_for_each_cpu(piix4_init_cpu_status, &s->gpe_cpu);
     memory_region_init_io(&s->io_cpu, &cpu_hotplug_ops, s, "apci-cpu-hotplug",
                           PIIX4_PROC_LEN);
+    memory_region_set_owner(&s->io_cpu, OBJECT(s));
     memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu);
     s->cpu_added_notifier.notify = piix4_cpu_added_req;
     qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
diff --git a/hw/isa/apm.c b/hw/isa/apm.c
index 5f21d21..376c564 100644
--- a/hw/isa/apm.c
+++ b/hw/isa/apm.c
@@ -97,6 +97,7 @@ void apm_init(PCIDevice *dev, APMState *apm, apm_ctrl_changed_t callback,
 
     /* ioport 0xb2, 0xb3 */
     memory_region_init_io(&apm->io, &apm_ops, apm, "apm-io", 2);
+    memory_region_set_owner(&apm->io, OBJECT(dev));
     memory_region_add_subregion(pci_address_space_io(dev), APM_CNT_IOPORT,
                                 &apm->io);
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 08/15] misc: add memory_region_set_owner calls

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial-pci.c | 1 +
 hw/misc/pc-testdev.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index 2138e35..6b6106b 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -106,6 +106,7 @@ static int multi_serial_pci_init(PCIDevice *dev)
         s->irq = pci->irqs[i];
         pci->name[i] = g_strdup_printf("uart #%d", i+1);
         memory_region_init_io(&s->io, &serial_io_ops, s, pci->name[i], 8);
+        memory_region_set_owner(&s->io, OBJECT(pci));
         memory_region_add_subregion(&pci->iobar, 8 * i, &s->io);
     }
     return 0;
diff --git a/hw/misc/pc-testdev.c b/hw/misc/pc-testdev.c
index 32df175..77998d6 100644
--- a/hw/misc/pc-testdev.c
+++ b/hw/misc/pc-testdev.c
@@ -150,12 +150,19 @@ static int init_test_device(ISADevice *isa)
 
     memory_region_init_io(&dev->ioport, &test_ioport_ops, dev,
                           "pc-testdev-ioport", 4);
+    memory_region_set_owner(&dev->ioport, OBJECT(dev));
+
     memory_region_init_io(&dev->flush, &test_flush_ops, dev,
                           "pc-testdev-flush-page", 4);
+    memory_region_set_owner(&dev->flush, OBJECT(dev));
+
     memory_region_init_io(&dev->irq, &test_irq_ops, dev,
                           "pc-testdev-irq-line", 24);
+    memory_region_set_owner(&dev->irq, OBJECT(dev));
+
     memory_region_init_io(&dev->iomem, &test_iomem_ops, dev,
                           "pc-testdev-iomem", IOMEM_LEN);
+    memory_region_set_owner(&dev->iomem, OBJECT(dev));
 
     memory_region_add_subregion(io,  0xe0,       &dev->ioport);
     memory_region_add_subregion(io,  0xe4,       &dev->flush);
-- 
1.8.1.4

[Qemu-devel] [PATCH 09/15] isa/portio: allow setting an owner

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/isa/isa-bus.c      |  2 ++
 include/exec/ioport.h |  3 +++
 ioport.c              | 10 ++++++++++
 3 files changed, 15 insertions(+)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 7860b17..d263d0f 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -100,6 +100,7 @@ static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 
 void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
 {
+    memory_region_set_owner(io, OBJECT(dev));
     memory_region_add_subregion(isabus->address_space_io, start, io);
     isa_init_ioport(dev, start);
 }
@@ -116,6 +117,7 @@ void isa_register_portio_list(ISADevice *dev, uint16_t start,
     isa_init_ioport(dev, start);
 
     portio_list_init(piolist, pio_start, opaque, name);
+    portio_list_set_owner(piolist, OBJECT(dev));
     portio_list_add(piolist, isabus->address_space_io, start);
 }
 
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index fc28350..5fe0d99 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -62,6 +62,7 @@ typedef struct PortioList {
     unsigned nr;
     struct MemoryRegion **regions;
     struct MemoryRegion **aliases;
+    struct Object *owner;
     void *opaque;
     const char *name;
 } PortioList;
@@ -69,6 +70,8 @@ typedef struct PortioList {
 void portio_list_init(PortioList *piolist,
                       const struct MemoryRegionPortio *callbacks,
                       void *opaque, const char *name);
+void portio_list_set_owner(PortioList *piolist,
+                           struct Object *owner);
 void portio_list_destroy(PortioList *piolist);
 void portio_list_add(PortioList *piolist,
                      struct MemoryRegion *address_space,
diff --git a/ioport.c b/ioport.c
index a0ac2a0..1cccd70 100644
--- a/ioport.c
+++ b/ioport.c
@@ -347,6 +347,12 @@ void portio_list_init(PortioList *piolist,
     piolist->address_space = NULL;
     piolist->opaque = opaque;
     piolist->name = name;
+    piolist->owner = NULL;
+}
+
+void portio_list_set_owner(PortioList *piolist, Object *owner)
+{
+    piolist->owner = owner;
 }
 
 void portio_list_destroy(PortioList *piolist)
@@ -386,8 +392,12 @@ static void portio_list_add_1(PortioList *piolist,
      */
     memory_region_init_io(region, ops, piolist->opaque, piolist->name,
                           INT64_MAX);
+    memory_region_set_owner(region, piolist->owner);
+
     memory_region_init_alias(alias, piolist->name,
                              region, start + off_low, off_high - off_low);
+    memory_region_set_owner(alias, piolist->owner);
+
     memory_region_add_subregion(piolist->address_space,
                                 start + off_low, alias);
     piolist->regions[piolist->nr] = region;
-- 
1.8.1.4

[Qemu-devel] [PATCH 10/15] vga: add memory_region_set_owner calls

[Paolo Bonzini]

Cc: Gerd Hoffman <kraxel@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/cirrus_vga.c | 19 ++++++++++++++-----
 hw/display/qxl.c        |  6 ++++--
 hw/display/vga-isa-mm.c |  2 +-
 hw/display/vga-isa.c    |  4 ++--
 hw/display/vga-pci.c    |  5 +++--
 hw/display/vga.c        | 19 ++++++++++++++-----
 hw/display/vga_int.h    |  9 +++++----
 hw/display/vmware_vga.c |  4 ++--
 8 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 64bfe2b..ffc4dd4 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2806,12 +2806,14 @@ static const MemoryRegionOps cirrus_vga_io_ops = {
     },
 };
 
-static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
+static void cirrus_init_common(CirrusVGAState * s, int device_id,
+                               DeviceState *owner,
                                MemoryRegion *system_memory,
                                MemoryRegion *system_io)
 {
     int i;
     static int inited;
+    int is_pci = !!object_dynamic_cast(OBJECT(owner), TYPE_PCI_DEVICE);
 
     if (!inited) {
         inited = 1;
@@ -2843,19 +2845,23 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
     /* Register ioport 0x3b0 - 0x3df */
     memory_region_init_io(&s->cirrus_vga_io, &cirrus_vga_io_ops, s,
                           "cirrus-io", 0x30);
+    memory_region_set_owner(&s->cirrus_vga_io, OBJECT(owner));
     memory_region_add_subregion(system_io, 0x3b0, &s->cirrus_vga_io);
 
     memory_region_init(&s->low_mem_container,
                        "cirrus-lowmem-container",
                        0x20000);
+    memory_region_set_owner(&s->low_mem_container, OBJECT(owner));
 
     memory_region_init_io(&s->low_mem, &cirrus_vga_mem_ops, s,
                           "cirrus-low-memory", 0x20000);
+    memory_region_set_owner(&s->low_mem, OBJECT(owner));
     memory_region_add_subregion(&s->low_mem_container, 0, &s->low_mem);
     for (i = 0; i < 2; ++i) {
         static const char *names[] = { "vga.bank0", "vga.bank1" };
         MemoryRegion *bank = &s->cirrus_bank[i];
         memory_region_init_alias(bank, names[i], &s->vga.vram, 0, 0x8000);
+        memory_region_set_owner(bank, OBJECT(owner));
         memory_region_set_enabled(bank, false);
         memory_region_add_subregion_overlap(&s->low_mem_container, i * 0x8000,
                                             bank, 1);
@@ -2870,6 +2876,7 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
     memory_region_init_io(&s->cirrus_linear_io, &cirrus_linear_io_ops, s,
                           "cirrus-linear-io", s->vga.vram_size_mb
                                               * 1024 * 1024);
+    memory_region_set_owner(&s->cirrus_linear_io, OBJECT(owner));
     memory_region_set_flush_coalesced(&s->cirrus_linear_io);
 
     /* I/O handler for LFB */
@@ -2878,11 +2885,13 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
                           s,
                           "cirrus-bitblt-mmio",
                           0x400000);
+    memory_region_set_owner(&s->cirrus_linear_bitblt_io, OBJECT(owner));
     memory_region_set_flush_coalesced(&s->cirrus_linear_bitblt_io);
 
     /* I/O handler for memory-mapped I/O */
     memory_region_init_io(&s->cirrus_mmio_io, &cirrus_mmio_io_ops, s,
                           "cirrus-mmio", CIRRUS_PNPMMIO_SIZE);
+    memory_region_set_owner(&s->cirrus_mmio_io, OBJECT(owner));
     memory_region_set_flush_coalesced(&s->cirrus_mmio_io);
 
     s->real_vram_size =
@@ -2912,8 +2921,8 @@ static int vga_initfn(ISADevice *dev)
     ISACirrusVGAState *d = ISA_CIRRUS_VGA(dev);
     VGACommonState *s = &d->cirrus_vga.vga;
 
-    vga_common_init(s);
-    cirrus_init_common(&d->cirrus_vga, CIRRUS_ID_CLGD5430, 0,
+    vga_common_init(s, DEVICE(dev));
+    cirrus_init_common(&d->cirrus_vga, CIRRUS_ID_CLGD5430, DEVICE(d),
                        isa_address_space(dev), isa_address_space_io(dev));
     s->con = graphic_console_init(DEVICE(dev), s->hw_ops, s);
     rom_add_vga(VGABIOS_CIRRUS_FILENAME);
@@ -2959,8 +2968,8 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
      int16_t device_id = pc->device_id;
 
      /* setup VGA */
-     vga_common_init(&s->vga);
-     cirrus_init_common(s, device_id, 1, pci_address_space(dev),
+     vga_common_init(&s->vga, DEVICE(dev));
+     cirrus_init_common(s, device_id, DEVICE(dev), pci_address_space(dev),
                         pci_address_space_io(dev));
      s->vga.con = graphic_console_init(DEVICE(dev), s->vga.hw_ops, &s->vga);
 
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c475cb1..3b6cc85 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2066,9 +2066,11 @@ static int qxl_init_primary(PCIDevice *dev)
     qxl->id = 0;
     qxl_init_ramsize(qxl);
     vga->vram_size_mb = qxl->vga.vram_size >> 20;
-    vga_common_init(vga);
-    vga_init(vga, pci_address_space(dev), pci_address_space_io(dev), false);
+    vga_common_init(vga, DEVICE(dev));
+    vga_init(vga, DEVICE(dev), pci_address_space(dev),
+             pci_address_space_io(dev), false);
     portio_list_init(qxl_vga_port_list, qxl_vga_portio_list, vga, "vga");
+    portio_list_set_owner(qxl_vga_port_list, OBJECT(dev));
     portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
 
     vga->con = graphic_console_init(DEVICE(dev), &qxl_ops, qxl);
diff --git a/hw/display/vga-isa-mm.c b/hw/display/vga-isa-mm.c
index ceeb92f..64c6fc3 100644
--- a/hw/display/vga-isa-mm.c
+++ b/hw/display/vga-isa-mm.c
@@ -132,7 +132,7 @@ int isa_vga_mm_init(hwaddr vram_base,
     s = g_malloc0(sizeof(*s));
 
     s->vga.vram_size_mb = VGA_RAM_SIZE >> 20;
-    vga_common_init(&s->vga);
+    vga_common_init(&s->vga, NULL);
     vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
 
     s->vga.con = graphic_console_init(NULL, s->vga.hw_ops, s);
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 9e63b69..f52c104 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -55,9 +55,9 @@ static int vga_initfn(ISADevice *dev)
     MemoryRegion *vga_io_memory;
     const MemoryRegionPortio *vga_ports, *vbe_ports;
 
-    vga_common_init(s);
+    vga_common_init(s, DEVICE(dev));
     s->legacy_address_space = isa_address_space(dev);
-    vga_io_memory = vga_init_io(s, &vga_ports, &vbe_ports);
+    vga_io_memory = vga_init_io(s, DEVICE(dev), &vga_ports, &vbe_ports);
     isa_register_portio_list(dev, 0x3b0, vga_ports, s, "vga");
     if (vbe_ports) {
         isa_register_portio_list(dev, 0x1ce, vbe_ports, s, "vbe");
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index cea8db7..3f860e9 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -147,8 +147,9 @@ static int pci_std_vga_initfn(PCIDevice *dev)
     VGACommonState *s = &d->vga;
 
     /* vga + console init */
-    vga_common_init(s);
-    vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true);
+    vga_common_init(s, DEVICE(dev));
+    vga_init(s, DEVICE(dev), pci_address_space(dev), pci_address_space_io(dev),
+             true);
 
     s->con = graphic_console_init(DEVICE(dev), s->hw_ops, s);
 
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 21a108d..2d7e37a 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -199,6 +199,8 @@ static void vga_update_memory_access(VGACommonState *s)
         base += isa_mem_base;
         region = g_malloc(sizeof(*region));
         memory_region_init_alias(region, "vga.chain4", &s->vram, offset, size);
+        memory_region_set_owner(region, memory_region_owner(&s->vram));
+
         memory_region_add_subregion_overlap(s->legacy_address_space, base,
                                             region, 2);
         s->chain4_alias = region;
@@ -2256,7 +2258,7 @@ static const GraphicHwOps vga_ops = {
     .text_update = vga_update_text,
 };
 
-void vga_common_init(VGACommonState *s)
+void vga_common_init(VGACommonState *s, DeviceState *owner)
 {
     int i, j, v, b;
 
@@ -2293,6 +2295,7 @@ void vga_common_init(VGACommonState *s)
 
     s->is_vbe_vmstate = 1;
     memory_region_init_ram(&s->vram, "vga.vram", s->vram_size);
+    memory_region_set_owner(&s->vram, OBJECT(owner));
     vmstate_register_ram_global(&s->vram);
     xen_register_framebuffer(&s->vram);
     s->vram_ptr = memory_region_get_ram_ptr(&s->vram);
@@ -2333,7 +2336,7 @@ static const MemoryRegionPortio vbe_portio_list[] = {
 };
 
 /* Used by both ISA and PCI */
-MemoryRegion *vga_init_io(VGACommonState *s,
+MemoryRegion *vga_init_io(VGACommonState *s, DeviceState *owner,
                           const MemoryRegionPortio **vga_ports,
                           const MemoryRegionPortio **vbe_ports)
 {
@@ -2345,13 +2348,15 @@ MemoryRegion *vga_init_io(VGACommonState *s,
     vga_mem = g_malloc(sizeof(*vga_mem));
     memory_region_init_io(vga_mem, &vga_mem_ops, s,
                           "vga-lowmem", 0x20000);
+    memory_region_set_owner(vga_mem, OBJECT(owner));
     memory_region_set_flush_coalesced(vga_mem);
 
     return vga_mem;
 }
 
-void vga_init(VGACommonState *s, MemoryRegion *address_space,
-              MemoryRegion *address_space_io, bool init_vga_ports)
+void vga_init(VGACommonState *s, DeviceState *owner,
+              MemoryRegion *address_space, MemoryRegion *address_space_io,
+              bool init_vga_ports)
 {
     MemoryRegion *vga_io_memory;
     const MemoryRegionPortio *vga_ports, *vbe_ports;
@@ -2364,7 +2369,7 @@ void vga_init(VGACommonState *s, MemoryRegion *address_space,
 
     s->legacy_address_space = address_space;
 
-    vga_io_memory = vga_init_io(s, &vga_ports, &vbe_ports);
+    vga_io_memory = vga_init_io(s, owner, &vga_ports, &vbe_ports);
     memory_region_add_subregion_overlap(address_space,
                                         isa_mem_base + 0x000a0000,
                                         vga_io_memory,
@@ -2372,10 +2377,12 @@ void vga_init(VGACommonState *s, MemoryRegion *address_space,
     memory_region_set_coalescing(vga_io_memory);
     if (init_vga_ports) {
         portio_list_init(vga_port_list, vga_ports, s, "vga");
+        portio_list_set_owner(vga_port_list, OBJECT(owner));
         portio_list_add(vga_port_list, address_space_io, 0x3b0);
     }
     if (vbe_ports) {
         portio_list_init(vbe_port_list, vbe_ports, s, "vbe");
+        portio_list_set_owner(vbe_port_list, OBJECT(owner));
         portio_list_add(vbe_port_list, address_space_io, 0x1ce);
     }
 }
@@ -2387,6 +2394,8 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
      */
     memory_region_init_alias(&s->vram_vbe, "vram.vbe",
                              &s->vram, 0, memory_region_size(&s->vram));
+    memory_region_set_owner(&s->vram_vbe, memory_region_owner(&s->vram));
+
     /* XXX: use optimized standard vga accesses */
     memory_region_add_subregion(system_memory,
                                 VBE_DISPI_LFB_PHYSICAL_ADDRESS,
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 66f9f3c..7fe4967 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -177,10 +177,11 @@ static inline int c6_to_8(int v)
     return (v << 2) | (b << 1) | b;
 }
 
-void vga_common_init(VGACommonState *s);
-void vga_init(VGACommonState *s, MemoryRegion *address_space,
-              MemoryRegion *address_space_io, bool init_vga_ports);
-MemoryRegion *vga_init_io(VGACommonState *s,
+void vga_common_init(VGACommonState *s, DeviceState *owner);
+void vga_init(VGACommonState *s, DeviceState *owner,
+              MemoryRegion *address_space, MemoryRegion *address_space_io,
+              bool init_vga_ports);
+MemoryRegion *vga_init_io(VGACommonState *s, DeviceState *owner,
                           const MemoryRegionPortio **vga_ports,
                           const MemoryRegionPortio **vbe_ports);
 void vga_common_reset(VGACommonState *s);
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index fd3569d..1f94b8e 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1198,8 +1198,8 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
     vmstate_register_ram_global(&s->fifo_ram);
     s->fifo_ptr = memory_region_get_ram_ptr(&s->fifo_ram);
 
-    vga_common_init(&s->vga);
-    vga_init(&s->vga, address_space, io, true);
+    vga_common_init(&s->vga, dev);
+    vga_init(&s->vga, dev, address_space, io, true);
     vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
     s->new_depth = 32;
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 11/15] pci-assign: add memory_region_set_owner calls

[Paolo Bonzini]

Cc: Alex Williamson <awilliam@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index ff85590..4b1c2d9 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -300,6 +300,7 @@ static void assigned_dev_iomem_setup(PCIDevice *pci_dev, int region_num,
     if (e_size > 0) {
         memory_region_init(&region->container, "assigned-dev-container",
                            e_size);
+        memory_region_set_owner(&region->container, OBJECT(pci_dev));
         memory_region_add_subregion(&region->container, 0, &region->real_iomem);
 
         /* deal with MSI-X MMIO page */
@@ -330,9 +331,12 @@ static void assigned_dev_ioport_setup(PCIDevice *pci_dev, int region_num,
 
     region->e_size = size;
     memory_region_init(&region->container, "assigned-dev-container", size);
+    memory_region_set_owner(&region->container, OBJECT(pci_dev));
+
     memory_region_init_io(&region->real_iomem, &assigned_dev_ioport_ops,
                           r_dev->v_addrs + region_num,
                           "assigned-dev-iomem", size);
+    memory_region_set_owner(&region->real_iomem, OBJECT(pci_dev));
     memory_region_add_subregion(&region->container, 0, &region->real_iomem);
 }
 
@@ -482,6 +486,8 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
                                       &slow_bar_ops, &pci_dev->v_addrs[i],
                                       "assigned-dev-slow-bar",
                                       cur_region->size);
+                memory_region_set_owner(&pci_dev->v_addrs[i].real_iomem,
+                                        OBJECT(pci_dev));
             } else {
                 void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
                 char name[32];
@@ -490,6 +496,9 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
                 memory_region_init_ram_ptr(&pci_dev->v_addrs[i].real_iomem,
                                            name, cur_region->size,
                                            virtbase);
+                memory_region_set_owner(&pci_dev->v_addrs[i].real_iomem,
+                                        OBJECT(pci_dev));
+
                 vmstate_register_ram(&pci_dev->v_addrs[i].real_iomem,
                                      &pci_dev->dev.qdev);
             }
@@ -1651,6 +1660,7 @@ static int assigned_dev_register_msix_mmio(AssignedDevice *dev)
 
     memory_region_init_io(&dev->mmio, &assigned_dev_msix_mmio_ops, dev,
                           "assigned-dev-msix", MSIX_PAGE_SIZE);
+    memory_region_set_owner(&dev->mmio, OBJECT(dev));
     return 0;
 }
 
@@ -1916,6 +1926,7 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
     snprintf(name, sizeof(name), "%s.rom",
             object_get_typename(OBJECT(dev)));
     memory_region_init_ram(&dev->dev.rom, name, st.st_size);
+    memory_region_set_owner(&dev->dev.rom, OBJECT(dev));
     vmstate_register_ram(&dev->dev.rom, &dev->dev.qdev);
     ptr = memory_region_get_ram_ptr(&dev->dev.rom);
     memset(ptr, 0xff, st.st_size);
-- 
1.8.1.4

[Qemu-devel] [PATCH 12/15] vfio: add memory_region_set_owner calls

[Paolo Bonzini]

Cc: Alex Williamson <awilliam@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/misc/vfio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index a1f5803..3c0dc9f 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -1156,6 +1156,7 @@ static void vfio_vga_probe_ati_3c3_quirk(VFIODevice *vdev)
 
     memory_region_init_io(&quirk->mem, &vfio_ati_3c3_quirk, quirk,
                           "vfio-ati-3c3-quirk", 1);
+    memory_region_set_owner(&quirk->mem, OBJECT(vdev));
     memory_region_add_subregion(&vdev->vga.region[QEMU_PCI_VGA_IO_HI].mem, 3,
                                 &quirk->mem);
 
@@ -1247,6 +1248,7 @@ static void vfio_probe_ati_4010_quirk(VFIODevice *vdev, int nr)
 
     memory_region_init_io(&quirk->mem, &vfio_ati_4010_quirk, quirk,
                           "vfio-ati-4010-quirk", 8);
+    memory_region_set_owner(&quirk->mem, OBJECT(vdev));
     memory_region_add_subregion_overlap(&vdev->bars[nr].mem, 0, &quirk->mem, 1);
 
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
@@ -1333,6 +1335,7 @@ static void vfio_probe_ati_f10_quirk(VFIODevice *vdev, int nr)
 
     memory_region_init_io(&quirk->mem, &vfio_ati_f10_quirk, quirk,
                           "vfio-ati-f10-quirk", 8);
+    memory_region_set_owner(&quirk->mem, OBJECT(vdev));
     memory_region_add_subregion_overlap(&vdev->bars[nr].mem, 0, &quirk->mem, 1);
 
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
@@ -1453,6 +1456,7 @@ static void vfio_vga_probe_nvidia_3d0_quirk(VFIODevice *vdev)
 
     memory_region_init_io(&quirk->mem, &vfio_nvidia_3d0_quirk, quirk,
                           "vfio-nvidia-3d0-quirk", 6);
+    memory_region_set_owner(&quirk->mem, OBJECT(vdev));
     memory_region_add_subregion(&vdev->vga.region[QEMU_PCI_VGA_IO_HI].mem,
                                 0x10, &quirk->mem);
 
@@ -1568,6 +1572,7 @@ static void vfio_probe_nvidia_bar5_window_quirk(VFIODevice *vdev, int nr)
 
     memory_region_init_io(&quirk->mem, &vfio_nvidia_bar5_window_quirk, quirk,
                           "vfio-nvidia-bar5-window-quirk", 16);
+    memory_region_set_owner(&quirk->mem, OBJECT(vdev));
     memory_region_add_subregion_overlap(&vdev->bars[nr].mem, 0, &quirk->mem, 1);
 
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
@@ -1647,6 +1652,7 @@ static void vfio_probe_nvidia_bar0_88000_quirk(VFIODevice *vdev, int nr)
     memory_region_init_io(&quirk->mem, &vfio_nvidia_bar0_88000_quirk, quirk,
                           "vfio-nvidia-bar0-88000-quirk",
                           TARGET_PAGE_ALIGN(PCIE_CONFIG_SPACE_SIZE));
+    memory_region_set_owner(&quirk->mem, OBJECT(vdev));
     memory_region_add_subregion_overlap(&vdev->bars[nr].mem,
                                         0x88000 & TARGET_PAGE_MASK,
                                         &quirk->mem, 1);
@@ -1726,6 +1732,7 @@ static void vfio_probe_nvidia_bar0_1800_quirk(VFIODevice *vdev, int nr)
     memory_region_init_io(&quirk->mem, &vfio_nvidia_bar0_1800_quirk, quirk,
                           "vfio-nvidia-bar0-1800-quirk",
                           TARGET_PAGE_ALIGN(PCI_CONFIG_SPACE_SIZE));
+    memory_region_set_owner(&quirk->mem, OBJECT(vdev));
     memory_region_add_subregion_overlap(&vdev->bars[nr].mem,
                                         0x1800 & TARGET_PAGE_MASK,
                                         &quirk->mem, 1);
@@ -2237,6 +2244,7 @@ empty_region:
         memory_region_init(submem, name, 0);
     }
 
+    memory_region_set_owner(submem, memory_region_owner(mem));
     memory_region_add_subregion(mem, offset, submem);
 
     return ret;
-- 
1.8.1.4

[Qemu-devel] [PATCH 13/15] exec: check MRU in qemu_ram_addr_from_host

[Paolo Bonzini]

This function is not used outside the iothread mutex, so it
can use ram_list.mru_block.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index bf287cb..9fd4c90 100644
--- a/exec.c
+++ b/exec.c
@@ -1441,18 +1441,26 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
         return 0;
     }
 
+    block = ram_list.mru_block;
+    if (block && block->host && host - block->host < block->length) {
+        goto found;
+    }
+
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
         /* This case append when the block is not mapped. */
         if (block->host == NULL) {
             continue;
         }
         if (host - block->host < block->length) {
-            *ram_addr = block->offset + (host - block->host);
-            return 0;
+            goto found;
         }
     }
 
     return -1;
+
+found:
+    *ram_addr = block->offset + (host - block->host);
+    return 0;
 }
 
 /* Some of the softmmu routines need to translate from a host pointer
-- 
1.8.1.4

[Qemu-devel] [PATCH 14/15] memory: return MemoryRegion from qemu_ram_addr_from_host

[Paolo Bonzini]

It will be needed in the next patch.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                    | 35 +++++++++++++++++++++--------------
 include/exec/cpu-common.h |  2 +-
 target-i386/kvm.c         |  4 ++--
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/exec.c b/exec.c
index 9fd4c90..39324ba 100644
--- a/exec.c
+++ b/exec.c
@@ -1329,15 +1329,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
 }
 #endif /* !_WIN32 */
 
-/* Return a host pointer to ram allocated with qemu_ram_alloc.
-   With the exception of the softmmu code in this file, this should
-   only be used for local memory (e.g. video ram) that the device owns,
-   and knows it isn't going to access beyond the end of the block.
-
-   It should not be used for general purpose DMA.
-   Use cpu_physical_memory_map/cpu_physical_memory_rw instead.
- */
-void *qemu_get_ram_ptr(ram_addr_t addr)
+static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
 {
     RAMBlock *block;
 
@@ -1357,6 +1349,21 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
 
 found:
     ram_list.mru_block = block;
+    return block;
+}
+
+/* Return a host pointer to ram allocated with qemu_ram_alloc.
+ * With the exception of the softmmu code in this file, this should
+ * only be used for local memory (e.g. video ram) that the device owns,
+ * and knows it isn't going to access beyond the end of the block.
+ *
+ * It should not be used for general purpose DMA.
+ * Use cpu_physical_memory_map/cpu_physical_memory_rw instead.
+ */
+void *qemu_get_ram_ptr(ram_addr_t addr)
+{
+    RAMBlock *block = qemu_get_ram_block(addr);
+
     if (xen_enabled()) {
         /* We need to check if the requested address is in the RAM
          * because we don't want to map the entire memory in QEMU.
@@ -1431,14 +1438,14 @@ static void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
     }
 }
 
-int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
+MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
 {
     RAMBlock *block;
     uint8_t *host = ptr;
 
     if (xen_enabled()) {
         *ram_addr = xen_ram_addr_from_mapcache(ptr);
-        return 0;
+        return qemu_get_ram_block(*ram_addr)->mr;
     }
 
     block = ram_list.mru_block;
@@ -1456,11 +1463,11 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
         }
     }
 
-    return -1;
+    return NULL;
 
 found:
     *ram_addr = block->offset + (host - block->host);
-    return 0;
+    return block->mr;
 }
 
 /* Some of the softmmu routines need to translate from a host pointer
@@ -1469,7 +1476,7 @@ ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
 {
     ram_addr_t ram_addr;
 
-    if (qemu_ram_addr_from_host(ptr, &ram_addr)) {
+    if (qemu_ram_addr_from_host(ptr, &ram_addr) == NULL) {
         fprintf(stderr, "Bad ram pointer %p\n", ptr);
         abort();
     }
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index e061e21..2db580a 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -50,7 +50,7 @@ typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
 
 void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
 /* This should not be used by devices.  */
-int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
+MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
 ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr);
 void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev);
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 9ffb6ca..7ba98cd 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -318,7 +318,7 @@ int kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
 
     if ((env->mcg_cap & MCG_SER_P) && addr
         && (code == BUS_MCEERR_AR || code == BUS_MCEERR_AO)) {
-        if (qemu_ram_addr_from_host(addr, &ram_addr) ||
+        if (qemu_ram_addr_from_host(addr, &ram_addr) == NULL ||
             !kvm_physical_memory_addr_from_host(c->kvm_state, addr, &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "
                     "QEMU itself instead of guest system!\n");
@@ -350,7 +350,7 @@ int kvm_arch_on_sigbus(int code, void *addr)
         hwaddr paddr;
 
         /* Hope we are lucky for AO MCE */
-        if (qemu_ram_addr_from_host(addr, &ram_addr) ||
+        if (qemu_ram_addr_from_host(addr, &ram_addr) == NULL ||
             !kvm_physical_memory_addr_from_host(CPU(first_cpu)->kvm_state,
                                                 addr, &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 14/15] memory: return MemoryRegion from qemu_ram_addr_from_host

[Peter Maydell]

On 2 June 2013 16:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
> -int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
> +MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
>  ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr);

This is weird, because now the _nofail and the standard
versions of this function return different things. Why
wouldn't a caller of the _nofail version potentially
need the MemoryRegion* too?

-- PMM

Re: [Qemu-devel] [PATCH 14/15] memory: return MemoryRegion from qemu_ram_addr_from_host

[Paolo Bonzini]

Il 02/06/2013 18:04, Peter Maydell ha scritto:
> On 2 June 2013 16:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> -int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
>> +MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
>>  ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr);
> 
> This is weird, because now the _nofail and the standard
> versions of this function return different things. Why
> wouldn't a caller of the _nofail version potentially
> need the MemoryRegion* too?

I'll adjust both functions.

Paolo

Re: [Qemu-devel] [PATCH 14/15] memory: return MemoryRegion from qemu_ram_addr_from_host

[Paolo Bonzini]

Il 03/06/2013 08:40, Paolo Bonzini ha scritto:
> Il 02/06/2013 18:04, Peter Maydell ha scritto:
>> On 2 June 2013 16:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> -int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
>>> +MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
>>>  ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr);
>>
>> This is weird, because now the _nofail and the standard
>> versions of this function return different things.

Ah, actually that's not a change.  The function used to return 0/-1 and
place the address in a by-reference argument.  It's a somewhat weird
decision made when the normal and _nofail version were split (commit
e890261, Export qemu_ram_addr_from_host, 2010-10-11).  Returning -1 to
indicate failure would have been ok, since it's not a valid ram_addr_t.

>> Why wouldn't a caller of the _nofail version potentially
>> need the MemoryRegion* too?

Because there are just a handful of calls, and all of them are in
cputlb.c which is not very much MemoryRegion-aware.  I'll just move it
there and make it static.

The right fix here would be to make all the MCE handling code not
KVM-specific.  Then it can be in exec.c and ram_addr_t can be private
(eliminating a good deal of confusion between it and hwaddr).

Paolo

[Qemu-devel] [PATCH 15/15] memory: ref/unref memory across address_space_map/unmap

[Paolo Bonzini]

The iothread mutex might be released between map and unmap, so the
mapped region might disappear.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 39324ba..538aa9e 100644
--- a/exec.c
+++ b/exec.c
@@ -2068,6 +2068,7 @@ void cpu_physical_memory_write_rom(hwaddr addr,
 }
 
 typedef struct {
+    MemoryRegion *mr;
     void *buffer;
     hwaddr addr;
     hwaddr len;
@@ -2165,15 +2166,18 @@ void *address_space_map(AddressSpace *as,
             bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, TARGET_PAGE_SIZE);
             bounce.addr = addr;
             bounce.len = l;
+            bounce.mr = mr;
             if (!is_write) {
                 address_space_read(as, addr, bounce.buffer, l);
             }
 
             *plen = l;
+            memory_region_ref(mr);
             return bounce.buffer;
         }
         if (!todo) {
             raddr = memory_region_get_ram_addr(mr) + xlat;
+            memory_region_ref(mr);
         } else {
             if (memory_region_get_ram_addr(mr) + xlat != raddr + todo) {
                 memory_region_unref(mr);
@@ -2200,8 +2204,12 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                          int is_write, hwaddr access_len)
 {
     if (buffer != bounce.buffer) {
+        MemoryRegion *mr;
+        ram_addr_t addr1;
+
+        mr = qemu_ram_addr_from_host(buffer, &addr1);
+        assert(mr);
         if (is_write) {
-            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
             while (access_len) {
                 unsigned l;
                 l = TARGET_PAGE_SIZE;
@@ -2215,6 +2223,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
         if (xen_enabled()) {
             xen_invalidate_map_cache_entry(buffer);
         }
+        memory_region_unref(mr);
         return;
     }
     if (is_write) {
@@ -2222,6 +2231,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
     }
     qemu_vfree(bounce.buffer);
     bounce.buffer = NULL;
+    memory_region_unref(bounce.mr);
     cpu_notify_map_clients();
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Peter Maydell]

On 2 June 2013 16:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Reference counting the region piggybacks on reference counting of a QOM
> object, the "owner" of the region.  The owner API is designed so that
> it will be called as little as possible.  Unowned subregions will get a
> region if memory_region_set_owner is called after the subregion is added.
> This is in general the common case already; often setting the owner can
> be delegated to a bus-specific API that already takes a DeviceState
> (for example pci_register_bar or sysbus_init_mmio).

This feels a bit fragile to me -- there doesn't seem to be
a clear rule for who has to set the owner of a region or
when they have to do it, or for ensuring that it doesn't
get forgotten altogether.

What happens if I take a MemoryRegion* that another device
has exposed to me as a sysbus mmio region (and so claimed
ownership of) and pass it to pci_register_bar()? Who owns
it at that point? [That's a legitimate thing to do, I think,
though I don't suppose anybody does it at the moment.
Sysbus MMIOs aren't only for mapping in the system address
space, they're a general way for one device to expose a
MemoryRegion * for use by another device.]

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Paolo Bonzini]

Il 02/06/2013 18:12, Peter Maydell ha scritto:
> On 2 June 2013 16:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Reference counting the region piggybacks on reference counting of a QOM
>> object, the "owner" of the region.  The owner API is designed so that
>> it will be called as little as possible.  Unowned subregions will get a
>> region if memory_region_set_owner is called after the subregion is added.
>> This is in general the common case already; often setting the owner can
>> be delegated to a bus-specific API that already takes a DeviceState
>> (for example pci_register_bar or sysbus_init_mmio).
> 
> This feels a bit fragile to me -- there doesn't seem to be
> a clear rule for who has to set the owner of a region or
> when they have to do it, or for ensuring that it doesn't
> get forgotten altogether.

The best rule would be "who creates it".  This would touch half the
files in hw/, which I am trying to avoid.

So I'm settling for:

* in general, the bus sets the owner for regions that are managed by the
bus (patches 5/6/9);

* if the device plays directly with address_space_memory/io (shouldn't
happen except in very special cases, such as patch 7) or if regions are
added/deleted dynamically to a container (patches 8/10/11/12), then the
device must set the owner itself.

All stuff that should be in a comment, I guess...

> What happens if I take a MemoryRegion* that another device
> has exposed to me as a sysbus mmio region (and so claimed
> ownership of) and pass it to pci_register_bar()?

You get an assertion failure.

> Who owns it at that point? [That's a legitimate thing to do, I think,
> though I don't suppose anybody does it at the moment.
> Sysbus MMIOs aren't only for mapping in the system address
> space, they're a general way for one device to expose a
> MemoryRegion * for use by another device.]

I don't think it is legitimate, MMIO regions are just for use via
sysbus_map_mmio.  But the same scenario could apply without sysbus MMIO
in the future (e.g. via QOM properties), so it is a good question.

The right thing to do is to use a container or alias region, and put the
1st region inside it.  Then the 1st region keeps its owner, and the
container/alias gets a new one.

Paolo

Re: [Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Peter Maydell]

On 3 June 2013 07:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/06/2013 18:12, Peter Maydell ha scritto:
>> What happens if I take a MemoryRegion* that another device
>> has exposed to me as a sysbus mmio region (and so claimed
>> ownership of) and pass it to pci_register_bar()?
>
> You get an assertion failure.
>
>> Who owns it at that point? [That's a legitimate thing to do, I think,
>> though I don't suppose anybody does it at the moment.
>> Sysbus MMIOs aren't only for mapping in the system address
>> space, they're a general way for one device to expose a
>> MemoryRegion * for use by another device.]
>
> I don't think it is legitimate, MMIO regions are just for use via
> sysbus_map_mmio.

This is definitely not true. We already make extensive use
of MMIO regions other than simply directly via sysbus_map_mmio.
Exposing a MemoryRegion* is just saying "here is something I
have which is some kind of memory mapped IO, do whatever you
need to with it" (which might be mapping it directly to the
system address space, or might be passing it to some other
device that wants a MemoryRegion*, or might be putting it in
a container MR or otherwise managing it). For example,
arm11mpcore.c does this:
   sysbus_init_mmio(dev, sysbus_mmio_get_region(s->priv, 0));
which I suspect will assert with your patches.
(In general sysbus_mmio_get_region() callers are a good
place to look for uses of sysbus MMIOs other than simple
mapping.)

> The right thing to do is to use a container or alias region, and put the
> 1st region inside it.  Then the 1st region keeps its owner, and the
> container/alias gets a new one.

I think the actual right fix is to make the creator of
the MR specify its owner. Anything else is just going to
have holes in it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Paolo Bonzini]

Il 03/06/2013 11:22, Peter Maydell ha scritto:
>>> Who owns it at that point? [That's a legitimate thing to do, I think,
>>> though I don't suppose anybody does it at the moment.
>>> Sysbus MMIOs aren't only for mapping in the system address
>>> space, they're a general way for one device to expose a
>>> MemoryRegion * for use by another device.]
>>
>> I don't think it is legitimate, MMIO regions are just for use via
>> sysbus_map_mmio.
> 
> This is definitely not true.

Indeed a wrong generalization.  (Though it is becomes almost true if you
replace sysbus_map_mmio with memory_region_add_subregion, for which
sysbus_map_mmio is a simple wrapper).

> We already make extensive use
> of MMIO regions other than simply directly via sysbus_map_mmio.
> Exposing a MemoryRegion* is just saying "here is something I
> have which is some kind of memory mapped IO, do whatever you
> need to with it" (which might be mapping it directly to the
> system address space, or might be passing it to some other
> device that wants a MemoryRegion*, or might be putting it in
> a container MR or otherwise managing it). For example,
> arm11mpcore.c does this:
>    sysbus_init_mmio(dev, sysbus_mmio_get_region(s->priv, 0));
> which I suspect will assert with your patches.

Thanks for the pointer.  All other occurrences of
sys_bus_mmio_get_region are either in non-qdevified OMAP code, or they
do what I said in my patch.

I'll fix a11mpcore to use an alias.

>> The right thing to do is to use a container or alias region, and put the
>> 1st region inside it.  Then the 1st region keeps its owner, and the
>> container/alias gets a new one.
> 
> I think the actual right fix is to make the creator of
> the MR specify its owner. Anything else is just going to
> have holes in it.

I think the rules I wrote down are easy to understand, and I'd really
like to avoid touching 783 instances of memory_region_init*.  The
patches say that devices doing their own stuff with regions are the
exception, not the rule.  Thus the bus functions (which already take a
DeviceState) are just as good a place to set ownership as
memory_region_init*.

Paolo

Re: [Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Peter Maydell]

On 3 June 2013 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 03/06/2013 11:22, Peter Maydell ha scritto:
>> arm11mpcore.c does this:
>>    sysbus_init_mmio(dev, sysbus_mmio_get_region(s->priv, 0));
>> which I suspect will assert with your patches.
>
> Thanks for the pointer.  All other occurrences of
> sys_bus_mmio_get_region are either in non-qdevified OMAP code, or they
> do what I said in my patch.
>
> I'll fix a11mpcore to use an alias.

Why? There is no need to -- this should be a perfectly
reasonable use of MemoryRegion*s. If your reference counting
code can't deal with it you need to fix the reference
counting code.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Paolo Bonzini]

Il 03/06/2013 11:58, Peter Maydell ha scritto:
> On 3 June 2013 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 03/06/2013 11:22, Peter Maydell ha scritto:
>>> arm11mpcore.c does this:
>>>    sysbus_init_mmio(dev, sysbus_mmio_get_region(s->priv, 0));
>>> which I suspect will assert with your patches.
>>
>> Thanks for the pointer.  All other occurrences of
>> sys_bus_mmio_get_region are either in non-qdevified OMAP code, or they
>> do what I said in my patch.
>>
>> I'll fix a11mpcore to use an alias.
> 
> Why? There is no need to -- this should be a perfectly
> reasonable use of MemoryRegion*s. If your reference counting
> code can't deal with it you need to fix the reference
> counting code.

It can:

1) I could set the owner to NULL before calling the sysbus_init_mmio;

2) I could add a variant of sysbus_init_mmio that doesn't set the owner;

3) I could skip setting the owner for sysbus altogether, since it is
only strictly required for unpluggable devices.

What I cannot do is having two owners, where each ref/unref pair will
ref/unref both objects.  I think you agree that it is overkill.

However, I think there is worth in preserving the chain through either
containment or aliasing.  From the nesting point of view,
realview_mpcore is exposing the region.  From the implementor point of
view, arm_gic is implementing the region (and arm_gic is the one that
would be ref/unref'd at execution time).  In either case,
arm11mpcore_priv is not what you want to see.  Its presence is just an
implementation detail of realview_mpcore.

Paolo

Re: [Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Peter Maydell]

On 3 June 2013 11:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 1) I could set the owner to NULL before calling the sysbus_init_mmio;
>
> 2) I could add a variant of sysbus_init_mmio that doesn't set the owner;
>
> 3) I could skip setting the owner for sysbus altogether, since it is
> only strictly required for unpluggable devices.

4) you could set the owner at the right place, ie when the
memory region is created.

> However, I think there is worth in preserving the chain through either
> containment or aliasing.  From the nesting point of view,
> realview_mpcore is exposing the region.  From the implementor point of
> view, arm_gic is implementing the region (and arm_gic is the one that
> would be ref/unref'd at execution time).  In either case,
> arm11mpcore_priv is not what you want to see.  Its presence is just an
> implementation detail of realview_mpcore.

I agree that the owner of the MR in this case should be
arm_gic. For aliasing, surely you need to actually reference
both devices? If the device that owns the container goes
away then your MR* is toast anyway, and if the device doing
the actual implementation goes away your MR* is also now
invalid. So they have to both hang around long enough for
you to finish whatever you were doing.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Paolo Bonzini]

Il 03/06/2013 12:25, Peter Maydell ha scritto:
> On 3 June 2013 11:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> 1) I could set the owner to NULL before calling the sysbus_init_mmio;
>>
>> 2) I could add a variant of sysbus_init_mmio that doesn't set the owner;
>>
>> 3) I could skip setting the owner for sysbus altogether, since it is
>> only strictly required for unpluggable devices.
> 
> 4) you could set the owner at the right place, ie when the
> memory region is created.
> 
>> However, I think there is worth in preserving the chain through either
>> containment or aliasing.  From the nesting point of view,
>> realview_mpcore is exposing the region.  From the implementor point of
>> view, arm_gic is implementing the region (and arm_gic is the one that
>> would be ref/unref'd at execution time).  In either case,
>> arm11mpcore_priv is not what you want to see.  Its presence is just an
>> implementation detail of realview_mpcore.
> 
> I agree that the owner of the MR in this case should be
> arm_gic.

The owner of the I/O region is arm_gic.  It is set when arm_gic does
sysbus_init_mmio.  But container regions have an owner too.  Here the
owner is set initially to arm11mpcore_priv, and then it bombs when
realview_mpcore tries to change it to itself.

Make each level wrap the region with a container makes sense to me.  But
actually sysbus_pass_mmio would match nicely sysbus_pass_irq as an API,
so (2) above would be a good choice too.

What I'm definitely not going to do, is go through 800 calls and add
owners to all of them.

> For aliasing, surely you need to actually reference
> both devices? If the device that owns the container goes
> away then your MR* is toast anyway, and if the device doing
> the actual implementation goes away your MR* is also now
> invalid. So they have to both hang around long enough for
> you to finish whatever you were doing.

Container and alias regions are "virtual", they are eliminated while
creating the flat memory map; I/O goes straight to the contained/aliased
region.  Hence, containers and aliases in principle need no owner; it is
still nice to have one, for symmetry and ease of debugging.

A reference to the aliased region is added when:

(1) the region is added to the flat memory map;

(2) the alias is created.

Both are done in patch 3.

Paolo