* [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find
@ 2013-05-06 14:25 Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 1/8] memory: add ref/unref calls Paolo Bonzini
` (10 more replies)
0 siblings, 11 replies; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-06 14:25 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, qemulist, stefanha
Hi,
this is an alternative approach to refactoring of dataplane's HostMem
code. Here, I take Ping Fan's idea of RCU-style updating of the
region list and apply it to the AddressSpace's FlatView. With this
change, dataplane can simply use memory_region_find instead of
hostmem.
This is a somewhat larger change, but I prefer it for two reasons.
1) it splits the task of adding BQL-less memory dispatch in two parts,
tacking memory_region_find first (which is simpler because locking
is left to the caller).
2) HostMem duplicates a lot of the FlatView logic, and adding the
RCU-style update in FlatView benefits everyone.
The missing ingredients here are:
1) remember and unreference the MemoryRegions that are used in
a vring entry. In order to implement this, it is probably simpler
to change vring.c to use virtio.c's VirtQueueElement data structure.
We want something like that anyway in order to support migration.
2) add an owner field to MemoryRegion, and set it for all MemoryRegions
for hot-unpluggable devices. In this series, ref/unref are stubs.
For simplicity I based the patches on my IOMMU rebase. I placed the
tree at git://github.com/bonzini/qemu.git, branch iommu.
Paolo
Paolo Bonzini (8):
memory: add ref/unref 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
memory: access FlatView from a local variable
memory: use a new FlatView pointer on every topology update
memory: add reference counting to FlatView
dataplane: replace hostmem with memory_region_find
exec.c | 63 +++++++++---
hw/core/loader.c | 1 +
hw/display/exynos4210_fimd.c | 6 +
hw/display/framebuffer.c | 10 +-
hw/i386/kvm/ioapic.c | 2 +
hw/i386/kvmvapic.c | 1 +
hw/misc/vfio.c | 2 +
hw/virtio/dataplane/Makefile.objs | 2 +-
hw/virtio/dataplane/hostmem.c | 176 ---------------------------------
hw/virtio/dataplane/vring.c | 56 +++++++++--
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/memory.h | 9 ++
include/hw/virtio/dataplane/hostmem.h | 57 -----------
include/hw/virtio/dataplane/vring.h | 3 +-
kvm-all.c | 2 +
memory.c | 142 +++++++++++++++++++++-----
target-arm/kvm.c | 2 +
target-i386/kvm.c | 4 +-
target-sparc/mmu_helper.c | 1 +
xen-all.c | 2 +
23 files changed, 253 insertions(+), 297 deletions(-)
delete mode 100644 hw/virtio/dataplane/hostmem.c
delete mode 100644 include/hw/virtio/dataplane/hostmem.h
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [RFC PATCH 1/8] memory: add ref/unref calls
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
@ 2013-05-06 14:25 ` Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 2/8] exec: check MRU in qemu_ram_addr_from_host Paolo Bonzini
` (9 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-06 14:25 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, qemulist, stefanha
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 | 10 ++++++----
hw/i386/kvm/ioapic.c | 2 ++
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/exec/memory.h | 9 +++++++++
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 ++
18 files changed, 70 insertions(+), 5 deletions(-)
diff --git a/exec.c b/exec.c
index 91cd28f..9f324bb 100644
--- a/exec.c
+++ b/exec.c
@@ -753,12 +753,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 7507914..97e7ba2 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 = 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 6cb5016..afa2e54 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 = 0;
w->host_fb_addr = NULL;
diff --git a/hw/display/framebuffer.c b/hw/display/framebuffer.c
index 6be31db..8288b93 100644
--- a/hw/display/framebuffer.c
+++ b/hw/display/framebuffer.c
@@ -54,10 +54,10 @@ void framebuffer_update_display(
src_len = src_width * rows;
mem_section = memory_region_find(address_space, base, src_len);
+ mem = mem_section.mr;
if (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);
@@ -67,10 +67,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);
@@ -107,4 +107,6 @@ void framebuffer_update_display(
DIRTY_MEMORY_VGA);
*first_row = first;
*last_row = last;
+out:
+ memory_region_unref(mem);
}
diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index 3ad951e..9192b39 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -114,6 +114,8 @@ static void kvm_ioapic_put(IOAPICCommonState *s)
fprintf(stderr, "KVM_GET_IRQCHIP failed: %s\n", strerror(ret));
abort();
}
+
+ memory_region_unref(mrs.mr);
}
static void kvm_ioapic_reset(DeviceState *dev)
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 5b558aa..6a98c7f 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 178dd11..0ae6878 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 37292ff..66829bb 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 = 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 fbabf99..190522b 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 d669756..fac8800 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/exec/memory.h b/include/exec/memory.h
index d2bbfb7..fb1b42f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -252,6 +252,15 @@ struct MemoryListener {
void memory_region_init(MemoryRegion *mr,
const char *name,
uint64_t size);
+
+static inline void memory_region_ref(MemoryRegion *mr)
+{
+}
+
+static inline void memory_region_unref(MemoryRegion *mr)
+{
+}
+
/**
* memory_region_init_io: Initialize an I/O memory region.
*
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 3a31602..1d876aa 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -766,6 +766,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);
}
@@ -773,6 +774,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 aa0eeb7..bc4cf4b 100644
--- a/memory.c
+++ b/memory.c
@@ -145,6 +145,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, \
@@ -260,11 +261,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);
}
@@ -763,6 +770,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);
@@ -960,6 +972,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;
}
@@ -1504,6 +1518,8 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
ret.size = int128_get64(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 a9649ae..3c1ccc2 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 (!section.size) {
return -1;
}
diff --git a/xen-all.c b/xen-all.c
index 539a154..2f24ced 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.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [RFC PATCH 2/8] exec: check MRU in qemu_ram_addr_from_host
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 1/8] memory: add ref/unref calls Paolo Bonzini
@ 2013-05-06 14:25 ` Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 3/8] memory: return MemoryRegion from qemu_ram_addr_from_host Paolo Bonzini
` (8 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-06 14:25 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, qemulist, stefanha
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 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/exec.c b/exec.c
index 9f324bb..8e46228 100644
--- a/exec.c
+++ b/exec.c
@@ -1404,18 +1404,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.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [RFC PATCH 3/8] memory: return MemoryRegion from qemu_ram_addr_from_host
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 1/8] memory: add ref/unref calls Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 2/8] exec: check MRU in qemu_ram_addr_from_host Paolo Bonzini
@ 2013-05-06 14:25 ` Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 4/8] memory: ref/unref memory across address_space_map/unmap Paolo Bonzini
` (7 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-06 14:25 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, qemulist, stefanha
It will be needed in the next patch.
The common parts of qemu_get_ram_ptr and qemu_ram_addr_from_host are
moved to a new function qemu_get_ram_block.
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 8e46228..54b57fc 100644
--- a/exec.c
+++ b/exec.c
@@ -1287,15 +1287,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;
@@ -1315,6 +1307,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.
@@ -1394,14 +1401,14 @@ void qemu_put_ram_ptr(void *addr)
trace_qemu_put_ram_ptr(addr);
}
-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;
@@ -1419,11 +1426,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
@@ -1432,7 +1439,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 2e5f11f..84dfd3b 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -53,7 +53,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
void *qemu_get_ram_ptr(ram_addr_t addr);
void qemu_put_ram_ptr(void *addr);
/* 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.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [RFC PATCH 4/8] memory: ref/unref memory across address_space_map/unmap
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
` (2 preceding siblings ...)
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 3/8] memory: return MemoryRegion from qemu_ram_addr_from_host Paolo Bonzini
@ 2013-05-06 14:25 ` Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 5/8] memory: access FlatView from a local variable Paolo Bonzini
` (6 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-06 14:25 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, qemulist, stefanha
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 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/exec.c b/exec.c
index 54b57fc..54ed203 100644
--- a/exec.c
+++ b/exec.c
@@ -2077,6 +2077,7 @@ void cpu_physical_memory_write_rom(hwaddr addr,
}
typedef struct {
+ MemoryRegion *mr;
void *buffer;
hwaddr addr;
hwaddr len;
@@ -2171,15 +2172,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 = section->mr;
if (!is_write) {
address_space_read(as, addr, bounce.buffer, l);
}
*plen = l;
+ memory_region_ref(section->mr);
return bounce.buffer;
}
if (!todo) {
raddr = memory_region_get_ram_addr(section->mr) + xlat;
+ memory_region_ref(section->mr);
} else {
if (memory_region_get_ram_addr(section->mr) + xlat != raddr + todo) {
break;
@@ -2204,8 +2208,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;
@@ -2219,6 +2227,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) {
@@ -2226,6 +2235,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.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [RFC PATCH 5/8] memory: access FlatView from a local variable
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
` (3 preceding siblings ...)
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 4/8] memory: ref/unref memory across address_space_map/unmap Paolo Bonzini
@ 2013-05-06 14:25 ` Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 6/8] memory: use a new FlatView pointer on every topology update Paolo Bonzini
` (5 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-06 14:25 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, qemulist, stefanha
We will soon require accesses to as->current_map to be placed under
a lock (with reference counting so as to keep the critical section
small). To simplify this change, always fetch as->current_map into
a local variable and access it through that variable.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
memory.c | 31 +++++++++++++++++++++----------
1 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/memory.c b/memory.c
index bc4cf4b..553b04c 100644
--- a/memory.c
+++ b/memory.c
@@ -632,13 +632,15 @@ static void address_space_add_del_ioeventfds(AddressSpace *as,
static void address_space_update_ioeventfds(AddressSpace *as)
{
+ FlatView *view;
FlatRange *fr;
unsigned ioeventfd_nb = 0;
MemoryRegionIoeventfd *ioeventfds = NULL;
AddrRange tmp;
unsigned i;
- FOR_EACH_FLAT_RANGE(fr, as->current_map) {
+ view = as->current_map;
+ FOR_EACH_FLAT_RANGE(fr, view) {
for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
tmp = addrrange_shift(fr->mr->ioeventfds[i].addr,
int128_sub(fr->addr.start,
@@ -1134,7 +1136,8 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
FlatRange *fr;
QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
- FOR_EACH_FLAT_RANGE(fr, as->current_map) {
+ FlatView *view = as->current_map;
+ FOR_EACH_FLAT_RANGE(fr, view) {
if (fr->mr == mr) {
MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync);
}
@@ -1184,12 +1187,14 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpace *as)
{
+ FlatView *view;
FlatRange *fr;
CoalescedMemoryRange *cmr;
AddrRange tmp;
MemoryRegionSection section;
- FOR_EACH_FLAT_RANGE(fr, as->current_map) {
+ view = as->current_map;
+ FOR_EACH_FLAT_RANGE(fr, view) {
if (fr->mr == mr) {
section = (MemoryRegionSection) {
.address_space = as,
@@ -1476,9 +1481,9 @@ static int cmp_flatrange_addr(const void *addr_, const void *fr_)
return 0;
}
-static FlatRange *address_space_lookup(AddressSpace *as, AddrRange addr)
+static FlatRange *flatview_lookup(FlatView *view, AddrRange addr)
{
- return bsearch(&addr, as->current_map->ranges, as->current_map->nr,
+ return bsearch(&addr, view->ranges, view->nr,
sizeof(FlatRange), cmp_flatrange_addr);
}
@@ -1489,6 +1494,7 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
MemoryRegion *root;
AddressSpace *as;
AddrRange range;
+ FlatView *view;
FlatRange *fr;
addr += mr->addr;
@@ -1499,13 +1505,14 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
as = memory_region_to_address_space(root);
range = addrrange_make(int128_make64(addr), int128_make64(size));
- fr = address_space_lookup(as, range);
+
+ view = as->current_map;
+ fr = flatview_lookup(view, range);
if (!fr) {
return ret;
}
- while (fr > as->current_map->ranges
- && addrrange_intersects(fr[-1].addr, range)) {
+ while (fr > view->ranges && addrrange_intersects(fr[-1].addr, range)) {
--fr;
}
@@ -1525,9 +1532,11 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
void address_space_sync_dirty_bitmap(AddressSpace *as)
{
+ FlatView *view;
FlatRange *fr;
- FOR_EACH_FLAT_RANGE(fr, as->current_map) {
+ view = as->current_map;
+ FOR_EACH_FLAT_RANGE(fr, view) {
MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync);
}
}
@@ -1547,6 +1556,7 @@ void memory_global_dirty_log_stop(void)
static void listener_add_address_space(MemoryListener *listener,
AddressSpace *as)
{
+ FlatView *view;
FlatRange *fr;
if (listener->address_space_filter
@@ -1560,7 +1570,8 @@ static void listener_add_address_space(MemoryListener *listener,
}
}
- FOR_EACH_FLAT_RANGE(fr, as->current_map) {
+ view = as->current_map;
+ FOR_EACH_FLAT_RANGE(fr, view) {
MemoryRegionSection section = {
.mr = fr->mr,
.address_space = as,
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [RFC PATCH 6/8] memory: use a new FlatView pointer on every topology update
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
` (4 preceding siblings ...)
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 5/8] memory: access FlatView from a local variable Paolo Bonzini
@ 2013-05-06 14:25 ` Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 7/8] memory: add reference counting to FlatView Paolo Bonzini
` (4 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-06 14:25 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, qemulist, stefanha
This is the first step towards converting as->current_map to
RCU-style updates, where the FlatView updates run concurrently
with uses of an old FlatView.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
memory.c | 34 ++++++++++++++++++----------------
1 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/memory.c b/memory.c
index 553b04c..fbb2657 100644
--- a/memory.c
+++ b/memory.c
@@ -273,6 +273,7 @@ static void flatview_destroy(FlatView *view)
memory_region_unref(view->ranges[i].mr);
}
g_free(view->ranges);
+ g_free(view);
}
static bool can_merge(FlatRange *r1, FlatRange *r2)
@@ -566,17 +567,18 @@ static void render_memory_region(FlatView *view,
}
/* Render a memory topology into a list of disjoint absolute ranges. */
-static FlatView generate_memory_topology(MemoryRegion *mr)
+static FlatView *generate_memory_topology(MemoryRegion *mr)
{
- FlatView view;
+ FlatView *view;
- flatview_init(&view);
+ view = g_new(FlatView, 1);
+ flatview_init(view);
if (mr) {
- render_memory_region(&view, mr, int128_zero(),
+ render_memory_region(view, mr, int128_zero(),
addrrange_make(int128_zero(), int128_2_64()), false);
}
- flatview_simplify(&view);
+ flatview_simplify(view);
return view;
}
@@ -664,8 +666,8 @@ static void address_space_update_ioeventfds(AddressSpace *as)
}
static void address_space_update_topology_pass(AddressSpace *as,
- FlatView old_view,
- FlatView new_view,
+ const FlatView *old_view,
+ const FlatView *new_view,
bool adding)
{
unsigned iold, inew;
@@ -675,14 +677,14 @@ static void address_space_update_topology_pass(AddressSpace *as,
* Kill ranges in the old map, and instantiate ranges in the new map.
*/
iold = inew = 0;
- while (iold < old_view.nr || inew < new_view.nr) {
- if (iold < old_view.nr) {
- frold = &old_view.ranges[iold];
+ while (iold < old_view->nr || inew < new_view->nr) {
+ if (iold < old_view->nr) {
+ frold = &old_view->ranges[iold];
} else {
frold = NULL;
}
- if (inew < new_view.nr) {
- frnew = &new_view.ranges[inew];
+ if (inew < new_view->nr) {
+ frnew = &new_view->ranges[inew];
} else {
frnew = NULL;
}
@@ -728,14 +730,14 @@ static void address_space_update_topology_pass(AddressSpace *as,
static void address_space_update_topology(AddressSpace *as)
{
- FlatView old_view = *as->current_map;
- FlatView new_view = generate_memory_topology(as->root);
+ FlatView *old_view = as->current_map;
+ FlatView *new_view = generate_memory_topology(as->root);
address_space_update_topology_pass(as, old_view, new_view, false);
address_space_update_topology_pass(as, old_view, new_view, true);
- *as->current_map = new_view;
- flatview_destroy(&old_view);
+ as->current_map = new_view;
+ flatview_destroy(old_view);
address_space_update_ioeventfds(as);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [RFC PATCH 7/8] memory: add reference counting to FlatView
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
` (5 preceding siblings ...)
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 6/8] memory: use a new FlatView pointer on every topology update Paolo Bonzini
@ 2013-05-06 14:25 ` Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 8/8] dataplane: replace hostmem with memory_region_find Paolo Bonzini
` (3 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-06 14:25 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, qemulist, stefanha
With this change, a FlatView can be used even after a concurrent
update has replaced it. Because we do not have RCU, we use a
mutex to protect the small critical sections that read/write the
as->current_map pointer. Accesses to the FlatView can be done
outside the mutex.
If a MemoryRegion will be used after the FlatView is unref-ed (or after
a MemoryListener callback is returned), a reference has to be added to
that MemoryRegion. For example, memory_region_find adds a reference to
the MemoryRegion that it returns.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
memory.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/memory.c b/memory.c
index fbb2657..f02a3cc 100644
--- a/memory.c
+++ b/memory.c
@@ -26,12 +26,26 @@ static unsigned memory_region_transaction_depth;
static bool memory_region_update_pending;
static bool global_dirty_log = false;
+/* Either the flat_view_mutex or the iothread mutex can be taken around reads
+ * of as->current_map; the critical section is extremely short, so I'm using a
+ * single mutex for every AS. We could also RCU for the read-side.
+ *
+ * Both locks are taken while writing to as->current_map (with the iothread
+ * mutex outside).
+ */
+static QemuMutex flat_view_mutex;
+
static QTAILQ_HEAD(memory_listeners, MemoryListener) memory_listeners
= QTAILQ_HEAD_INITIALIZER(memory_listeners);
static QTAILQ_HEAD(, AddressSpace) address_spaces
= QTAILQ_HEAD_INITIALIZER(address_spaces);
+static void memory_init(void)
+{
+ qemu_mutex_init(&flat_view_mutex);
+}
+
typedef struct AddrRange AddrRange;
/*
@@ -222,6 +236,7 @@ struct FlatRange {
* order.
*/
struct FlatView {
+ unsigned ref;
FlatRange *ranges;
unsigned nr;
unsigned nr_allocated;
@@ -243,6 +258,7 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b)
static void flatview_init(FlatView *view)
{
+ view->ref = 1;
view->ranges = NULL;
view->nr = 0;
view->nr_allocated = 0;
@@ -276,6 +292,18 @@ static void flatview_destroy(FlatView *view)
g_free(view);
}
+static void flatview_ref(FlatView *view)
+{
+ __sync_fetch_and_add(&view->ref, 1);
+}
+
+static void flatview_unref(FlatView *view)
+{
+ if (__sync_fetch_and_sub(&view->ref, 1) == 1) {
+ flatview_destroy(view);
+ }
+}
+
static bool can_merge(FlatRange *r1, FlatRange *r2)
{
return int128_eq(addrrange_end(r1->addr), r2->addr.start)
@@ -728,16 +756,38 @@ static void address_space_update_topology_pass(AddressSpace *as,
}
+static FlatView *address_space_get_flatview(AddressSpace *as)
+{
+ FlatView *view;
+
+ qemu_mutex_lock(&flat_view_mutex);
+ view = as->current_map;
+ flatview_ref(view);
+ qemu_mutex_unlock(&flat_view_mutex);
+ return view;
+}
+
static void address_space_update_topology(AddressSpace *as)
{
- FlatView *old_view = as->current_map;
+ FlatView *old_view = address_space_get_flatview(as);
FlatView *new_view = generate_memory_topology(as->root);
address_space_update_topology_pass(as, old_view, new_view, false);
address_space_update_topology_pass(as, old_view, new_view, true);
+ qemu_mutex_lock(&flat_view_mutex);
+ flatview_unref(as->current_map);
as->current_map = new_view;
- flatview_destroy(old_view);
+ qemu_mutex_unlock(&flat_view_mutex);
+
+ /* Note that all the old MemoryRegions are still alive up to this
+ * point. This relieves most MemoryListeners from the need to
+ * ref/unref the MemoryRegions they get---unless they use them
+ * outside the iothread mutex, in which case precise reference
+ * counting is necessary.
+ */
+ flatview_unref(old_view);
+
address_space_update_ioeventfds(as);
}
@@ -1138,12 +1188,13 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
FlatRange *fr;
QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
- FlatView *view = as->current_map;
+ FlatView *view = address_space_get_flatview(as);
FOR_EACH_FLAT_RANGE(fr, view) {
if (fr->mr == mr) {
MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync);
}
}
+ flatview_unref(view);
}
}
@@ -1195,7 +1246,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
AddrRange tmp;
MemoryRegionSection section;
- view = as->current_map;
+ view = address_space_get_flatview(as);
FOR_EACH_FLAT_RANGE(fr, view) {
if (fr->mr == mr) {
section = (MemoryRegionSection) {
@@ -1221,6 +1272,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
}
}
}
+ flatview_unref(view);
}
static void memory_region_update_coalesced_range(MemoryRegion *mr)
@@ -1508,7 +1560,7 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
as = memory_region_to_address_space(root);
range = addrrange_make(int128_make64(addr), int128_make64(size));
- view = as->current_map;
+ view = address_space_get_flatview(as);
fr = flatview_lookup(view, range);
if (!fr) {
return ret;
@@ -1529,6 +1581,7 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
ret.readonly = fr->readonly;
memory_region_ref(ret.mr);
+ flatview_unref(view);
return ret;
}
@@ -1537,10 +1590,11 @@ void address_space_sync_dirty_bitmap(AddressSpace *as)
FlatView *view;
FlatRange *fr;
- view = as->current_map;
+ view = address_space_get_flatview(as);
FOR_EACH_FLAT_RANGE(fr, view) {
MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync);
}
+ flatview_unref(view);
}
void memory_global_dirty_log_start(void)
@@ -1572,7 +1626,7 @@ static void listener_add_address_space(MemoryListener *listener,
}
}
- view = as->current_map;
+ view = address_space_get_flatview(as);
FOR_EACH_FLAT_RANGE(fr, view) {
MemoryRegionSection section = {
.mr = fr->mr,
@@ -1586,6 +1640,7 @@ static void listener_add_address_space(MemoryListener *listener,
listener->region_add(listener, §ion);
}
}
+ flatview_unref(view);
}
void memory_listener_register(MemoryListener *listener, AddressSpace *filter)
@@ -1619,6 +1674,10 @@ void memory_listener_unregister(MemoryListener *listener)
void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
{
+ if (QTAILQ_EMPTY(&address_spaces)) {
+ memory_init();
+ }
+
memory_region_transaction_begin();
as->root = root;
as->current_map = g_new(FlatView, 1);
@@ -1640,7 +1699,7 @@ void address_space_destroy(AddressSpace *as)
memory_region_transaction_commit();
QTAILQ_REMOVE(&address_spaces, as, address_spaces_link);
address_space_destroy_dispatch(as);
- flatview_destroy(as->current_map);
+ flatview_unref(as->current_map);
g_free(as->name);
g_free(as->current_map);
g_free(as->ioeventfds);
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [RFC PATCH 8/8] dataplane: replace hostmem with memory_region_find
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
` (6 preceding siblings ...)
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 7/8] memory: add reference counting to FlatView Paolo Bonzini
@ 2013-05-06 14:25 ` Paolo Bonzini
2013-05-08 6:20 ` [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, " liu ping fan
` (2 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-06 14:25 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, qemulist, stefanha
This is incomplete, because the region is unref-ed immediately.
It would not be safe against memory hot-unplug. Otherwise it
works.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/virtio/dataplane/Makefile.objs | 2 +-
hw/virtio/dataplane/hostmem.c | 183 ---------------------------------
hw/virtio/dataplane/vring.c | 56 ++++++++--
include/hw/virtio/dataplane/hostmem.h | 58 -----------
include/hw/virtio/dataplane/vring.h | 3 +-
5 files changed, 48 insertions(+), 254 deletions(-)
delete mode 100644 hw/virtio/dataplane/hostmem.c
delete mode 100644 include/hw/virtio/dataplane/hostmem.h
diff --git a/hw/virtio/dataplane/Makefile.objs b/hw/virtio/dataplane/Makefile.objs
index a91bf33..9a8cfc0 100644
--- a/hw/virtio/dataplane/Makefile.objs
+++ b/hw/virtio/dataplane/Makefile.objs
@@ -1 +1 @@
-common-obj-y += hostmem.o vring.o
+common-obj-y += vring.o
diff --git a/hw/virtio/dataplane/hostmem.c b/hw/virtio/dataplane/hostmem.c
deleted file mode 100644
index 66829bb..0000000
--- a/hw/virtio/dataplane/hostmem.c
+++ /dev/null
@@ -1,183 +0,0 @@
-/*
- * Thread-safe guest to host memory mapping
- *
- * Copyright 2012 Red Hat, Inc. and/or its affiliates
- *
- * Authors:
- * Stefan Hajnoczi <stefanha@redhat.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#include "exec/address-spaces.h"
-#include "hw/virtio/dataplane/hostmem.h"
-
-static int hostmem_lookup_cmp(const void *phys_, const void *region_)
-{
- hwaddr phys = *(const hwaddr *)phys_;
- const HostMemRegion *region = region_;
-
- if (phys < region->guest_addr) {
- return -1;
- } else if (phys >= region->guest_addr + region->size) {
- return 1;
- } else {
- return 0;
- }
-}
-
-/**
- * Map guest physical address to host pointer
- */
-void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
-{
- HostMemRegion *region;
- void *host_addr = NULL;
- hwaddr offset_within_region;
-
- qemu_mutex_lock(&hostmem->current_regions_lock);
- region = bsearch(&phys, hostmem->current_regions,
- hostmem->num_current_regions,
- sizeof(hostmem->current_regions[0]),
- hostmem_lookup_cmp);
- if (!region) {
- goto out;
- }
- if (is_write && region->readonly) {
- goto out;
- }
- offset_within_region = phys - region->guest_addr;
- if (len <= region->size - offset_within_region) {
- host_addr = region->host_addr + offset_within_region;
- }
-out:
- qemu_mutex_unlock(&hostmem->current_regions_lock);
-
- return host_addr;
-}
-
-/**
- * Install new regions list
- */
-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;
- qemu_mutex_unlock(&hostmem->current_regions_lock);
-
- /* Reset new regions list */
- hostmem->new_regions = NULL;
- hostmem->num_new_regions = 0;
-}
-
-/**
- * Add a MemoryRegionSection to the new regions list
- */
-static void hostmem_append_new_region(HostMem *hostmem,
- MemoryRegionSection *section)
-{
- void *ram_ptr = memory_region_get_ram_ptr(section->mr);
- size_t num = hostmem->num_new_regions;
- size_t new_size = (num + 1) * sizeof(hostmem->new_regions[0]);
-
- hostmem->new_regions = g_realloc(hostmem->new_regions, new_size);
- hostmem->new_regions[num] = (HostMemRegion){
- .host_addr = ram_ptr + section->offset_within_region,
- .guest_addr = section->offset_within_address_space,
- .size = 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,
- MemoryRegionSection *section)
-{
- HostMem *hostmem = container_of(listener, HostMem, listener);
-
- /* Ignore non-RAM regions, we may not be able to map them */
- if (!memory_region_is_ram(section->mr)) {
- return;
- }
-
- /* Ignore regions with dirty logging, we cannot mark them dirty */
- if (memory_region_is_logging(section->mr)) {
- return;
- }
-
- hostmem_append_new_region(hostmem, section);
-}
-
-/* We don't implement most MemoryListener callbacks, use these nop stubs */
-static void hostmem_listener_dummy(MemoryListener *listener)
-{
-}
-
-static void hostmem_listener_section_dummy(MemoryListener *listener,
- MemoryRegionSection *section)
-{
-}
-
-static void hostmem_listener_eventfd_dummy(MemoryListener *listener,
- MemoryRegionSection *section,
- bool match_data, uint64_t data,
- EventNotifier *e)
-{
-}
-
-static void hostmem_listener_coalesced_mmio_dummy(MemoryListener *listener,
- MemoryRegionSection *section,
- hwaddr addr, hwaddr len)
-{
-}
-
-void hostmem_init(HostMem *hostmem)
-{
- memset(hostmem, 0, sizeof(*hostmem));
-
- qemu_mutex_init(&hostmem->current_regions_lock);
-
- hostmem->listener = (MemoryListener){
- .begin = hostmem_listener_dummy,
- .commit = hostmem_listener_commit,
- .region_add = hostmem_listener_append_region,
- .region_del = hostmem_listener_section_dummy,
- .region_nop = hostmem_listener_append_region,
- .log_start = hostmem_listener_section_dummy,
- .log_stop = hostmem_listener_section_dummy,
- .log_sync = hostmem_listener_section_dummy,
- .log_global_start = hostmem_listener_dummy,
- .log_global_stop = hostmem_listener_dummy,
- .eventfd_add = hostmem_listener_eventfd_dummy,
- .eventfd_del = hostmem_listener_eventfd_dummy,
- .coalesced_mmio_add = hostmem_listener_coalesced_mmio_dummy,
- .coalesced_mmio_del = hostmem_listener_coalesced_mmio_dummy,
- .priority = 10,
- };
-
- memory_listener_register(&hostmem->listener, &address_space_memory);
- if (hostmem->num_new_regions > 0) {
- hostmem_listener_commit(&hostmem->listener);
- }
-}
-
-void hostmem_finalize(HostMem *hostmem)
-{
- memory_listener_unregister(&hostmem->listener);
- g_free(hostmem->new_regions);
- g_free(hostmem->current_regions);
- qemu_mutex_destroy(&hostmem->current_regions_lock);
-}
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index e0d6e83..3b94279 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -15,9 +15,41 @@
*/
#include "trace.h"
+#include "hw/hw.h"
+#include "exec/memory.h"
+#include "exec/address-spaces.h"
#include "hw/virtio/dataplane/vring.h"
#include "qemu/error-report.h"
+static void *vring_map(MemoryRegion **mr, hwaddr phys, hwaddr len,
+ bool is_write)
+{
+ MemoryRegionSection section = memory_region_find(get_system_memory(), phys, len);
+
+ if (!section.mr || section.size < len) {
+ goto out;
+ }
+ if (is_write && section.readonly) {
+ goto out;
+ }
+ if (!memory_region_is_ram(section.mr)) {
+ goto out;
+ }
+
+ /* Ignore regions with dirty logging, we cannot mark them dirty */
+ if (memory_region_is_logging(section.mr)) {
+ goto out;
+ }
+
+ *mr = section.mr;
+ return memory_region_get_ram_ptr(section.mr) + section.offset_within_region;
+
+out:
+ memory_region_unref(section.mr);
+ *mr = NULL;
+ return NULL;
+}
+
/* Map the guest's vring to host memory */
bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
{
@@ -27,8 +59,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
vring->broken = false;
- hostmem_init(&vring->hostmem);
- vring_ptr = hostmem_lookup(&vring->hostmem, vring_addr, vring_size, true);
+ vring_ptr = vring_map(&vring->mr, vring_addr, vring_size, true);
if (!vring_ptr) {
error_report("Failed to map vring "
"addr %#" HWADDR_PRIx " size %" HWADDR_PRIu,
@@ -51,7 +82,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
void vring_teardown(Vring *vring)
{
- hostmem_finalize(&vring->hostmem);
+ memory_region_unref(vring->mr);
}
/* Disable guest->host notifies */
@@ -136,11 +167,13 @@ static int get_indirect(Vring *vring,
do {
struct vring_desc *desc_ptr;
+ MemoryRegion *mr;
/* Translate indirect descriptor */
- desc_ptr = hostmem_lookup(&vring->hostmem,
- indirect->addr + found * sizeof(desc),
- sizeof(desc), false);
+ desc_ptr = vring_map(&mr,
+ indirect->addr + found * sizeof(desc),
+ sizeof(desc), false);
+ memory_region_unref(mr); /* TODO */
if (!desc_ptr) {
error_report("Failed to map indirect descriptor "
"addr %#" PRIx64 " len %zu",
@@ -172,8 +205,9 @@ static int get_indirect(Vring *vring,
return -ENOBUFS;
}
- iov->iov_base = hostmem_lookup(&vring->hostmem, desc.addr, desc.len,
- desc.flags & VRING_DESC_F_WRITE);
+ iov->iov_base = vring_map(&mr, desc.addr, desc.len,
+ desc.flags & VRING_DESC_F_WRITE);
+ memory_region_unref(mr); /* TODO */
if (!iov->iov_base) {
error_report("Failed to map indirect descriptor"
"addr %#" PRIx64 " len %u",
@@ -221,6 +255,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
struct vring_desc desc;
unsigned int i, head, found = 0, num = vring->vr.num;
uint16_t avail_idx, last_avail_idx;
+ MemoryRegion *mr;
/* If there was a fatal error then refuse operation */
if (vring->broken) {
@@ -300,8 +335,9 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
}
/* TODO handle non-contiguous memory across region boundaries */
- iov->iov_base = hostmem_lookup(&vring->hostmem, desc.addr, desc.len,
- desc.flags & VRING_DESC_F_WRITE);
+ iov->iov_base = vring_map(&mr, desc.addr, desc.len,
+ desc.flags & VRING_DESC_F_WRITE);
+ memory_region_unref(mr); /* TODO */
if (!iov->iov_base) {
error_report("Failed to map vring desc addr %#" PRIx64 " len %u",
(uint64_t)desc.addr, desc.len);
diff --git a/include/hw/virtio/dataplane/hostmem.h b/include/hw/virtio/dataplane/hostmem.h
deleted file mode 100644
index 2810f4b..0000000
--- a/include/hw/virtio/dataplane/hostmem.h
+++ /dev/null
@@ -1,58 +0,0 @@
-/*
- * Thread-safe guest to host memory mapping
- *
- * Copyright 2012 Red Hat, Inc. and/or its affiliates
- *
- * Authors:
- * Stefan Hajnoczi <stefanha@redhat.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#ifndef HOSTMEM_H
-#define HOSTMEM_H
-
-#include "exec/memory.h"
-#include "qemu/thread.h"
-
-typedef struct {
- MemoryRegion *mr;
- void *host_addr;
- hwaddr guest_addr;
- uint64_t size;
- bool readonly;
-} HostMemRegion;
-
-typedef struct {
- /* The listener is invoked when regions change and a new list of regions is
- * built up completely before they are installed.
- */
- MemoryListener listener;
- HostMemRegion *new_regions;
- size_t num_new_regions;
-
- /* Current regions are accessed from multiple threads either to lookup
- * addresses or to install a new list of regions. The lock protects the
- * pointer and the regions.
- */
- QemuMutex current_regions_lock;
- HostMemRegion *current_regions;
- size_t num_current_regions;
-} HostMem;
-
-void hostmem_init(HostMem *hostmem);
-void hostmem_finalize(HostMem *hostmem);
-
-/**
- * Map a guest physical address to a pointer
- *
- * Note that there is map/unmap mechanism here. The caller must ensure that
- * mapped memory is no longer used across events like hot memory unplug. This
- * can be done with other mechanisms like bdrv_drain_all() that quiesce
- * in-flight I/O.
- */
-void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write);
-
-#endif /* HOSTMEM_H */
diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
index 9380cb5..caf5a41 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -19,11 +19,10 @@
#include <linux/virtio_ring.h>
#include "qemu-common.h"
-#include "hostmem.h"
#include "hw/virtio/virtio.h"
typedef struct {
- HostMem hostmem; /* guest memory mapper */
+ MemoryRegion *mr; /* memory region containing the vring */
struct vring vr; /* virtqueue vring mapped to host memory */
uint16_t last_avail_idx; /* last processed avail ring index */
uint16_t last_used_idx; /* last processed used ring index */
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
` (7 preceding siblings ...)
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 8/8] dataplane: replace hostmem with memory_region_find Paolo Bonzini
@ 2013-05-08 6:20 ` liu ping fan
2013-05-08 15:44 ` Paolo Bonzini
2013-05-08 9:18 ` Stefan Hajnoczi
2013-05-08 9:25 ` Stefan Hajnoczi
10 siblings, 1 reply; 16+ messages in thread
From: liu ping fan @ 2013-05-08 6:20 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: jan.kiszka, qemu-devel, stefanha
On Mon, May 6, 2013 at 10:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Hi,
>
> this is an alternative approach to refactoring of dataplane's HostMem
> code. Here, I take Ping Fan's idea of RCU-style updating of the
> region list and apply it to the AddressSpace's FlatView. With this
In fact, I am worrying about the priority of MemoryListener, if it is
true, then we should drop RCU-style idea. Also if it is true, there is
already a bug with hostmem listener. It should use region_del, not
region_nop to reconstruct the local view. But just let me have a deep
thinking.
Regards,
Pingfan
> change, dataplane can simply use memory_region_find instead of
> hostmem.
>
> This is a somewhat larger change, but I prefer it for two reasons.
>
> 1) it splits the task of adding BQL-less memory dispatch in two parts,
> tacking memory_region_find first (which is simpler because locking
> is left to the caller).
>
> 2) HostMem duplicates a lot of the FlatView logic, and adding the
> RCU-style update in FlatView benefits everyone.
>
> The missing ingredients here are:
>
> 1) remember and unreference the MemoryRegions that are used in
> a vring entry. In order to implement this, it is probably simpler
> to change vring.c to use virtio.c's VirtQueueElement data structure.
> We want something like that anyway in order to support migration.
>
> 2) add an owner field to MemoryRegion, and set it for all MemoryRegions
> for hot-unpluggable devices. In this series, ref/unref are stubs.
>
> For simplicity I based the patches on my IOMMU rebase. I placed the
> tree at git://github.com/bonzini/qemu.git, branch iommu.
>
> Paolo
>
> Paolo Bonzini (8):
> memory: add ref/unref 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
> memory: access FlatView from a local variable
> memory: use a new FlatView pointer on every topology update
> memory: add reference counting to FlatView
> dataplane: replace hostmem with memory_region_find
>
> exec.c | 63 +++++++++---
> hw/core/loader.c | 1 +
> hw/display/exynos4210_fimd.c | 6 +
> hw/display/framebuffer.c | 10 +-
> hw/i386/kvm/ioapic.c | 2 +
> hw/i386/kvmvapic.c | 1 +
> hw/misc/vfio.c | 2 +
> hw/virtio/dataplane/Makefile.objs | 2 +-
> hw/virtio/dataplane/hostmem.c | 176 ---------------------------------
> hw/virtio/dataplane/vring.c | 56 +++++++++--
> 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/memory.h | 9 ++
> include/hw/virtio/dataplane/hostmem.h | 57 -----------
> include/hw/virtio/dataplane/vring.h | 3 +-
> kvm-all.c | 2 +
> memory.c | 142 +++++++++++++++++++++-----
> target-arm/kvm.c | 2 +
> target-i386/kvm.c | 4 +-
> target-sparc/mmu_helper.c | 1 +
> xen-all.c | 2 +
> 23 files changed, 253 insertions(+), 297 deletions(-)
> delete mode 100644 hw/virtio/dataplane/hostmem.c
> delete mode 100644 include/hw/virtio/dataplane/hostmem.h
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
` (8 preceding siblings ...)
2013-05-08 6:20 ` [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, " liu ping fan
@ 2013-05-08 9:18 ` Stefan Hajnoczi
2013-05-08 9:25 ` Stefan Hajnoczi
10 siblings, 0 replies; 16+ messages in thread
From: Stefan Hajnoczi @ 2013-05-08 9:18 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: jan.kiszka, qemu-devel, qemulist
On Mon, May 06, 2013 at 04:25:13PM +0200, Paolo Bonzini wrote:
> this is an alternative approach to refactoring of dataplane's HostMem
> code. Here, I take Ping Fan's idea of RCU-style updating of the
> region list and apply it to the AddressSpace's FlatView. With this
> change, dataplane can simply use memory_region_find instead of
> hostmem.
>
> This is a somewhat larger change, but I prefer it for two reasons.
>
> 1) it splits the task of adding BQL-less memory dispatch in two parts,
> tacking memory_region_find first (which is simpler because locking
> is left to the caller).
>
> 2) HostMem duplicates a lot of the FlatView logic, and adding the
> RCU-style update in FlatView benefits everyone.
>
> The missing ingredients here are:
>
> 1) remember and unreference the MemoryRegions that are used in
> a vring entry. In order to implement this, it is probably simpler
> to change vring.c to use virtio.c's VirtQueueElement data structure.
> We want something like that anyway in order to support migration.
Agreed. I want to drop vring.c and have virtio.c use thread-safe APIs
so it can be used from dataplane. VirtQueueElement can hide the
MemoryRegion reference so the virtio device caller (net, block, etc)
doesn't need to juggle references manually.
> 2) add an owner field to MemoryRegion, and set it for all MemoryRegions
> for hot-unpluggable devices. In this series, ref/unref are stubs.
>
> For simplicity I based the patches on my IOMMU rebase. I placed the
> tree at git://github.com/bonzini/qemu.git, branch iommu.
Are you hoping that Ping Fan will pick up this RFC or will you push it
yourself?
Either way, I'm very interested in a thread-safe memory API.
Stefan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
` (9 preceding siblings ...)
2013-05-08 9:18 ` Stefan Hajnoczi
@ 2013-05-08 9:25 ` Stefan Hajnoczi
10 siblings, 0 replies; 16+ messages in thread
From: Stefan Hajnoczi @ 2013-05-08 9:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: jan.kiszka, qemu-devel, qemulist
On Mon, May 06, 2013 at 04:25:13PM +0200, Paolo Bonzini wrote:
> this is an alternative approach to refactoring of dataplane's HostMem
> code. Here, I take Ping Fan's idea of RCU-style updating of the
> region list and apply it to the AddressSpace's FlatView. With this
> change, dataplane can simply use memory_region_find instead of
> hostmem.
Getting rid of hostmem is nice since we nearly have the necessary
infrastructure in the core memory API already.
Stefan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find
2013-05-08 6:20 ` [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, " liu ping fan
@ 2013-05-08 15:44 ` Paolo Bonzini
2013-05-09 0:53 ` liu ping fan
0 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-08 15:44 UTC (permalink / raw)
To: liu ping fan; +Cc: jan.kiszka, qemu-devel, stefanha
Il 08/05/2013 08:20, liu ping fan ha scritto:
> On Mon, May 6, 2013 at 10:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Hi,
>>
>> this is an alternative approach to refactoring of dataplane's HostMem
>> code. Here, I take Ping Fan's idea of RCU-style updating of the
>> region list and apply it to the AddressSpace's FlatView. With this
>
> In fact, I am worrying about the priority of MemoryListener, if it is
> true, then we should drop RCU-style idea.
You mean in hostmem, or in general as in this patch? Note that this
patch releases the old FlatView at the end of all MemoryListener operations.
Paolo
> Also if it is true, there is
> already a bug with hostmem listener. It should use region_del, not
> region_nop to reconstruct the local view. But just let me have a deep
> thinking.
>
> Regards,
> Pingfan
>> change, dataplane can simply use memory_region_find instead of
>> hostmem.
>>
>> This is a somewhat larger change, but I prefer it for two reasons.
>>
>> 1) it splits the task of adding BQL-less memory dispatch in two parts,
>> tacking memory_region_find first (which is simpler because locking
>> is left to the caller).
>>
>> 2) HostMem duplicates a lot of the FlatView logic, and adding the
>> RCU-style update in FlatView benefits everyone.
>>
>> The missing ingredients here are:
>>
>> 1) remember and unreference the MemoryRegions that are used in
>> a vring entry. In order to implement this, it is probably simpler
>> to change vring.c to use virtio.c's VirtQueueElement data structure.
>> We want something like that anyway in order to support migration.
>>
>> 2) add an owner field to MemoryRegion, and set it for all MemoryRegions
>> for hot-unpluggable devices. In this series, ref/unref are stubs.
>>
>> For simplicity I based the patches on my IOMMU rebase. I placed the
>> tree at git://github.com/bonzini/qemu.git, branch iommu.
>>
>> Paolo
>>
>> Paolo Bonzini (8):
>> memory: add ref/unref 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
>> memory: access FlatView from a local variable
>> memory: use a new FlatView pointer on every topology update
>> memory: add reference counting to FlatView
>> dataplane: replace hostmem with memory_region_find
>>
>> exec.c | 63 +++++++++---
>> hw/core/loader.c | 1 +
>> hw/display/exynos4210_fimd.c | 6 +
>> hw/display/framebuffer.c | 10 +-
>> hw/i386/kvm/ioapic.c | 2 +
>> hw/i386/kvmvapic.c | 1 +
>> hw/misc/vfio.c | 2 +
>> hw/virtio/dataplane/Makefile.objs | 2 +-
>> hw/virtio/dataplane/hostmem.c | 176 ---------------------------------
>> hw/virtio/dataplane/vring.c | 56 +++++++++--
>> 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/memory.h | 9 ++
>> include/hw/virtio/dataplane/hostmem.h | 57 -----------
>> include/hw/virtio/dataplane/vring.h | 3 +-
>> kvm-all.c | 2 +
>> memory.c | 142 +++++++++++++++++++++-----
>> target-arm/kvm.c | 2 +
>> target-i386/kvm.c | 4 +-
>> target-sparc/mmu_helper.c | 1 +
>> xen-all.c | 2 +
>> 23 files changed, 253 insertions(+), 297 deletions(-)
>> delete mode 100644 hw/virtio/dataplane/hostmem.c
>> delete mode 100644 include/hw/virtio/dataplane/hostmem.h
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find
2013-05-08 15:44 ` Paolo Bonzini
@ 2013-05-09 0:53 ` liu ping fan
2013-05-09 14:50 ` Paolo Bonzini
0 siblings, 1 reply; 16+ messages in thread
From: liu ping fan @ 2013-05-09 0:53 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: jan.kiszka, qemu-devel, stefanha
On Wed, May 8, 2013 at 11:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 08/05/2013 08:20, liu ping fan ha scritto:
>> On Mon, May 6, 2013 at 10:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Hi,
>>>
>>> this is an alternative approach to refactoring of dataplane's HostMem
>>> code. Here, I take Ping Fan's idea of RCU-style updating of the
>>> region list and apply it to the AddressSpace's FlatView. With this
>>
>> In fact, I am worrying about the priority of MemoryListener, if it is
>> true, then we should drop RCU-style idea.
>
> You mean in hostmem, or in general as in this patch? Note that this
> patch releases the old FlatView at the end of all MemoryListener operations.
>
Both in hostmem and this patch, they all broke the original design of
the MemoryListener, see notes for priority in code.
I have set out 2 patches to highlight this issue, and have CC you and Stefanha.
Regards,
Pingfan
> Paolo
>
>> Also if it is true, there is
>> already a bug with hostmem listener. It should use region_del, not
>> region_nop to reconstruct the local view. But just let me have a deep
>> thinking.
>>
>> Regards,
>> Pingfan
>>> change, dataplane can simply use memory_region_find instead of
>>> hostmem.
>>>
>>> This is a somewhat larger change, but I prefer it for two reasons.
>>>
>>> 1) it splits the task of adding BQL-less memory dispatch in two parts,
>>> tacking memory_region_find first (which is simpler because locking
>>> is left to the caller).
>>>
>>> 2) HostMem duplicates a lot of the FlatView logic, and adding the
>>> RCU-style update in FlatView benefits everyone.
>>>
>>> The missing ingredients here are:
>>>
>>> 1) remember and unreference the MemoryRegions that are used in
>>> a vring entry. In order to implement this, it is probably simpler
>>> to change vring.c to use virtio.c's VirtQueueElement data structure.
>>> We want something like that anyway in order to support migration.
>>>
>>> 2) add an owner field to MemoryRegion, and set it for all MemoryRegions
>>> for hot-unpluggable devices. In this series, ref/unref are stubs.
>>>
>>> For simplicity I based the patches on my IOMMU rebase. I placed the
>>> tree at git://github.com/bonzini/qemu.git, branch iommu.
>>>
>>> Paolo
>>>
>>> Paolo Bonzini (8):
>>> memory: add ref/unref 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
>>> memory: access FlatView from a local variable
>>> memory: use a new FlatView pointer on every topology update
>>> memory: add reference counting to FlatView
>>> dataplane: replace hostmem with memory_region_find
>>>
>>> exec.c | 63 +++++++++---
>>> hw/core/loader.c | 1 +
>>> hw/display/exynos4210_fimd.c | 6 +
>>> hw/display/framebuffer.c | 10 +-
>>> hw/i386/kvm/ioapic.c | 2 +
>>> hw/i386/kvmvapic.c | 1 +
>>> hw/misc/vfio.c | 2 +
>>> hw/virtio/dataplane/Makefile.objs | 2 +-
>>> hw/virtio/dataplane/hostmem.c | 176 ---------------------------------
>>> hw/virtio/dataplane/vring.c | 56 +++++++++--
>>> 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/memory.h | 9 ++
>>> include/hw/virtio/dataplane/hostmem.h | 57 -----------
>>> include/hw/virtio/dataplane/vring.h | 3 +-
>>> kvm-all.c | 2 +
>>> memory.c | 142 +++++++++++++++++++++-----
>>> target-arm/kvm.c | 2 +
>>> target-i386/kvm.c | 4 +-
>>> target-sparc/mmu_helper.c | 1 +
>>> xen-all.c | 2 +
>>> 23 files changed, 253 insertions(+), 297 deletions(-)
>>> delete mode 100644 hw/virtio/dataplane/hostmem.c
>>> delete mode 100644 include/hw/virtio/dataplane/hostmem.h
>>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find
2013-05-09 0:53 ` liu ping fan
@ 2013-05-09 14:50 ` Paolo Bonzini
2013-05-10 0:23 ` liu ping fan
0 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2013-05-09 14:50 UTC (permalink / raw)
To: liu ping fan; +Cc: jan.kiszka, qemu-devel, stefanha
Il 09/05/2013 02:53, liu ping fan ha scritto:
> On Wed, May 8, 2013 at 11:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 08/05/2013 08:20, liu ping fan ha scritto:
>>> On Mon, May 6, 2013 at 10:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> this is an alternative approach to refactoring of dataplane's HostMem
>>>> code. Here, I take Ping Fan's idea of RCU-style updating of the
>>>> region list and apply it to the AddressSpace's FlatView. With this
>>>
>>> In fact, I am worrying about the priority of MemoryListener, if it is
>>> true, then we should drop RCU-style idea.
>>
>> You mean in hostmem, or in general as in this patch? Note that this
>> patch releases the old FlatView at the end of all MemoryListener operations.
>>
> Both in hostmem and this patch, they all broke the original design of
> the MemoryListener, see notes for priority in code.
I think both hostmem and this patch are fine. The hypervisor is never
involved, all accesses go through the "old" FlatView and regions cannot
disappear thanks to ref/unref.
In fact, we need _more_ RCU-style updates, not less. For BQL-less
dispatch, address space mapping/translation can race against the
MemoryListeners in exec.c. To fix this, phys_sections and
AddressSpaceDispatch need to be reference counted and RCU-ified as well.
Paolo
> I have set out 2 patches to highlight this issue, and have CC you and Stefanha.
>
> Regards,
> Pingfan
>
>> Paolo
>>
>>> Also if it is true, there is
>>> already a bug with hostmem listener. It should use region_del, not
>>> region_nop to reconstruct the local view. But just let me have a deep
>>> thinking.
>>>
>>> Regards,
>>> Pingfan
>>>> change, dataplane can simply use memory_region_find instead of
>>>> hostmem.
>>>>
>>>> This is a somewhat larger change, but I prefer it for two reasons.
>>>>
>>>> 1) it splits the task of adding BQL-less memory dispatch in two parts,
>>>> tacking memory_region_find first (which is simpler because locking
>>>> is left to the caller).
>>>>
>>>> 2) HostMem duplicates a lot of the FlatView logic, and adding the
>>>> RCU-style update in FlatView benefits everyone.
>>>>
>>>> The missing ingredients here are:
>>>>
>>>> 1) remember and unreference the MemoryRegions that are used in
>>>> a vring entry. In order to implement this, it is probably simpler
>>>> to change vring.c to use virtio.c's VirtQueueElement data structure.
>>>> We want something like that anyway in order to support migration.
>>>>
>>>> 2) add an owner field to MemoryRegion, and set it for all MemoryRegions
>>>> for hot-unpluggable devices. In this series, ref/unref are stubs.
>>>>
>>>> For simplicity I based the patches on my IOMMU rebase. I placed the
>>>> tree at git://github.com/bonzini/qemu.git, branch iommu.
>>>>
>>>> Paolo
>>>>
>>>> Paolo Bonzini (8):
>>>> memory: add ref/unref 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
>>>> memory: access FlatView from a local variable
>>>> memory: use a new FlatView pointer on every topology update
>>>> memory: add reference counting to FlatView
>>>> dataplane: replace hostmem with memory_region_find
>>>>
>>>> exec.c | 63 +++++++++---
>>>> hw/core/loader.c | 1 +
>>>> hw/display/exynos4210_fimd.c | 6 +
>>>> hw/display/framebuffer.c | 10 +-
>>>> hw/i386/kvm/ioapic.c | 2 +
>>>> hw/i386/kvmvapic.c | 1 +
>>>> hw/misc/vfio.c | 2 +
>>>> hw/virtio/dataplane/Makefile.objs | 2 +-
>>>> hw/virtio/dataplane/hostmem.c | 176 ---------------------------------
>>>> hw/virtio/dataplane/vring.c | 56 +++++++++--
>>>> 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/memory.h | 9 ++
>>>> include/hw/virtio/dataplane/hostmem.h | 57 -----------
>>>> include/hw/virtio/dataplane/vring.h | 3 +-
>>>> kvm-all.c | 2 +
>>>> memory.c | 142 +++++++++++++++++++++-----
>>>> target-arm/kvm.c | 2 +
>>>> target-i386/kvm.c | 4 +-
>>>> target-sparc/mmu_helper.c | 1 +
>>>> xen-all.c | 2 +
>>>> 23 files changed, 253 insertions(+), 297 deletions(-)
>>>> delete mode 100644 hw/virtio/dataplane/hostmem.c
>>>> delete mode 100644 include/hw/virtio/dataplane/hostmem.h
>>>>
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find
2013-05-09 14:50 ` Paolo Bonzini
@ 2013-05-10 0:23 ` liu ping fan
0 siblings, 0 replies; 16+ messages in thread
From: liu ping fan @ 2013-05-10 0:23 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: jan.kiszka, qemu-devel, stefanha
On Thu, May 9, 2013 at 10:50 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/05/2013 02:53, liu ping fan ha scritto:
>> On Wed, May 8, 2013 at 11:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 08/05/2013 08:20, liu ping fan ha scritto:
>>>> On Mon, May 6, 2013 at 10:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> this is an alternative approach to refactoring of dataplane's HostMem
>>>>> code. Here, I take Ping Fan's idea of RCU-style updating of the
>>>>> region list and apply it to the AddressSpace's FlatView. With this
>>>>
>>>> In fact, I am worrying about the priority of MemoryListener, if it is
>>>> true, then we should drop RCU-style idea.
>>>
>>> You mean in hostmem, or in general as in this patch? Note that this
>>> patch releases the old FlatView at the end of all MemoryListener operations.
>>>
>> Both in hostmem and this patch, they all broke the original design of
>> the MemoryListener, see notes for priority in code.
>
> I think both hostmem and this patch are fine. The hypervisor is never
> involved, all accesses go through the "old" FlatView and regions cannot
> disappear thanks to ref/unref.
>
Here, we worry about add, not del.
> In fact, we need _more_ RCU-style updates, not less. For BQL-less
> dispatch, address space mapping/translation can race against the
> MemoryListeners in exec.c. To fix this, phys_sections and
> AddressSpaceDispatch need to be reference counted and RCU-ified as well.
>
Agree, I like RCU too.
> Paolo
>
>> I have set out 2 patches to highlight this issue, and have CC you and Stefanha.
>>
>> Regards,
>> Pingfan
>>
>>> Paolo
>>>
>>>> Also if it is true, there is
>>>> already a bug with hostmem listener. It should use region_del, not
>>>> region_nop to reconstruct the local view. But just let me have a deep
>>>> thinking.
>>>>
>>>> Regards,
>>>> Pingfan
>>>>> change, dataplane can simply use memory_region_find instead of
>>>>> hostmem.
>>>>>
>>>>> This is a somewhat larger change, but I prefer it for two reasons.
>>>>>
>>>>> 1) it splits the task of adding BQL-less memory dispatch in two parts,
>>>>> tacking memory_region_find first (which is simpler because locking
>>>>> is left to the caller).
>>>>>
>>>>> 2) HostMem duplicates a lot of the FlatView logic, and adding the
>>>>> RCU-style update in FlatView benefits everyone.
>>>>>
>>>>> The missing ingredients here are:
>>>>>
>>>>> 1) remember and unreference the MemoryRegions that are used in
>>>>> a vring entry. In order to implement this, it is probably simpler
>>>>> to change vring.c to use virtio.c's VirtQueueElement data structure.
>>>>> We want something like that anyway in order to support migration.
>>>>>
>>>>> 2) add an owner field to MemoryRegion, and set it for all MemoryRegions
>>>>> for hot-unpluggable devices. In this series, ref/unref are stubs.
>>>>>
>>>>> For simplicity I based the patches on my IOMMU rebase. I placed the
>>>>> tree at git://github.com/bonzini/qemu.git, branch iommu.
>>>>>
>>>>> Paolo
>>>>>
>>>>> Paolo Bonzini (8):
>>>>> memory: add ref/unref 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
>>>>> memory: access FlatView from a local variable
>>>>> memory: use a new FlatView pointer on every topology update
>>>>> memory: add reference counting to FlatView
>>>>> dataplane: replace hostmem with memory_region_find
>>>>>
>>>>> exec.c | 63 +++++++++---
>>>>> hw/core/loader.c | 1 +
>>>>> hw/display/exynos4210_fimd.c | 6 +
>>>>> hw/display/framebuffer.c | 10 +-
>>>>> hw/i386/kvm/ioapic.c | 2 +
>>>>> hw/i386/kvmvapic.c | 1 +
>>>>> hw/misc/vfio.c | 2 +
>>>>> hw/virtio/dataplane/Makefile.objs | 2 +-
>>>>> hw/virtio/dataplane/hostmem.c | 176 ---------------------------------
>>>>> hw/virtio/dataplane/vring.c | 56 +++++++++--
>>>>> 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/memory.h | 9 ++
>>>>> include/hw/virtio/dataplane/hostmem.h | 57 -----------
>>>>> include/hw/virtio/dataplane/vring.h | 3 +-
>>>>> kvm-all.c | 2 +
>>>>> memory.c | 142 +++++++++++++++++++++-----
>>>>> target-arm/kvm.c | 2 +
>>>>> target-i386/kvm.c | 4 +-
>>>>> target-sparc/mmu_helper.c | 1 +
>>>>> xen-all.c | 2 +
>>>>> 23 files changed, 253 insertions(+), 297 deletions(-)
>>>>> delete mode 100644 hw/virtio/dataplane/hostmem.c
>>>>> delete mode 100644 include/hw/virtio/dataplane/hostmem.h
>>>>>
>>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-05-10 0:23 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-06 14:25 [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 1/8] memory: add ref/unref calls Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 2/8] exec: check MRU in qemu_ram_addr_from_host Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 3/8] memory: return MemoryRegion from qemu_ram_addr_from_host Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 4/8] memory: ref/unref memory across address_space_map/unmap Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 5/8] memory: access FlatView from a local variable Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 6/8] memory: use a new FlatView pointer on every topology update Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 7/8] memory: add reference counting to FlatView Paolo Bonzini
2013-05-06 14:25 ` [Qemu-devel] [RFC PATCH 8/8] dataplane: replace hostmem with memory_region_find Paolo Bonzini
2013-05-08 6:20 ` [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, " liu ping fan
2013-05-08 15:44 ` Paolo Bonzini
2013-05-09 0:53 ` liu ping fan
2013-05-09 14:50 ` Paolo Bonzini
2013-05-10 0:23 ` liu ping fan
2013-05-08 9:18 ` Stefan Hajnoczi
2013-05-08 9:25 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).