Patchwork [2/5] hostmem: make hostmem global and RAM hotunplg safe

login
register
mail settings
Submitter pingfan liu
Date April 1, 2013, 8:20 a.m.
Message ID <1364804434-7980-3-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/232676/
State New
Headers show

Comments

pingfan liu - April 1, 2013, 8:20 a.m.
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

The hostmem listener will translate gpa to hva quickly. Make it
global, so other component can use it.
Also this patch adopt MemoryRegionOps's ref/unref interface to
make it survive the RAM hotunplug.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/dataplane/hostmem.c |  130 +++++++++++++++++++++++++++++++++--------------
 hw/dataplane/hostmem.h |   19 ++------
 2 files changed, 95 insertions(+), 54 deletions(-)
liguang - April 2, 2013, 6:11 a.m.
在 2013-04-01一的 16:20 +0800,Liu Ping Fan写道:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> The hostmem listener will translate gpa to hva quickly. Make it
> global, so other component can use it.
> Also this patch adopt MemoryRegionOps's ref/unref interface to
> make it survive the RAM hotunplug.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/dataplane/hostmem.c |  130 +++++++++++++++++++++++++++++++++--------------
>  hw/dataplane/hostmem.h |   19 ++------
>  2 files changed, 95 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/dataplane/hostmem.c b/hw/dataplane/hostmem.c
> index 380537e..86c02cd 100644
> --- a/hw/dataplane/hostmem.c
> +++ b/hw/dataplane/hostmem.c
> @@ -13,6 +13,12 @@
>  
>  #include "exec/address-spaces.h"
>  #include "hostmem.h"
> +#include "qemu/main-loop.h"
> +
> +/* lock to protect the access to cur_hostmem */
> +static QemuMutex hostmem_lock;
> +static HostMem *cur_hostmem;
> +static HostMem *next_hostmem;
>  
>  static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>  {
> @@ -28,16 +34,49 @@ static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>      }
>  }
>  
> +static void hostmem_ref(HostMem *hostmem)
> +{
> +    assert(__sync_add_and_fetch(&hostmem->ref, 1) > 0);
> +}
> +
> +void hostmem_unref(HostMem *hostmem)
> +{
> +    int i;
> +    HostMemRegion *hmr;
> +
> +    if (!__sync_sub_and_fetch(&hostmem->ref, 1)) {

maybe '__sync_sub_and_fetch(&hostmem->ref, 1) > 0' is more safer

> +        for (i = 0; i < hostmem->num_current_regions; i++) {
> +            hmr = &hostmem->current_regions[i];
> +            /* Fix me, when memory hotunplug implement
> +             * assert(hmr->mr_ops->unref)
> +             */
> +            if (hmr->mr->ops && hmr->mr->ops->unref) {
> +                hmr->mr->ops->unref();
> +            }
> +        }
> +        g_free(hostmem->current_regions);
> +        g_free(hostmem);
> +    }
> +}
> +
>  /**
>   * Map guest physical address to host pointer
> + * also inc refcnt of *mr, caller need to unref later
>   */
> -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write)
>  {
>      HostMemRegion *region;
>      void *host_addr = NULL;
>      hwaddr offset_within_region;
> +    HostMem *hostmem;
> +
> +    assert(mr);
> +    *mr = NULL;
> +    qemu_mutex_lock(&hostmem_lock);
> +    hostmem = cur_hostmem;
> +    hostmem_ref(hostmem);
> +    qemu_mutex_unlock(&hostmem_lock);
>  
> -    qemu_mutex_lock(&hostmem->current_regions_lock);
>      region = bsearch(&phys, hostmem->current_regions,
>                       hostmem->num_current_regions,
>                       sizeof(hostmem->current_regions[0]),
> @@ -52,28 +91,33 @@ void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
>      if (len <= region->size - offset_within_region) {
>          host_addr = region->host_addr + offset_within_region;
>      }
> -out:
> -    qemu_mutex_unlock(&hostmem->current_regions_lock);
> +    *mr = region->mr;
> +    memory_region_ref(*mr);
>  
> +out:
> +    hostmem_unref(hostmem);
>      return host_addr;
>  }
>  
> +static void hostmem_listener_begin(MemoryListener *listener)
> +{
> +    next_hostmem = g_new0(HostMem, 1);
> +    next_hostmem->ref = 1;
> +}
> +
>  /**
>   * Install new regions list
>   */
>  static void hostmem_listener_commit(MemoryListener *listener)
>  {
> -    HostMem *hostmem = container_of(listener, HostMem, listener);
> +    HostMem *tmp;
>  
> -    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);
> +    tmp = cur_hostmem;
> +    qemu_mutex_lock(&hostmem_lock);
> +    cur_hostmem = next_hostmem;
> +    qemu_mutex_unlock(&hostmem_lock);
> +    hostmem_unref(tmp);
>  
> -    /* Reset new regions list */
> -    hostmem->new_regions = NULL;
> -    hostmem->num_new_regions = 0;
>  }
>  
>  /**
> @@ -83,23 +127,24 @@ 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]);
> +    size_t num = hostmem->num_current_regions;
> +    size_t new_size = (num + 1) * sizeof(hostmem->current_regions[0]);
>  
> -    hostmem->new_regions = g_realloc(hostmem->new_regions, new_size);
> -    hostmem->new_regions[num] = (HostMemRegion){
> +    hostmem->current_regions = g_realloc(hostmem->current_regions, new_size);
> +    hostmem->current_regions[num] = (HostMemRegion){
>          .host_addr = ram_ptr + section->offset_within_region,
>          .guest_addr = section->offset_within_address_space,
> +        .mr = section->mr,
>          .size = section->size,
>          .readonly = section->readonly,
>      };
> -    hostmem->num_new_regions++;
> +    hostmem->num_current_regions++;
>  }
>  
> -static void hostmem_listener_append_region(MemoryListener *listener,
> +static void hostmem_listener_nop_region(MemoryListener *listener,
>                                             MemoryRegionSection *section)
>  {
> -    HostMem *hostmem = container_of(listener, HostMem, listener);
> +    HostMem *hostmem = next_hostmem;
>  
>      /* Ignore non-RAM regions, we may not be able to map them */
>      if (!memory_region_is_ram(section->mr)) {
> @@ -114,6 +159,18 @@ static void hostmem_listener_append_region(MemoryListener *listener,
>      hostmem_append_new_region(hostmem, section);
>  }
>  
> +static void hostmem_listener_append_region(MemoryListener *listener,
> +                                           MemoryRegionSection *section)
> +{
> +    hostmem_listener_nop_region(listener, section);
> +    /* Fix me, when memory hotunplug implement
> +     * assert(section->mr->ops->ref)
> +     */
> +    if (section->mr->ops && section->mr->ops->ref) {
> +        section->mr->ops->ref();
> +    }
> +}
> +
>  /* We don't implement most MemoryListener callbacks, use these nop stubs */
>  static void hostmem_listener_dummy(MemoryListener *listener)
>  {
> @@ -137,18 +194,12 @@ static void hostmem_listener_coalesced_mmio_dummy(MemoryListener *listener,
>  {
>  }
>  
> -void hostmem_init(HostMem *hostmem)
> -{
> -    memset(hostmem, 0, sizeof(*hostmem));
> -
> -    qemu_mutex_init(&hostmem->current_regions_lock);
> -
> -    hostmem->listener = (MemoryListener){
> -        .begin = hostmem_listener_dummy,
> +static MemoryListener hostmem_listener = {
> +        .begin = hostmem_listener_begin,
>          .commit = hostmem_listener_commit,
>          .region_add = hostmem_listener_append_region,
>          .region_del = hostmem_listener_section_dummy,
> -        .region_nop = hostmem_listener_append_region,
> +        .region_nop = hostmem_listener_nop_region,
>          .log_start = hostmem_listener_section_dummy,
>          .log_stop = hostmem_listener_section_dummy,
>          .log_sync = hostmem_listener_section_dummy,
> @@ -159,18 +210,19 @@ void hostmem_init(HostMem *hostmem)
>          .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_init(void)
> +{
> +    qemu_mutex_init(&hostmem_lock);
> +    cur_hostmem = g_new0(HostMem, 1);
> +    cur_hostmem->ref = 1;
> +    memory_listener_register(&hostmem_listener, &address_space_memory);
>  }
>  
> -void hostmem_finalize(HostMem *hostmem)
> +void hostmem_finalize(void)
>  {
> -    memory_listener_unregister(&hostmem->listener);
> -    g_free(hostmem->new_regions);
> -    g_free(hostmem->current_regions);
> -    qemu_mutex_destroy(&hostmem->current_regions_lock);
> +    memory_listener_unregister(&hostmem_listener);
> +    hostmem_unref(cur_hostmem);
> +    qemu_mutex_destroy(&hostmem_lock);
>  }
> diff --git a/hw/dataplane/hostmem.h b/hw/dataplane/hostmem.h
> index b2cf093..883ba74 100644
> --- a/hw/dataplane/hostmem.h
> +++ b/hw/dataplane/hostmem.h
> @@ -20,29 +20,18 @@
>  typedef struct {
>      void *host_addr;
>      hwaddr guest_addr;
> +    MemoryRegion *mr;
>      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;
> +    int ref;
>      HostMemRegion *current_regions;
>      size_t num_current_regions;
>  } HostMem;
>  
> -void hostmem_init(HostMem *hostmem);
> -void hostmem_finalize(HostMem *hostmem);
> +void hostmem_unref(HostMem *hostmem);
>  
>  /**
>   * Map a guest physical address to a pointer
> @@ -52,6 +41,6 @@ void hostmem_finalize(HostMem *hostmem);
>   * 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);
> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write);
>  
>  #endif /* HOSTMEM_H */
Paolo Bonzini - April 11, 2013, 10:09 a.m.
Il 01/04/2013 10:20, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> The hostmem listener will translate gpa to hva quickly. Make it
> global, so other component can use it.
> Also this patch adopt MemoryRegionOps's ref/unref interface to
> make it survive the RAM hotunplug.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/dataplane/hostmem.c |  130 +++++++++++++++++++++++++++++++++--------------
>  hw/dataplane/hostmem.h |   19 ++------
>  2 files changed, 95 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/dataplane/hostmem.c b/hw/dataplane/hostmem.c
> index 380537e..86c02cd 100644
> --- a/hw/dataplane/hostmem.c
> +++ b/hw/dataplane/hostmem.c
> @@ -13,6 +13,12 @@
>  
>  #include "exec/address-spaces.h"
>  #include "hostmem.h"
> +#include "qemu/main-loop.h"
> +
> +/* lock to protect the access to cur_hostmem */
> +static QemuMutex hostmem_lock;
> +static HostMem *cur_hostmem;
> +static HostMem *next_hostmem;
>  
>  static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>  {
> @@ -28,16 +34,49 @@ static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>      }
>  }
>  
> +static void hostmem_ref(HostMem *hostmem)
> +{
> +    assert(__sync_add_and_fetch(&hostmem->ref, 1) > 0);
> +}
> +
> +void hostmem_unref(HostMem *hostmem)
> +{
> +    int i;
> +    HostMemRegion *hmr;
> +
> +    if (!__sync_sub_and_fetch(&hostmem->ref, 1)) {
> +        for (i = 0; i < hostmem->num_current_regions; i++) {
> +            hmr = &hostmem->current_regions[i];
> +            /* Fix me, when memory hotunplug implement
> +             * assert(hmr->mr_ops->unref)
> +             */
> +            if (hmr->mr->ops && hmr->mr->ops->unref) {
> +                hmr->mr->ops->unref();
> +            }
> +        }
> +        g_free(hostmem->current_regions);
> +        g_free(hostmem);
> +    }
> +}
> +
>  /**
>   * Map guest physical address to host pointer
> + * also inc refcnt of *mr, caller need to unref later
>   */
> -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write)
>  {
>      HostMemRegion *region;
>      void *host_addr = NULL;
>      hwaddr offset_within_region;
> +    HostMem *hostmem;
> +
> +    assert(mr);
> +    *mr = NULL;

I think it's simpler if you allow a NULL MemoryRegion.

Also, it is better if you keep the HostMem type, because in principle
different HostMems could be used for different AddressSpaces.
Basically, what is now HostMem would become HostMemRegions, and the new
HostMem type would have these fields:

   QemuMutex         hostmem_lock;
   HostMemRegions   *cur_regions;
   HostMemRegions   *next_regions;

matching your three globals.  I like the rcu-ready design.

Finally, please split this patch and patch 3 differently:

1) add HostMemRegions

2) add ref/unref of memory regions to hostmem

3) add MemoryRegion argument to hostmem_lookup, leave it NULL in vring

4) add ref/unref of memory regions to vring

Paolo

> +    qemu_mutex_lock(&hostmem_lock);
> +    hostmem = cur_hostmem;
> +    hostmem_ref(hostmem);
> +    qemu_mutex_unlock(&hostmem_lock);
>  
> -    qemu_mutex_lock(&hostmem->current_regions_lock);
>      region = bsearch(&phys, hostmem->current_regions,
>                       hostmem->num_current_regions,
>                       sizeof(hostmem->current_regions[0]),
> @@ -52,28 +91,33 @@ void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
>      if (len <= region->size - offset_within_region) {
>          host_addr = region->host_addr + offset_within_region;
>      }
> -out:
> -    qemu_mutex_unlock(&hostmem->current_regions_lock);
> +    *mr = region->mr;
> +    memory_region_ref(*mr);
>  
> +out:
> +    hostmem_unref(hostmem);
>      return host_addr;
>  }
>  
> +static void hostmem_listener_begin(MemoryListener *listener)
> +{
> +    next_hostmem = g_new0(HostMem, 1);
> +    next_hostmem->ref = 1;
> +}
> +
>  /**
>   * Install new regions list
>   */
>  static void hostmem_listener_commit(MemoryListener *listener)
>  {
> -    HostMem *hostmem = container_of(listener, HostMem, listener);
> +    HostMem *tmp;
>  
> -    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);
> +    tmp = cur_hostmem;
> +    qemu_mutex_lock(&hostmem_lock);
> +    cur_hostmem = next_hostmem;
> +    qemu_mutex_unlock(&hostmem_lock);
> +    hostmem_unref(tmp);
>  
> -    /* Reset new regions list */
> -    hostmem->new_regions = NULL;
> -    hostmem->num_new_regions = 0;
>  }
>  
>  /**
> @@ -83,23 +127,24 @@ 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]);
> +    size_t num = hostmem->num_current_regions;
> +    size_t new_size = (num + 1) * sizeof(hostmem->current_regions[0]);
>  
> -    hostmem->new_regions = g_realloc(hostmem->new_regions, new_size);
> -    hostmem->new_regions[num] = (HostMemRegion){
> +    hostmem->current_regions = g_realloc(hostmem->current_regions, new_size);
> +    hostmem->current_regions[num] = (HostMemRegion){
>          .host_addr = ram_ptr + section->offset_within_region,
>          .guest_addr = section->offset_within_address_space,
> +        .mr = section->mr,
>          .size = section->size,
>          .readonly = section->readonly,
>      };
> -    hostmem->num_new_regions++;
> +    hostmem->num_current_regions++;
>  }
>  
> -static void hostmem_listener_append_region(MemoryListener *listener,
> +static void hostmem_listener_nop_region(MemoryListener *listener,
>                                             MemoryRegionSection *section)
>  {
> -    HostMem *hostmem = container_of(listener, HostMem, listener);
> +    HostMem *hostmem = next_hostmem;
>  
>      /* Ignore non-RAM regions, we may not be able to map them */
>      if (!memory_region_is_ram(section->mr)) {
> @@ -114,6 +159,18 @@ static void hostmem_listener_append_region(MemoryListener *listener,
>      hostmem_append_new_region(hostmem, section);
>  }
>  
> +static void hostmem_listener_append_region(MemoryListener *listener,
> +                                           MemoryRegionSection *section)
> +{
> +    hostmem_listener_nop_region(listener, section);
> +    /* Fix me, when memory hotunplug implement
> +     * assert(section->mr->ops->ref)
> +     */
> +    if (section->mr->ops && section->mr->ops->ref) {
> +        section->mr->ops->ref();
> +    }
> +}
> +
>  /* We don't implement most MemoryListener callbacks, use these nop stubs */
>  static void hostmem_listener_dummy(MemoryListener *listener)
>  {
> @@ -137,18 +194,12 @@ static void hostmem_listener_coalesced_mmio_dummy(MemoryListener *listener,
>  {
>  }
>  
> -void hostmem_init(HostMem *hostmem)
> -{
> -    memset(hostmem, 0, sizeof(*hostmem));
> -
> -    qemu_mutex_init(&hostmem->current_regions_lock);
> -
> -    hostmem->listener = (MemoryListener){
> -        .begin = hostmem_listener_dummy,
> +static MemoryListener hostmem_listener = {
> +        .begin = hostmem_listener_begin,
>          .commit = hostmem_listener_commit,
>          .region_add = hostmem_listener_append_region,
>          .region_del = hostmem_listener_section_dummy,
> -        .region_nop = hostmem_listener_append_region,
> +        .region_nop = hostmem_listener_nop_region,
>          .log_start = hostmem_listener_section_dummy,
>          .log_stop = hostmem_listener_section_dummy,
>          .log_sync = hostmem_listener_section_dummy,
> @@ -159,18 +210,19 @@ void hostmem_init(HostMem *hostmem)
>          .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_init(void)
> +{
> +    qemu_mutex_init(&hostmem_lock);
> +    cur_hostmem = g_new0(HostMem, 1);
> +    cur_hostmem->ref = 1;
> +    memory_listener_register(&hostmem_listener, &address_space_memory);
>  }
>  
> -void hostmem_finalize(HostMem *hostmem)
> +void hostmem_finalize(void)
>  {
> -    memory_listener_unregister(&hostmem->listener);
> -    g_free(hostmem->new_regions);
> -    g_free(hostmem->current_regions);
> -    qemu_mutex_destroy(&hostmem->current_regions_lock);
> +    memory_listener_unregister(&hostmem_listener);
> +    hostmem_unref(cur_hostmem);
> +    qemu_mutex_destroy(&hostmem_lock);
>  }
> diff --git a/hw/dataplane/hostmem.h b/hw/dataplane/hostmem.h
> index b2cf093..883ba74 100644
> --- a/hw/dataplane/hostmem.h
> +++ b/hw/dataplane/hostmem.h
> @@ -20,29 +20,18 @@
>  typedef struct {
>      void *host_addr;
>      hwaddr guest_addr;
> +    MemoryRegion *mr;
>      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;
> +    int ref;
>      HostMemRegion *current_regions;
>      size_t num_current_regions;
>  } HostMem;
>  
> -void hostmem_init(HostMem *hostmem);
> -void hostmem_finalize(HostMem *hostmem);
> +void hostmem_unref(HostMem *hostmem);
>  
>  /**
>   * Map a guest physical address to a pointer
> @@ -52,6 +41,6 @@ void hostmem_finalize(HostMem *hostmem);
>   * 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);
> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write);
>  
>  #endif /* HOSTMEM_H */
>
Stefan Hajnoczi - April 11, 2013, 10:11 a.m.
On Mon, Apr 01, 2013 at 04:20:31PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> The hostmem listener will translate gpa to hva quickly. Make it
> global, so other component can use it.
> Also this patch adopt MemoryRegionOps's ref/unref interface to
> make it survive the RAM hotunplug.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/dataplane/hostmem.c |  130 +++++++++++++++++++++++++++++++++--------------
>  hw/dataplane/hostmem.h |   19 ++------
>  2 files changed, 95 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/dataplane/hostmem.c b/hw/dataplane/hostmem.c
> index 380537e..86c02cd 100644
> --- a/hw/dataplane/hostmem.c
> +++ b/hw/dataplane/hostmem.c
> @@ -13,6 +13,12 @@
>  
>  #include "exec/address-spaces.h"
>  #include "hostmem.h"
> +#include "qemu/main-loop.h"
> +
> +/* lock to protect the access to cur_hostmem */
> +static QemuMutex hostmem_lock;
> +static HostMem *cur_hostmem;
> +static HostMem *next_hostmem;
>  
>  static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>  {
> @@ -28,16 +34,49 @@ static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>      }
>  }
>  
> +static void hostmem_ref(HostMem *hostmem)
> +{
> +    assert(__sync_add_and_fetch(&hostmem->ref, 1) > 0);

man 3 assert:
"If  the macro NDEBUG was defined at the moment <assert.h> was last
included, the macro assert() generates no code, and hence does nothing
at all."

Never rely on a side-effect within an assert() call.  Store the return
value into a local variable and test the local in the assert().

Also, these sync gcc builtins are not available on all platforms or gcc
versions.  We need to be a little careful to avoid breaking builds here.
Maybe __sync_add_and_fetch() is fine but I wanted to mention it because
I've had trouble with these in the past.

I suggest using glib atomics instead, if possible.

> +}
> +
> +void hostmem_unref(HostMem *hostmem)
> +{
> +    int i;
> +    HostMemRegion *hmr;
> +
> +    if (!__sync_sub_and_fetch(&hostmem->ref, 1)) {
> +        for (i = 0; i < hostmem->num_current_regions; i++) {
> +            hmr = &hostmem->current_regions[i];
> +            /* Fix me, when memory hotunplug implement
> +             * assert(hmr->mr_ops->unref)
> +             */

Why this fixme?  Can you resolve it by making ->unref() return bool from
the start of this patch series?

> +            if (hmr->mr->ops && hmr->mr->ops->unref) {
> +                hmr->mr->ops->unref();
> +            }
> +        }
> +        g_free(hostmem->current_regions);
> +        g_free(hostmem);
> +    }
> +}
> +
>  /**
>   * Map guest physical address to host pointer
> + * also inc refcnt of *mr, caller need to unref later
>   */
> -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write)

A cleaner approach is to keep the hostmem_foo(HostMem *) functions and
have a global interface without the HostMem*.  That way the global
wrapper focusses on acquiring cur_hostmem while the existing functions
stay unchanged and focus on performing the actual operation.

>  {
>      HostMemRegion *region;
>      void *host_addr = NULL;
>      hwaddr offset_within_region;
> +    HostMem *hostmem;
> +
> +    assert(mr);
> +    *mr = NULL;
> +    qemu_mutex_lock(&hostmem_lock);
> +    hostmem = cur_hostmem;
> +    hostmem_ref(hostmem);
> +    qemu_mutex_unlock(&hostmem_lock);
>  
> -    qemu_mutex_lock(&hostmem->current_regions_lock);

Why is it safe to drop this lock?  The memory API could invoke callbacks
in another thread which causes us to update regions.

>      region = bsearch(&phys, hostmem->current_regions,
>                       hostmem->num_current_regions,
>                       sizeof(hostmem->current_regions[0]),
> @@ -52,28 +91,33 @@ void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
>      if (len <= region->size - offset_within_region) {
>          host_addr = region->host_addr + offset_within_region;
>      }
> -out:
> -    qemu_mutex_unlock(&hostmem->current_regions_lock);
> +    *mr = region->mr;
> +    memory_region_ref(*mr);

How does this protect against the QEMU thread hot unplugging while we
are searching hostmem->current_regions[]?  Our mr would be stale and the
ref operation would corrupt memory if mr has already been freed!

>  
> +out:
> +    hostmem_unref(hostmem);
>      return host_addr;
>  }
>  
> +static void hostmem_listener_begin(MemoryListener *listener)
> +{
> +    next_hostmem = g_new0(HostMem, 1);
> +    next_hostmem->ref = 1;
> +}
> +
>  /**
>   * Install new regions list
>   */
>  static void hostmem_listener_commit(MemoryListener *listener)
>  {
> -    HostMem *hostmem = container_of(listener, HostMem, listener);
> +    HostMem *tmp;
>  
> -    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);
> +    tmp = cur_hostmem;
> +    qemu_mutex_lock(&hostmem_lock);

In hostmem_lookup() you accessed cur_hostmem inside the lock.  Does the
lock protect cur_hostmem or not?

> +    cur_hostmem = next_hostmem;
> +    qemu_mutex_unlock(&hostmem_lock);
> +    hostmem_unref(tmp);
>  
> -    /* Reset new regions list */
> -    hostmem->new_regions = NULL;
> -    hostmem->num_new_regions = 0;
>  }
>  
>  /**

I gave up here.  The approach you are trying to take isn't clear in this
patch.  I've pointed out some inconsistencies but they make it hard to
review more since I don't understand what you're trying to do.

Please split patches into logical steps and especially document
locks/refcounts to explain their scope - what do they protect?

Stefan
Paolo Bonzini - April 11, 2013, 10:26 a.m.
Il 11/04/2013 12:11, Stefan Hajnoczi ha scritto:
> Also, these sync gcc builtins are not available on all platforms or gcc
> versions.  We need to be a little careful to avoid breaking builds here.
> Maybe __sync_add_and_fetch() is fine but I wanted to mention it because
> I've had trouble with these in the past.

We are already using GCC atomics elsewhere (spice, migration, vhost).
They are really ugly to type and not complete, so we should have our own
wrapper include, but they are fine.

Paolo
pingfan liu - April 12, 2013, 3:55 a.m.
On Thu, Apr 11, 2013 at 6:11 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Apr 01, 2013 at 04:20:31PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> The hostmem listener will translate gpa to hva quickly. Make it
>> global, so other component can use it.
>> Also this patch adopt MemoryRegionOps's ref/unref interface to
>> make it survive the RAM hotunplug.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/dataplane/hostmem.c |  130 +++++++++++++++++++++++++++++++++--------------
>>  hw/dataplane/hostmem.h |   19 ++------
>>  2 files changed, 95 insertions(+), 54 deletions(-)
>>
>> diff --git a/hw/dataplane/hostmem.c b/hw/dataplane/hostmem.c
>> index 380537e..86c02cd 100644
>> --- a/hw/dataplane/hostmem.c
>> +++ b/hw/dataplane/hostmem.c
>> @@ -13,6 +13,12 @@
>>
>>  #include "exec/address-spaces.h"
>>  #include "hostmem.h"
>> +#include "qemu/main-loop.h"
>> +
>> +/* lock to protect the access to cur_hostmem */
>> +static QemuMutex hostmem_lock;
>> +static HostMem *cur_hostmem;
>> +static HostMem *next_hostmem;
>>
>>  static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>>  {
>> @@ -28,16 +34,49 @@ static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>>      }
>>  }
>>
>> +static void hostmem_ref(HostMem *hostmem)
>> +{
>> +    assert(__sync_add_and_fetch(&hostmem->ref, 1) > 0);
>
> man 3 assert:
> "If  the macro NDEBUG was defined at the moment <assert.h> was last
> included, the macro assert() generates no code, and hence does nothing
> at all."
>
So if needed, using abort()?
> Never rely on a side-effect within an assert() call.  Store the return
> value into a local variable and test the local in the assert().
>
Ok.
> Also, these sync gcc builtins are not available on all platforms or gcc
> versions.  We need to be a little careful to avoid breaking builds here.
> Maybe __sync_add_and_fetch() is fine but I wanted to mention it because
> I've had trouble with these in the past.
>
> I suggest using glib atomics instead, if possible.
>
>> +}
>> +
>> +void hostmem_unref(HostMem *hostmem)
>> +{
>> +    int i;
>> +    HostMemRegion *hmr;
>> +
>> +    if (!__sync_sub_and_fetch(&hostmem->ref, 1)) {
>> +        for (i = 0; i < hostmem->num_current_regions; i++) {
>> +            hmr = &hostmem->current_regions[i];
>> +            /* Fix me, when memory hotunplug implement
>> +             * assert(hmr->mr_ops->unref)
>> +             */
>
> Why this fixme?  Can you resolve it by making ->unref() return bool from
> the start of this patch series?
>
This is used to notice the developer that the ref/unref interface
should be implemented for RAM device, since it is can be used during
RAM unplug.  And as you mentioned, here s/assert/abort/  Right?
>> +            if (hmr->mr->ops && hmr->mr->ops->unref) {
>> +                hmr->mr->ops->unref();
>> +            }
>> +        }
>> +        g_free(hostmem->current_regions);
>> +        g_free(hostmem);
>> +    }
>> +}
>> +
>>  /**
>>   * Map guest physical address to host pointer
>> + * also inc refcnt of *mr, caller need to unref later
>>   */
>> -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
>> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write)
>
> A cleaner approach is to keep the hostmem_foo(HostMem *) functions and
> have a global interface without the HostMem*.  That way the global
> wrapper focusses on acquiring cur_hostmem while the existing functions
> stay unchanged and focus on performing the actual operation.
>
The new API enforce a param "MemoryRegion **mr",  and rely on the
refcnt on it to survive the RAM unplug.  The caller should unref this
mr when done with it.  But the old API can not provide this, and not
easy to provide a wrapper.
>>  {
>>      HostMemRegion *region;
>>      void *host_addr = NULL;
>>      hwaddr offset_within_region;
>> +    HostMem *hostmem;
>> +
>> +    assert(mr);
>> +    *mr = NULL;
>> +    qemu_mutex_lock(&hostmem_lock);
>> +    hostmem = cur_hostmem;
>> +    hostmem_ref(hostmem);
>> +    qemu_mutex_unlock(&hostmem_lock);
>>
>> -    qemu_mutex_lock(&hostmem->current_regions_lock);
>
> Why is it safe to drop this lock?  The memory API could invoke callbacks
> in another thread which causes us to update regions.
>
The trick is the RCU. The lookup work will cur_hostmem, meanwhile if
there is a updater, it changes next_hostmem. So there is no race
between them.
>>      region = bsearch(&phys, hostmem->current_regions,
>>                       hostmem->num_current_regions,
>>                       sizeof(hostmem->current_regions[0]),
>> @@ -52,28 +91,33 @@ void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
>>      if (len <= region->size - offset_within_region) {
>>          host_addr = region->host_addr + offset_within_region;
>>      }
>> -out:
>> -    qemu_mutex_unlock(&hostmem->current_regions_lock);
>> +    *mr = region->mr;
>> +    memory_region_ref(*mr);
>
> How does this protect against the QEMU thread hot unplugging while we
> are searching hostmem->current_regions[]?  Our mr would be stale and the
> ref operation would corrupt memory if mr has already been freed!
>
When hostmem_listener_append_region, we inc refcnt of mr, this will
pin the mr. During the lookup, it will help us agaist unplug.  Then
after cur_hostmem retired to past, we will release the corresponding
refcnt which it holds.

>>
>> +out:
>> +    hostmem_unref(hostmem);
>>      return host_addr;
>>  }
>>
>> +static void hostmem_listener_begin(MemoryListener *listener)
>> +{
>> +    next_hostmem = g_new0(HostMem, 1);
>> +    next_hostmem->ref = 1;
>> +}
>> +
>>  /**
>>   * Install new regions list
>>   */
>>  static void hostmem_listener_commit(MemoryListener *listener)
>>  {
>> -    HostMem *hostmem = container_of(listener, HostMem, listener);
>> +    HostMem *tmp;
>>
>> -    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);
>> +    tmp = cur_hostmem;
>> +    qemu_mutex_lock(&hostmem_lock);
>
> In hostmem_lookup() you accessed cur_hostmem inside the lock.  Does the
> lock protect cur_hostmem or not?
>
Yes, protect. Supposed we have HostMem A, and it will become B. Then
hostmem_lookup will either see A or B.  If it see A, it should use A
refcnt agaist hostmem_listener_commit to drop A.  This refcnt has no
relation with mr's object's refcnt.
>> +    cur_hostmem = next_hostmem;
>> +    qemu_mutex_unlock(&hostmem_lock);
>> +    hostmem_unref(tmp);
>>
>> -    /* Reset new regions list */
>> -    hostmem->new_regions = NULL;
>> -    hostmem->num_new_regions = 0;
>>  }
>>
>>  /**
>
> I gave up here.  The approach you are trying to take isn't clear in this

The main idea differ from origianl code is rcu, we have two version of
HostMem, reader uses cur_hostmem, updater uses next_hostmem
> patch.  I've pointed out some inconsistencies but they make it hard to
> review more since I don't understand what you're trying to do.
>
> Please split patches into logical steps and especially document
> locks/refcounts to explain their scope - what do they protect?
>
Sorry for that, I will try to imrove it, and more document.

Thanks and regards,
Pingfan

> Stefan
pingfan liu - April 12, 2013, 6:46 a.m.
On Thu, Apr 11, 2013 at 6:09 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 01/04/2013 10:20, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> The hostmem listener will translate gpa to hva quickly. Make it
>> global, so other component can use it.
>> Also this patch adopt MemoryRegionOps's ref/unref interface to
>> make it survive the RAM hotunplug.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/dataplane/hostmem.c |  130 +++++++++++++++++++++++++++++++++--------------
>>  hw/dataplane/hostmem.h |   19 ++------
>>  2 files changed, 95 insertions(+), 54 deletions(-)
>>
>> diff --git a/hw/dataplane/hostmem.c b/hw/dataplane/hostmem.c
>> index 380537e..86c02cd 100644
>> --- a/hw/dataplane/hostmem.c
>> +++ b/hw/dataplane/hostmem.c
>> @@ -13,6 +13,12 @@
>>
>>  #include "exec/address-spaces.h"
>>  #include "hostmem.h"
>> +#include "qemu/main-loop.h"
>> +
>> +/* lock to protect the access to cur_hostmem */
>> +static QemuMutex hostmem_lock;
>> +static HostMem *cur_hostmem;
>> +static HostMem *next_hostmem;
>>
>>  static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>>  {
>> @@ -28,16 +34,49 @@ static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>>      }
>>  }
>>
>> +static void hostmem_ref(HostMem *hostmem)
>> +{
>> +    assert(__sync_add_and_fetch(&hostmem->ref, 1) > 0);
>> +}
>> +
>> +void hostmem_unref(HostMem *hostmem)
>> +{
>> +    int i;
>> +    HostMemRegion *hmr;
>> +
>> +    if (!__sync_sub_and_fetch(&hostmem->ref, 1)) {
>> +        for (i = 0; i < hostmem->num_current_regions; i++) {
>> +            hmr = &hostmem->current_regions[i];
>> +            /* Fix me, when memory hotunplug implement
>> +             * assert(hmr->mr_ops->unref)
>> +             */
>> +            if (hmr->mr->ops && hmr->mr->ops->unref) {
>> +                hmr->mr->ops->unref();
>> +            }
>> +        }
>> +        g_free(hostmem->current_regions);
>> +        g_free(hostmem);
>> +    }
>> +}
>> +
>>  /**
>>   * Map guest physical address to host pointer
>> + * also inc refcnt of *mr, caller need to unref later
>>   */
>> -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
>> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write)
>>  {
>>      HostMemRegion *region;
>>      void *host_addr = NULL;
>>      hwaddr offset_within_region;
>> +    HostMem *hostmem;
>> +
>> +    assert(mr);
>> +    *mr = NULL;
>
> I think it's simpler if you allow a NULL MemoryRegion.
>
But how can we guarantee the caller to release the refcnt?  In fact,
if @mr is not exposed to external component directly, it is still
required internally, for example, cpu_physical_memory_map() ported
onto HostMem, we will still rely on _unmap() to release the refcnt.

> Also, it is better if you keep the HostMem type, because in principle
> different HostMems could be used for different AddressSpaces.
> Basically, what is now HostMem would become HostMemRegions, and the new
> HostMem type would have these fields:
>
>    QemuMutex         hostmem_lock;
>    HostMemRegions   *cur_regions;
>    HostMemRegions   *next_regions;
>
> matching your three globals.  I like the rcu-ready design.
>
What about
AddressSpaceMemoryRegion {
  QemuMutex hostmem_lock;
  HostMem *cur_hostmem;
  HostMem *next_hostmem;
  MemoryListener listener;
}
So each AddressSpaceMemoryRegion can be bound with specific AddressSpace.

> Finally, please split this patch and patch 3 differently:
>
> 1) add HostMemRegions
>
> 2) add ref/unref of memory regions to hostmem
>
> 3) add MemoryRegion argument to hostmem_lookup, leave it NULL in vring
>
As explained above, seem we can not leave it NULL. (In theory, vring
seated on RAM can not be unplug, since it is kernel.  But I think we
can not rely on the guest's behavior)
> 4) add ref/unref of memory regions to vring
>
Thank you very much :-).  Will follow that, and arrange patches more clearly.

