[Qemu-devel] [PULL 05/18] dataplane: replace hostmem with memory_region_find

[Stefan Hajnoczi]

From: Paolo Bonzini <address@hidden>

Signed-off-by: Paolo Bonzini <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>
---
 hw/virtio/dataplane/Makefile.objs     |   2 +-
 hw/virtio/dataplane/hostmem.c         | 183 ----------------------------------
 hw/virtio/dataplane/vring.c           |  78 +++++++++++++--
 include/hw/virtio/dataplane/hostmem.h |  58 -----------
 include/hw/virtio/dataplane/vring.h   |   3 +-
 5 files changed, 72 insertions(+), 252 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 901d98b..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 <address@hidden>
- *
- * 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 = 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,
-                                           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 1bf9dd3..250d45e 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -15,9 +15,53 @@
  */
 
 #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"
 
+/* vring_map can be coupled with vring_unmap or (if you still have the
+ * value returned in *mr) memory_region_unref.
+ */
+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 || int128_get64(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;
+}
+
+static void vring_unmap(void *buffer, bool is_write)
+{
+    ram_addr_t addr;
+    MemoryRegion *mr;
+
+    mr = qemu_ram_addr_from_host(buffer, &addr);
+    memory_region_unref(mr);
+}
+
 /* Map the guest's vring to host memory */
 bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
 {
@@ -27,8 +71,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,
@@ -54,7 +97,7 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
     virtio_queue_set_last_avail_idx(vdev, n, vring->last_avail_idx);
     virtio_queue_invalidate_signalled_used(vdev, n);
 
-    hostmem_finalize(&vring->hostmem);
+    memory_region_unref(vring->mr);
 }
 
 /* Disable guest->host notifies */
@@ -117,6 +160,7 @@ static int get_desc(Vring *vring, VirtQueueElement *elem,
     unsigned *num;
     struct iovec *iov;
     hwaddr *addr;
+    MemoryRegion *mr;
 
     if (desc->flags & VRING_DESC_F_WRITE) {
         num = &elem->in_num;
@@ -141,14 +185,16 @@ static int get_desc(Vring *vring, VirtQueueElement *elem,
     }
 
     /* 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);
     if (!iov->iov_base) {
         error_report("Failed to map descriptor addr %#" PRIx64 " len %u",
                      (uint64_t)desc->addr, desc->len);
         return -EFAULT;
     }
 
+    /* The MemoryRegion is looked up again and unref'ed later, leave the
+     * ref in place.  */
     iov->iov_len = desc->len;
     *addr = desc->addr;
     *num += 1;
@@ -183,11 +229,12 @@ static int get_indirect(Vring *vring, VirtQueueElement 
*elem,
 
     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);
         if (!desc_ptr) {
             error_report("Failed to map indirect descriptor "
                          "addr %#" PRIx64 " len %zu",
@@ -197,6 +244,7 @@ static int get_indirect(Vring *vring, VirtQueueElement 
*elem,
             return -EFAULT;
         }
         desc = *desc_ptr;
+        memory_region_unref(mr);
 
         /* Ensure descriptor has been loaded before accessing fields */
         barrier(); /* read_barrier_depends(); */
@@ -226,6 +274,20 @@ static int get_indirect(Vring *vring, VirtQueueElement 
*elem,
 
 void vring_free_element(VirtQueueElement *elem)
 {
+    int i;
+
+    /* This assumes that the iovecs, if changed, are never moved past
+     * the end of the valid area.  This is true if iovec manipulations
+     * are done with iov_discard_front and iov_discard_back.
+     */
+    for (i = 0; i < elem->out_num; i++) {
+        vring_unmap(elem->out_sg[i].iov_base, false);
+    }
+
+    for (i = 0; i < elem->in_num; i++) {
+        vring_unmap(elem->in_sg[i].iov_base, true);
+    }
+
     g_slice_free(VirtQueueElement, elem);
 }
 
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 <address@hidden>
- *
- * 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 3d6c0df..63e7bf4 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.8.4.2