From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58373) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tczwy-0007rN-4y for qemu-devel@nongnu.org; Mon, 26 Nov 2012 09:46:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tczws-0007JU-Sv for qemu-devel@nongnu.org; Mon, 26 Nov 2012 09:46:20 -0500 Received: from hub021-nj-6.exch021.serverdata.net ([206.225.164.222]:10244) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tczws-0007JH-O5 for qemu-devel@nongnu.org; Mon, 26 Nov 2012 09:46:14 -0500 Message-ID: <50B380B3.8080905@cloudswitch.com> Date: Mon, 26 Nov 2012 09:46:11 -0500 From: Don Koch MIME-Version: 1.0 References: <1353522781-12721-1-git-send-email-stefanha@redhat.com> <1353522781-12721-4-git-send-email-stefanha@redhat.com> In-Reply-To: <1353522781-12721-4-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 03/12] dataplane: add host memory mapping code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Anthony Liguori , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Michael Roth , Blue Swirl , khoa@us.ibm.com, Paolo Bonzini , Asias He On 11/21/2012 01:32 PM, Stefan Hajnoczi wrote: > The data plane thread needs to map guest physical addresses to host > pointers. Normally this is done with cpu_physical_memory_map() but the > function assumes the global mutex is held. The data plane thread does > not touch the global mutex and therefore needs a thread-safe memory > mapping mechanism. > > Hostmem registers a MemoryListener similar to how vhost collects and > pushes memory region information into the kernel. There is a > fine-grained lock on the regions list which is held during lookup and > when installing a new regions list. > > When the physical memory map changes the MemoryListener callbacks are > invoked. They build up a new list of memory regions which is finally > installed when the list has been completed. > > Note that this approach is not safe across memory hotplug because mapped > pointers may still be in used across memory unplug. However, this is > currently a problem for QEMU in general and needs to be addressed in the > future. > > Signed-off-by: Stefan Hajnoczi > --- > hw/dataplane/Makefile.objs | 3 + > hw/dataplane/hostmem.c | 165 +++++++++++++++++++++++++++++++++++++++++++++ > hw/dataplane/hostmem.h | 52 ++++++++++++++ > 3 files changed, 220 insertions(+) > create mode 100644 hw/dataplane/Makefile.objs > create mode 100644 hw/dataplane/hostmem.c > create mode 100644 hw/dataplane/hostmem.h > > diff --git a/hw/dataplane/Makefile.objs b/hw/dataplane/Makefile.objs > new file mode 100644 > index 0000000..8c8dea1 > --- /dev/null > +++ b/hw/dataplane/Makefile.objs > @@ -0,0 +1,3 @@ > +ifeq ($(CONFIG_VIRTIO), y) > +common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += hostmem.o > +endif > diff --git a/hw/dataplane/hostmem.c b/hw/dataplane/hostmem.c > new file mode 100644 > index 0000000..48aabf0 > --- /dev/null > +++ b/hw/dataplane/hostmem.c > @@ -0,0 +1,165 @@ > +/* > + * Thread-safe guest to host memory mapping > + * > + * Copyright 2012 Red Hat, Inc. and/or its affiliates > + * > + * Authors: > + * Stefan Hajnoczi > + * > + * 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-memory.h" > +#include "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 (offset_within_region + len <= region->size) { > + 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); > + > + qemu_mutex_lock(&hostmem->current_regions_lock); > + 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, > + }; > + hostmem->num_new_regions++; > +} > + > +static void hostmem_listener_append_region(MemoryListener *listener, > + MemoryRegionSection *section) > +{ > + Hostmem *hostmem = container_of(listener, Hostmem, listener); > + > + if (memory_region_is_ram(section->mr)) { > + 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)); > + > + 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, OK, more explicitly, I find it odd that append_region is a nop (unless there's a new meaning of "nop" I am unaware of). I would have expected the section_dummy function. > + .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, > + }; > -d