Regards,
Pingfan
> Paolo
>
>> +    qemu_mutex_lock(&hostmem_lock);
>> +    hostmem = cur_hostmem;
>> +    hostmem_ref(hostmem);
>> +    qemu_mutex_unlock(&hostmem_lock);
>>
>> -    qemu_mutex_lock(&hostmem->current_regions_lock);
>>      region = bsearch(&phys, hostmem->current_regions,
>>                       hostmem->num_current_regions,
>>                       sizeof(hostmem->current_regions[0]),
>> @@ -52,28 +91,33 @@ void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
>>      if (len <= region->size - offset_within_region) {
>>          host_addr = region->host_addr + offset_within_region;
>>      }
>> -out:
>> -    qemu_mutex_unlock(&hostmem->current_regions_lock);
>> +    *mr = region->mr;
>> +    memory_region_ref(*mr);
>>
>> +out:
>> +    hostmem_unref(hostmem);
>>      return host_addr;
>>  }
>>
>> +static void hostmem_listener_begin(MemoryListener *listener)
>> +{
>> +    next_hostmem = g_new0(HostMem, 1);
>> +    next_hostmem->ref = 1;
>> +}
>> +
>>  /**
>>   * Install new regions list
>>   */
>>  static void hostmem_listener_commit(MemoryListener *listener)
>>  {
>> -    HostMem *hostmem = container_of(listener, HostMem, listener);
>> +    HostMem *tmp;
>>
>> -    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);
>> +    tmp = cur_hostmem;
>> +    qemu_mutex_lock(&hostmem_lock);
>> +    cur_hostmem = next_hostmem;
>> +    qemu_mutex_unlock(&hostmem_lock);
>> +    hostmem_unref(tmp);
>>
>> -    /* Reset new regions list */
>> -    hostmem->new_regions = NULL;
>> -    hostmem->num_new_regions = 0;
>>  }
>>
>>  /**
>> @@ -83,23 +127,24 @@ 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]);
>> +    size_t num = hostmem->num_current_regions;
>> +    size_t new_size = (num + 1) * sizeof(hostmem->current_regions[0]);
>>
>> -    hostmem->new_regions = g_realloc(hostmem->new_regions, new_size);
>> -    hostmem->new_regions[num] = (HostMemRegion){
>> +    hostmem->current_regions = g_realloc(hostmem->current_regions, new_size);
>> +    hostmem->current_regions[num] = (HostMemRegion){
>>          .host_addr = ram_ptr + section->offset_within_region,
>>          .guest_addr = section->offset_within_address_space,
>> +        .mr = section->mr,
>>          .size = section->size,
>>          .readonly = section->readonly,
>>      };
>> -    hostmem->num_new_regions++;
>> +    hostmem->num_current_regions++;
>>  }
>>
>> -static void hostmem_listener_append_region(MemoryListener *listener,
>> +static void hostmem_listener_nop_region(MemoryListener *listener,
>>                                             MemoryRegionSection *section)
>>  {
>> -    HostMem *hostmem = container_of(listener, HostMem, listener);
>> +    HostMem *hostmem = next_hostmem;
>>
>>      /* Ignore non-RAM regions, we may not be able to map them */
>>      if (!memory_region_is_ram(section->mr)) {
>> @@ -114,6 +159,18 @@ static void hostmem_listener_append_region(MemoryListener *listener,
>>      hostmem_append_new_region(hostmem, section);
>>  }
>>
>> +static void hostmem_listener_append_region(MemoryListener *listener,
>> +                                           MemoryRegionSection *section)
>> +{
>> +    hostmem_listener_nop_region(listener, section);
>> +    /* Fix me, when memory hotunplug implement
>> +     * assert(section->mr->ops->ref)
>> +     */
>> +    if (section->mr->ops && section->mr->ops->ref) {
>> +        section->mr->ops->ref();
>> +    }
>> +}
>> +
>>  /* We don't implement most MemoryListener callbacks, use these nop stubs */
>>  static void hostmem_listener_dummy(MemoryListener *listener)
>>  {
>> @@ -137,18 +194,12 @@ static void hostmem_listener_coalesced_mmio_dummy(MemoryListener *listener,
>>  {
>>  }
>>
>> -void hostmem_init(HostMem *hostmem)
>> -{
>> -    memset(hostmem, 0, sizeof(*hostmem));
>> -
>> -    qemu_mutex_init(&hostmem->current_regions_lock);
>> -
>> -    hostmem->listener = (MemoryListener){
>> -        .begin = hostmem_listener_dummy,
>> +static MemoryListener hostmem_listener = {
>> +        .begin = hostmem_listener_begin,
>>          .commit = hostmem_listener_commit,
>>          .region_add = hostmem_listener_append_region,
>>          .region_del = hostmem_listener_section_dummy,
>> -        .region_nop = hostmem_listener_append_region,
>> +        .region_nop = hostmem_listener_nop_region,
>>          .log_start = hostmem_listener_section_dummy,
>>          .log_stop = hostmem_listener_section_dummy,
>>          .log_sync = hostmem_listener_section_dummy,
>> @@ -159,18 +210,19 @@ void hostmem_init(HostMem *hostmem)
>>          .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_init(void)
>> +{
>> +    qemu_mutex_init(&hostmem_lock);
>> +    cur_hostmem = g_new0(HostMem, 1);
>> +    cur_hostmem->ref = 1;
>> +    memory_listener_register(&hostmem_listener, &address_space_memory);
>>  }
>>
>> -void hostmem_finalize(HostMem *hostmem)
>> +void hostmem_finalize(void)
>>  {
>> -    memory_listener_unregister(&hostmem->listener);
>> -    g_free(hostmem->new_regions);
>> -    g_free(hostmem->current_regions);
>> -    qemu_mutex_destroy(&hostmem->current_regions_lock);
>> +    memory_listener_unregister(&hostmem_listener);
>> +    hostmem_unref(cur_hostmem);
>> +    qemu_mutex_destroy(&hostmem_lock);
>>  }
>> diff --git a/hw/dataplane/hostmem.h b/hw/dataplane/hostmem.h
>> index b2cf093..883ba74 100644
>> --- a/hw/dataplane/hostmem.h
>> +++ b/hw/dataplane/hostmem.h
>> @@ -20,29 +20,18 @@
>>  typedef struct {
>>      void *host_addr;
>>      hwaddr guest_addr;
>> +    MemoryRegion *mr;
>>      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;
>> +    int ref;
>>      HostMemRegion *current_regions;
>>      size_t num_current_regions;
>>  } HostMem;
>>
>> -void hostmem_init(HostMem *hostmem);
>> -void hostmem_finalize(HostMem *hostmem);
>> +void hostmem_unref(HostMem *hostmem);
>>
>>  /**
>>   * Map a guest physical address to a pointer
>> @@ -52,6 +41,6 @@ void hostmem_finalize(HostMem *hostmem);
>>   * 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);
>> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write);
>>
>>  #endif /* HOSTMEM_H */
>>
>
Stefan Hajnoczi - April 12, 2013, 8:21 a.m.
On Fri, Apr 12, 2013 at 02:46:41PM +0800, liu ping fan wrote:
> On Thu, Apr 11, 2013 at 6:09 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 01/04/2013 10:20, Liu Ping Fan ha scritto:
> > Finally, please split this patch and patch 3 differently:
> >
> > 1) add HostMemRegions
> >
> > 2) add ref/unref of memory regions to hostmem
> >
> > 3) add MemoryRegion argument to hostmem_lookup, leave it NULL in vring
> >
> As explained above, seem we can not leave it NULL. (In theory, vring
> seated on RAM can not be unplug, since it is kernel.  But I think we
> can not rely on the guest's behavior)

It should be possible to evacuate the vring.  The guest needs to reset
the device so the vring is unmapped.  Then memory can be unplugged.
Reinitializing the device will allocate a new vring and things work
again.

Stefan
Stefan Hajnoczi - April 12, 2013, 8:38 a.m.
On Fri, Apr 12, 2013 at 11:55:39AM +0800, liu ping fan wrote:
> On Thu, Apr 11, 2013 at 6:11 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Mon, Apr 01, 2013 at 04:20:31PM +0800, Liu Ping Fan wrote:
> >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >>
> >> The hostmem listener will translate gpa to hva quickly. Make it
> >> global, so other component can use it.
> >> Also this patch adopt MemoryRegionOps's ref/unref interface to
> >> make it survive the RAM hotunplug.
> >>
> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> ---
> >>  hw/dataplane/hostmem.c |  130 +++++++++++++++++++++++++++++++++--------------
> >>  hw/dataplane/hostmem.h |   19 ++------
> >>  2 files changed, 95 insertions(+), 54 deletions(-)
> >>
> >> diff --git a/hw/dataplane/hostmem.c b/hw/dataplane/hostmem.c
> >> index 380537e..86c02cd 100644
> >> --- a/hw/dataplane/hostmem.c
> >> +++ b/hw/dataplane/hostmem.c
> >> @@ -13,6 +13,12 @@
> >>
> >>  #include "exec/address-spaces.h"
> >>  #include "hostmem.h"
> >> +#include "qemu/main-loop.h"
> >> +
> >> +/* lock to protect the access to cur_hostmem */
> >> +static QemuMutex hostmem_lock;
> >> +static HostMem *cur_hostmem;
> >> +static HostMem *next_hostmem;
> >>
> >>  static int hostmem_lookup_cmp(const void *phys_, const void *region_)
> >>  {
> >> @@ -28,16 +34,49 @@ static int hostmem_lookup_cmp(const void *phys_, const void *region_)
> >>      }
> >>  }
> >>
> >> +static void hostmem_ref(HostMem *hostmem)
> >> +{
> >> +    assert(__sync_add_and_fetch(&hostmem->ref, 1) > 0);
> >
> > man 3 assert:
> > "If  the macro NDEBUG was defined at the moment <assert.h> was last
> > included, the macro assert() generates no code, and hence does nothing
> > at all."
> >
> So if needed, using abort()?

No, you can still use assert but don't put side-effects into the
assertion:

int old = __sync_add_and_fetch(&hostmem->ref, 1);
assert(old > 0);

> > Never rely on a side-effect within an assert() call.  Store the return
> > value into a local variable and test the local in the assert().
> >
> Ok.
> > Also, these sync gcc builtins are not available on all platforms or gcc
> > versions.  We need to be a little careful to avoid breaking builds here.
> > Maybe __sync_add_and_fetch() is fine but I wanted to mention it because
> > I've had trouble with these in the past.
> >
> > I suggest using glib atomics instead, if possible.
> >
> >> +}
> >> +
> >> +void hostmem_unref(HostMem *hostmem)
> >> +{
> >> +    int i;
> >> +    HostMemRegion *hmr;
> >> +
> >> +    if (!__sync_sub_and_fetch(&hostmem->ref, 1)) {
> >> +        for (i = 0; i < hostmem->num_current_regions; i++) {
> >> +            hmr = &hostmem->current_regions[i];
> >> +            /* Fix me, when memory hotunplug implement
> >> +             * assert(hmr->mr_ops->unref)
> >> +             */
> >
> > Why this fixme?  Can you resolve it by making ->unref() return bool from
> > the start of this patch series?
> >
> This is used to notice the developer that the ref/unref interface
> should be implemented for RAM device, since it is can be used during
> RAM unplug.  And as you mentioned, here s/assert/abort/  Right?

Ah, sorry.  I misread the assertion, you want to require that ->unref()
is implemented.

The assertion is fine the way it is.

> >> +            if (hmr->mr->ops && hmr->mr->ops->unref) {
> >> +                hmr->mr->ops->unref();
> >> +            }
> >> +        }
> >> +        g_free(hostmem->current_regions);
> >> +        g_free(hostmem);
> >> +    }
> >> +}
> >> +
> >>  /**
> >>   * Map guest physical address to host pointer
> >> + * also inc refcnt of *mr, caller need to unref later
> >>   */
> >> -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
> >> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write)
> >
> > A cleaner approach is to keep the hostmem_foo(HostMem *) functions and
> > have a global interface without the HostMem*.  That way the global
> > wrapper focusses on acquiring cur_hostmem while the existing functions
> > stay unchanged and focus on performing the actual operation.
> >
> The new API enforce a param "MemoryRegion **mr",  and rely on the
> refcnt on it to survive the RAM unplug.  The caller should unref this
> mr when done with it.  But the old API can not provide this, and not
> easy to provide a wrapper.

I understand, MemoryRegion needs to be added to the function.

But it would be nice to keep the hostmem_lock and cur_hostmem stuff
separate to make this function easier to understand.  In other words,
keep the globals away from the code that operates on a HostMem.  It
makes the code easier to read and guarantees to the reader that you are
not mixing in assumptions about global state.

> >>  {
> >>      HostMemRegion *region;
> >>      void *host_addr = NULL;
> >>      hwaddr offset_within_region;
> >> +    HostMem *hostmem;
> >> +
> >> +    assert(mr);
> >> +    *mr = NULL;
> >> +    qemu_mutex_lock(&hostmem_lock);
> >> +    hostmem = cur_hostmem;
> >> +    hostmem_ref(hostmem);
> >> +    qemu_mutex_unlock(&hostmem_lock);
> >>
> >> -    qemu_mutex_lock(&hostmem->current_regions_lock);
> >
> > Why is it safe to drop this lock?  The memory API could invoke callbacks
> > in another thread which causes us to update regions.
> >
> The trick is the RCU. The lookup work will cur_hostmem, meanwhile if
> there is a updater, it changes next_hostmem. So there is no race
> between them.

I see.  Please document the nature of the cur/next variables in
comments.

You can make the code review process smoother by laying out patches in a
way that makes them logical and easy to understand.  Right now it feels
like I have to reverse-engineer what you were thinking in order to
understand the patches.

> >>      region = bsearch(&phys, hostmem->current_regions,
> >>                       hostmem->num_current_regions,
> >>                       sizeof(hostmem->current_regions[0]),
> >> @@ -52,28 +91,33 @@ void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
> >>      if (len <= region->size - offset_within_region) {
> >>          host_addr = region->host_addr + offset_within_region;
> >>      }
> >> -out:
> >> -    qemu_mutex_unlock(&hostmem->current_regions_lock);
> >> +    *mr = region->mr;
> >> +    memory_region_ref(*mr);
> >
> > How does this protect against the QEMU thread hot unplugging while we
> > are searching hostmem->current_regions[]?  Our mr would be stale and the
> > ref operation would corrupt memory if mr has already been freed!
> >
> When hostmem_listener_append_region, we inc refcnt of mr, this will
> pin the mr. During the lookup, it will help us agaist unplug.  Then
> after cur_hostmem retired to past, we will release the corresponding
> refcnt which it holds.

I see.  This also explain why hostmem_ref()/hostmem_unref() a
asymmetric: ref() just increments hostmem's refcount while unref()
actually decrements refcounts for memory regions.  It's something I
wondered about when looking at those functions.

> >>
> >> +out:
> >> +    hostmem_unref(hostmem);
> >>      return host_addr;
> >>  }
> >>
> >> +static void hostmem_listener_begin(MemoryListener *listener)
> >> +{
> >> +    next_hostmem = g_new0(HostMem, 1);
> >> +    next_hostmem->ref = 1;
> >> +}
> >> +
> >>  /**
> >>   * Install new regions list
> >>   */
> >>  static void hostmem_listener_commit(MemoryListener *listener)
> >>  {
> >> -    HostMem *hostmem = container_of(listener, HostMem, listener);
> >> +    HostMem *tmp;
> >>
> >> -    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);
> >> +    tmp = cur_hostmem;
> >> +    qemu_mutex_lock(&hostmem_lock);
> >
> > In hostmem_lookup() you accessed cur_hostmem inside the lock.  Does the
> > lock protect cur_hostmem or not?
> >
> Yes, protect. Supposed we have HostMem A, and it will become B. Then
> hostmem_lookup will either see A or B.  If it see A, it should use A
> refcnt agaist hostmem_listener_commit to drop A.  This refcnt has no
> relation with mr's object's refcnt.

My question is why you are accessing cur_hostmem outside hostmem_lock
but then assigning it inside the lock on the next line...

> >> +    cur_hostmem = next_hostmem;

...here.

If you want an atomic exchange then tmp = cur_hostmem should be inside
the lock.
Paolo Bonzini - April 12, 2013, 10:03 a.m.
Il 12/04/2013 10:38, Stefan Hajnoczi ha scritto:
>> > Yes, protect. Supposed we have HostMem A, and it will become B. Then
>> > hostmem_lookup will either see A or B.  If it see A, it should use A
>> > refcnt agaist hostmem_listener_commit to drop A.  This refcnt has no
>> > relation with mr's object's refcnt.
> My question is why you are accessing cur_hostmem outside hostmem_lock
> but then assigning it inside the lock on the next line...
> 
>>>> > >> +    cur_hostmem = next_hostmem;
> ...here.
> 
> If you want an atomic exchange then tmp = cur_hostmem should be inside
> the lock.

It will work because readers will grab either the hostmem_lock or the
BQL, while writers will grab both.  A kind of local/global lock, but I'm
not sure it was intentional. :)  It's simpler to just move the read
inside the lock.

Paolo
pingfan liu - April 15, 2013, 1:42 a.m.
On Fri, Apr 12, 2013 at 6:03 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 12/04/2013 10:38, Stefan Hajnoczi ha scritto:
>>> > Yes, protect. Supposed we have HostMem A, and it will become B. Then
>>> > hostmem_lookup will either see A or B.  If it see A, it should use A
>>> > refcnt agaist hostmem_listener_commit to drop A.  This refcnt has no
>>> > relation with mr's object's refcnt.
>> My question is why you are accessing cur_hostmem outside hostmem_lock
>> but then assigning it inside the lock on the next line...
>>
>>>>> > >> +    cur_hostmem = next_hostmem;
>> ...here.
>>
>> If you want an atomic exchange then tmp = cur_hostmem should be inside
>> the lock.
>
> It will work because readers will grab either the hostmem_lock or the
> BQL, while writers will grab both.  A kind of local/global lock, but I'm
No only hostmem_lock is used to protect readers from writers.  While
the writers are protected agaist each other by biglock.  So leaving
the "cur_hostmem = next_hostmem" outside to reflect the lock's rules.

Regards,
Pingfan
> not sure it was intentional. :)  It's simpler to just move the read
> inside the lock.
>
> Paolo
Paolo Bonzini - April 15, 2013, 6:38 a.m.
Il 15/04/2013 03:42, liu ping fan ha scritto:
> > It will work because readers will grab either the hostmem_lock or the
> > BQL, while writers will grab both.  A kind of local/global lock, but I'm
> 
> No only hostmem_lock is used to protect readers from writers.  While
> the writers are protected agaist each other by biglock.  So leaving
> the "cur_hostmem = next_hostmem" outside to reflect the lock's rules.

Doesn't help much if you do not document them...

Paolo

Patch

diff --git a/hw/dataplane/hostmem.c b/hw/dataplane/hostmem.c
index 380537e..86c02cd 100644
--- a/hw/dataplane/hostmem.c
+++ b/hw/dataplane/hostmem.c
@@ -13,6 +13,12 @@ 
 
 #include "exec/address-spaces.h"
 #include "hostmem.h"
+#include "qemu/main-loop.h"
+
+/* lock to protect the access to cur_hostmem */
+static QemuMutex hostmem_lock;
+static HostMem *cur_hostmem;
+static HostMem *next_hostmem;
 
 static int hostmem_lookup_cmp(const void *phys_, const void *region_)
 {
@@ -28,16 +34,49 @@  static int hostmem_lookup_cmp(const void *phys_, const void *region_)
     }
 }
 
