Re: [Qemu-devel] [PATCH 2/5] hostmem: make hostmem global and RAM hotunplg safe

[liu ping fan]

On Thu, Apr 11, 2013 at 6:09 PM, Paolo Bonzini <address@hidden> wrote:
> Il 01/04/2013 10:20, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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 */
>>
>