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