+static void hostmem_ref(HostMem *hostmem)
+{
+    assert(__sync_add_and_fetch(&hostmem->ref, 1) > 0);
+}
+
+void hostmem_unref(HostMem *hostmem)
+{
+    int i;
+    HostMemRegion *hmr;
+
+    if (!__sync_sub_and_fetch(&hostmem->ref, 1)) {
+        for (i = 0; i < hostmem->num_current_regions; i++) {
+            hmr = &hostmem->current_regions[i];
+            /* Fix me, when memory hotunplug implement
+             * assert(hmr->mr_ops->unref)
+             */
+            if (hmr->mr->ops && hmr->mr->ops->unref) {
+                hmr->mr->ops->unref();
+            }
+        }
+        g_free(hostmem->current_regions);
+        g_free(hostmem);
+    }
+}
+
 /**
  * Map guest physical address to host pointer
+ * also inc refcnt of *mr, caller need to unref later
  */
-void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
+void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write)
 {
     HostMemRegion *region;
     void *host_addr = NULL;
     hwaddr offset_within_region;
+    HostMem *hostmem;
+
+    assert(mr);
+    *mr = NULL;
+    qemu_mutex_lock(&hostmem_lock);
+    hostmem = cur_hostmem;
+    hostmem_ref(hostmem);
+    qemu_mutex_unlock(&hostmem_lock);
 
-    qemu_mutex_lock(&hostmem->current_regions_lock);
     region = bsearch(&phys, hostmem->current_regions,
                      hostmem->num_current_regions,
                      sizeof(hostmem->current_regions[0]),
@@ -52,28 +91,33 @@  void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write)
     if (len <= region->size - offset_within_region) {
         host_addr = region->host_addr + offset_within_region;
     }
-out:
-    qemu_mutex_unlock(&hostmem->current_regions_lock);
+    *mr = region->mr;
+    memory_region_ref(*mr);
 
+out:
+    hostmem_unref(hostmem);
     return host_addr;
 }
 
