Re: [Qemu-devel] [PATCH v3 03/12] dataplane: add host memory mapping code

[Stefan Hajnoczi <stefanha@redhat.com>]

On Mon, Nov 26, 2012 at 09:46:11AM -0500, Don Koch wrote:
> 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 <stefanha@redhat.com>
> > ---
> >  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 <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-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.

Cleared it up in the previous version email thread.

For reference to others reading this, it's because even unchanged
("nop") regions must be added to our region list.  We're rebuilding the
list from scratch on each MemoryListener transaction (begin/commit) and
therefore *do* care about unchanged regions.

Stefan