+static void hostmem_listener_begin(MemoryListener *listener)
+{
+    next_hostmem = g_new0(HostMem, 1);
+    next_hostmem->ref = 1;
+}
+
 /**
  * Install new regions list
  */
 static void hostmem_listener_commit(MemoryListener *listener)
 {
-    HostMem *hostmem = container_of(listener, HostMem, listener);
+    HostMem *tmp;
 
-    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);
+    tmp = cur_hostmem;
+    qemu_mutex_lock(&hostmem_lock);
+    cur_hostmem = next_hostmem;
+    qemu_mutex_unlock(&hostmem_lock);
+    hostmem_unref(tmp);
 
-    /* Reset new regions list */
-    hostmem->new_regions = NULL;
-    hostmem->num_new_regions = 0;
 }
 
 /**
@@ -83,23 +127,24 @@  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]);
+    size_t num = hostmem->num_current_regions;
+    size_t new_size = (num + 1) * sizeof(hostmem->current_regions[0]);
 
-    hostmem->new_regions = g_realloc(hostmem->new_regions, new_size);
-    hostmem->new_regions[num] = (HostMemRegion){
+    hostmem->current_regions = g_realloc(hostmem->current_regions, new_size);
+    hostmem->current_regions[num] = (HostMemRegion){
         .host_addr = ram_ptr + section->offset_within_region,
         .guest_addr = section->offset_within_address_space,
+        .mr = section->mr,
         .size = section->size,
         .readonly = section->readonly,
     };
-    hostmem->num_new_regions++;
+    hostmem->num_current_regions++;
 }
 
-static void hostmem_listener_append_region(MemoryListener *listener,
+static void hostmem_listener_nop_region(MemoryListener *listener,
                                            MemoryRegionSection *section)
 {
-    HostMem *hostmem = container_of(listener, HostMem, listener);
+    HostMem *hostmem = next_hostmem;
 
     /* Ignore non-RAM regions, we may not be able to map them */
     if (!memory_region_is_ram(section->mr)) {
@@ -114,6 +159,18 @@  static void hostmem_listener_append_region(MemoryListener *listener,
     hostmem_append_new_region(hostmem, section);
 }
 
+static void hostmem_listener_append_region(MemoryListener *listener,
+                                           MemoryRegionSection *section)
+{
+    hostmem_listener_nop_region(listener, section);
+    /* Fix me, when memory hotunplug implement
+     * assert(section->mr->ops->ref)
+     */
+    if (section->mr->ops && section->mr->ops->ref) {
+        section->mr->ops->ref();
+    }
+}
+
 /* We don't implement most MemoryListener callbacks, use these nop stubs */
 static void hostmem_listener_dummy(MemoryListener *listener)
 {
@@ -137,18 +194,12 @@  static void hostmem_listener_coalesced_mmio_dummy(MemoryListener *listener,
 {
 }
 
-void hostmem_init(HostMem *hostmem)
-{
-    memset(hostmem, 0, sizeof(*hostmem));
-
-    qemu_mutex_init(&hostmem->current_regions_lock);
-
-    hostmem->listener = (MemoryListener){
-        .begin = hostmem_listener_dummy,
+static MemoryListener hostmem_listener = {
+        .begin = hostmem_listener_begin,
         .commit = hostmem_listener_commit,
         .region_add = hostmem_listener_append_region,
         .region_del = hostmem_listener_section_dummy,
-        .region_nop = hostmem_listener_append_region,
+        .region_nop = hostmem_listener_nop_region,
         .log_start = hostmem_listener_section_dummy,
         .log_stop = hostmem_listener_section_dummy,
         .log_sync = hostmem_listener_section_dummy,
@@ -159,18 +210,19 @@  void hostmem_init(HostMem *hostmem)
         .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_init(void)
+{
+    qemu_mutex_init(&hostmem_lock);
+    cur_hostmem = g_new0(HostMem, 1);
+    cur_hostmem->ref = 1;
+    memory_listener_register(&hostmem_listener, &address_space_memory);
 }
 
-void hostmem_finalize(HostMem *hostmem)
+void hostmem_finalize(void)
 {
-    memory_listener_unregister(&hostmem->listener);
-    g_free(hostmem->new_regions);
-    g_free(hostmem->current_regions);
-    qemu_mutex_destroy(&hostmem->current_regions_lock);
+    memory_listener_unregister(&hostmem_listener);
+    hostmem_unref(cur_hostmem);
+    qemu_mutex_destroy(&hostmem_lock);
 }
diff --git a/hw/dataplane/hostmem.h b/hw/dataplane/hostmem.h
index b2cf093..883ba74 100644
--- a/hw/dataplane/hostmem.h
+++ b/hw/dataplane/hostmem.h
@@ -20,29 +20,18 @@ 
 typedef struct {
     void *host_addr;
     hwaddr guest_addr;
+    MemoryRegion *mr;
     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;
+    int ref;
     HostMemRegion *current_regions;
     size_t num_current_regions;
 } HostMem;
 
-void hostmem_init(HostMem *hostmem);
-void hostmem_finalize(HostMem *hostmem);
+void hostmem_unref(HostMem *hostmem);
 
 /**
  * Map a guest physical address to a pointer
@@ -52,6 +41,6 @@  void hostmem_finalize(HostMem *hostmem);
  * 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);
+void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write);
 
 #endif /* HOSTMEM_H */