[Qemu-devel] [PATCH 0/4] More cleanups for RAMBlock/ram_addr_t vs. MemoryRegion

[Qemu-devel] [PATCH 0/4] More cleanups for RAMBlock/ram_addr_t vs. MemoryRegion

[Paolo Bonzini]

This removes some more cases where we unnecessarily go through
ram_addr_t.  Now that MemoryRegions have a RAMBlock pointer, it is
possible to keep reasoning in terms of MemoryRegion+offset everywhere,
except when managing dirty bitmaps and TCG TLB entries.  ram_addr.h
is now included only by cputlb.c, exec.c, kvm-all.c (for dirty bitmaps),
memory.c and migration/ram.c (also for dirty bitmaps).

The diffstat is lying, because some of the insertions actually come
from doc comments. :)

Paolo

Paolo Bonzini (4):
  memory: remove qemu_get_ram_fd, qemu_set_ram_fd,
    qemu_ram_block_host_ptr
  exec: remove ram_addr argument from qemu_ram_block_from_host
  memory: split memory_region_from_host from qemu_ram_addr_from_host
  exec: hide mr->ram_addr from qemu_get_ram_ptr users

 cputlb.c                     |   3 +-
 exec.c                       | 110 +++++++++++++------------------------------
 hw/misc/ivshmem.c            |   5 +-
 hw/virtio/vhost-user.c       |  25 +++++-----
 include/exec/cpu-common.h    |   4 +-
 include/exec/memory.h        |  36 ++++++++++++--
 include/exec/ram_addr.h      |   3 --
 memory.c                     |  39 +++++++++++----
 migration/postcopy-ram.c     |   3 +-
 scripts/dump-guest-memory.py |  19 ++------
 target-i386/kvm.c            |   6 ++-
 11 files changed, 122 insertions(+), 131 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/4] memory: remove qemu_get_ram_fd, qemu_set_ram_fd, qemu_ram_block_host_ptr

[Paolo Bonzini]

Remove direct uses of ram_addr_t and optimize memory_region_{get,set}_fd
now that a MemoryRegion knows its RAMBlock directly.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                  | 34 ----------------------------------
 hw/misc/ivshmem.c       |  5 ++---
 hw/virtio/vhost-user.c  | 15 +++++++--------
 include/exec/memory.h   | 11 +++++++++++
 include/exec/ram_addr.h |  3 ---
 memory.c                | 21 +++++++++++++++++----
 6 files changed, 37 insertions(+), 52 deletions(-)

diff --git a/exec.c b/exec.c
index a3a93ae..3330e7d 100644
--- a/exec.c
+++ b/exec.c
@@ -1815,40 +1815,6 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
 }
 #endif /* !_WIN32 */
 
-int qemu_get_ram_fd(ram_addr_t addr)
-{
-    RAMBlock *block;
-    int fd;
-
-    rcu_read_lock();
-    block = qemu_get_ram_block(addr);
-    fd = block->fd;
-    rcu_read_unlock();
-    return fd;
-}
-
-void qemu_set_ram_fd(ram_addr_t addr, int fd)
-{
-    RAMBlock *block;
-
-    rcu_read_lock();
-    block = qemu_get_ram_block(addr);
-    block->fd = fd;
-    rcu_read_unlock();
-}
-
-void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
-{
-    RAMBlock *block;
-    void *ptr;
-
-    rcu_read_lock();
-    block = qemu_get_ram_block(addr);
-    ptr = ramblock_ptr(block, 0);
-    rcu_read_unlock();
-    return ptr;
-}
-
 /* Return a host pointer to ram allocated with qemu_ram_alloc.
  * This should not be used for general purpose DMA.  Use address_space_map
  * or address_space_rw instead. For local memory (e.g. video ram) that the
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index e40f23b..90be9f7 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -33,7 +33,6 @@
 #include "sysemu/hostmem.h"
 #include "sysemu/qtest.h"
 #include "qapi/visitor.h"
-#include "exec/ram_addr.h"
 
 #include "hw/misc/ivshmem.h"
 
@@ -533,7 +532,7 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
     }
     memory_region_init_ram_ptr(&s->server_bar2, OBJECT(s),
                                "ivshmem.bar2", size, ptr);
-    qemu_set_ram_fd(memory_region_get_ram_addr(&s->server_bar2), fd);
+    memory_region_set_fd(&s->server_bar2, fd);
     s->ivshmem_bar2 = &s->server_bar2;
 }
 
@@ -940,7 +939,7 @@ static void ivshmem_exit(PCIDevice *dev)
                              strerror(errno));
             }
 
-            fd = qemu_get_ram_fd(memory_region_get_ram_addr(s->ivshmem_bar2));
+            fd = memory_region_get_fd(s->ivshmem_bar2);
             close(fd);
         }
 
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 5914e85..41908c0 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -248,17 +248,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
     for (i = 0; i < dev->mem->nregions; ++i) {
         struct vhost_memory_region *reg = dev->mem->regions + i;
         ram_addr_t ram_addr;
+        MemoryRegion *mr;
 
         assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
-        qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
-                                &ram_addr);
-        fd = qemu_get_ram_fd(ram_addr);
+        mr = qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
+                                     &ram_addr);
+        fd = memory_region_get_fd(mr);
         if (fd > 0) {
             msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
             msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
             msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
             msg.payload.memory.regions[fd_num].mmap_offset = reg->userspace_addr -
-                (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
+                (uintptr_t) memory_region_get_ram_ptr(mr);
             assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
             fds[fd_num++] = fd;
         }
@@ -621,12 +622,10 @@ static bool vhost_user_can_merge(struct vhost_dev *dev,
     MemoryRegion *mr;
 
     mr = qemu_ram_addr_from_host((void *)(uintptr_t)start1, &ram_addr);
-    assert(mr);
-    mfd = qemu_get_ram_fd(ram_addr);
+    mfd = memory_region_get_fd(mr);
 
     mr = qemu_ram_addr_from_host((void *)(uintptr_t)start2, &ram_addr);
-    assert(mr);
-    rfd = qemu_get_ram_fd(ram_addr);
+    rfd = memory_region_get_fd(mr);
 
     return mfd == rfd;
 }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index f649697..1678494 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -667,6 +667,17 @@ static inline bool memory_region_is_rom(MemoryRegion *mr)
 int memory_region_get_fd(MemoryRegion *mr);
 
 /**
+ * memory_region_set_fd: Mark a RAM memory region as backed by a
+ * file descriptor.
+ *
+ * This function is typically used after memory_region_init_ram_ptr().
+ *
+ * @mr: the memory region being queried.
+ * @fd: the file descriptor that backs @mr.
+ */
+void memory_region_set_fd(MemoryRegion *mr, int fd);
+
+/**
  * memory_region_get_ram_ptr: Get a pointer into a RAM memory region.
  *
  * Returns a host pointer to a RAM memory region (created with
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 5b6e1b8..2a9465d 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -105,9 +105,6 @@ RAMBlock *qemu_ram_alloc_resizeable(ram_addr_t size, ram_addr_t max_size,
                                                     uint64_t length,
                                                     void *host),
                                     MemoryRegion *mr, Error **errp);
-int qemu_get_ram_fd(ram_addr_t addr);
-void qemu_set_ram_fd(ram_addr_t addr, int fd);
-void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
 void qemu_ram_free(RAMBlock *block);
 
 int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp);
diff --git a/memory.c b/memory.c
index 0f52522..d6a4a68 100644
--- a/memory.c
+++ b/memory.c
@@ -1626,13 +1626,26 @@ void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
 
 int memory_region_get_fd(MemoryRegion *mr)
 {
-    if (mr->alias) {
-        return memory_region_get_fd(mr->alias);
+    int fd;
+
+    rcu_read_lock();
+    while (mr->alias) {
+        mr = mr->alias;
     }
+    fd = mr->ram_block->fd;
+    rcu_read_unlock();
 
-    assert(mr->ram_block);
+    return fd;
+}
 
-    return qemu_get_ram_fd(memory_region_get_ram_addr(mr));
+void memory_region_set_fd(MemoryRegion *mr, int fd)
+{
+    rcu_read_lock();
+    while (mr->alias) {
+        mr = mr->alias;
+    }
+    mr->ram_block->fd = fd;
+    rcu_read_unlock();
 }
 
 void *memory_region_get_ram_ptr(MemoryRegion *mr)
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/4] memory: remove qemu_get_ram_fd, qemu_set_ram_fd, qemu_ram_block_host_ptr

[Marc-André Lureau]

Hi

On Thu, May 26, 2016 at 10:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Remove direct uses of ram_addr_t and optimize memory_region_{get,set}_fd
> now that a MemoryRegion knows its RAMBlock directly.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                  | 34 ----------------------------------
>  hw/misc/ivshmem.c       |  5 ++---
>  hw/virtio/vhost-user.c  | 15 +++++++--------
>  include/exec/memory.h   | 11 +++++++++++
>  include/exec/ram_addr.h |  3 ---
>  memory.c                | 21 +++++++++++++++++----
>  6 files changed, 37 insertions(+), 52 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index a3a93ae..3330e7d 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1815,40 +1815,6 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
>  }
>  #endif /* !_WIN32 */
>
> -int qemu_get_ram_fd(ram_addr_t addr)
> -{
> -    RAMBlock *block;
> -    int fd;
> -
> -    rcu_read_lock();
> -    block = qemu_get_ram_block(addr);
> -    fd = block->fd;
> -    rcu_read_unlock();
> -    return fd;
> -}
> -
> -void qemu_set_ram_fd(ram_addr_t addr, int fd)
> -{
> -    RAMBlock *block;
> -
> -    rcu_read_lock();
> -    block = qemu_get_ram_block(addr);
> -    block->fd = fd;
> -    rcu_read_unlock();
> -}
> -
> -void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
> -{
> -    RAMBlock *block;
> -    void *ptr;
> -
> -    rcu_read_lock();
> -    block = qemu_get_ram_block(addr);
> -    ptr = ramblock_ptr(block, 0);
> -    rcu_read_unlock();
> -    return ptr;
> -}
> -
>  /* Return a host pointer to ram allocated with qemu_ram_alloc.
>   * This should not be used for general purpose DMA.  Use address_space_map
>   * or address_space_rw instead. For local memory (e.g. video ram) that the
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index e40f23b..90be9f7 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -33,7 +33,6 @@
>  #include "sysemu/hostmem.h"
>  #include "sysemu/qtest.h"
>  #include "qapi/visitor.h"
> -#include "exec/ram_addr.h"
>
>  #include "hw/misc/ivshmem.h"
>
> @@ -533,7 +532,7 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
>      }
>      memory_region_init_ram_ptr(&s->server_bar2, OBJECT(s),
>                                 "ivshmem.bar2", size, ptr);
> -    qemu_set_ram_fd(memory_region_get_ram_addr(&s->server_bar2), fd);
> +    memory_region_set_fd(&s->server_bar2, fd);
>      s->ivshmem_bar2 = &s->server_bar2;
>  }
>
> @@ -940,7 +939,7 @@ static void ivshmem_exit(PCIDevice *dev)
>                               strerror(errno));
>              }
>
> -            fd = qemu_get_ram_fd(memory_region_get_ram_addr(s->ivshmem_bar2));
> +            fd = memory_region_get_fd(s->ivshmem_bar2);
>              close(fd);
>          }
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 5914e85..41908c0 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -248,17 +248,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
>      for (i = 0; i < dev->mem->nregions; ++i) {
>          struct vhost_memory_region *reg = dev->mem->regions + i;
>          ram_addr_t ram_addr;
> +        MemoryRegion *mr;
>
>          assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> -        qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
> -                                &ram_addr);
> -        fd = qemu_get_ram_fd(ram_addr);
> +        mr = qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
> +                                     &ram_addr);
> +        fd = memory_region_get_fd(mr);
>          if (fd > 0) {
>              msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
>              msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
>              msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
>              msg.payload.memory.regions[fd_num].mmap_offset = reg->userspace_addr -
> -                (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
> +                (uintptr_t) memory_region_get_ram_ptr(mr);
>              assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
>              fds[fd_num++] = fd;
>          }
> @@ -621,12 +622,10 @@ static bool vhost_user_can_merge(struct vhost_dev *dev,
>      MemoryRegion *mr;
>
>      mr = qemu_ram_addr_from_host((void *)(uintptr_t)start1, &ram_addr);
> -    assert(mr);
> -    mfd = qemu_get_ram_fd(ram_addr);
> +    mfd = memory_region_get_fd(mr);
>
>      mr = qemu_ram_addr_from_host((void *)(uintptr_t)start2, &ram_addr);
> -    assert(mr);
> -    rfd = qemu_get_ram_fd(ram_addr);
> +    rfd = memory_region_get_fd(mr);
>

You get rid of a few assert in the patch, any particular reason?

>      return mfd == rfd;
>  }
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index f649697..1678494 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -667,6 +667,17 @@ static inline bool memory_region_is_rom(MemoryRegion *mr)
>  int memory_region_get_fd(MemoryRegion *mr);
>
>  /**
> + * memory_region_set_fd: Mark a RAM memory region as backed by a
> + * file descriptor.
> + *
> + * This function is typically used after memory_region_init_ram_ptr().
> + *
> + * @mr: the memory region being queried.

...being updated

> + * @fd: the file descriptor that backs @mr.
> + */
> +void memory_region_set_fd(MemoryRegion *mr, int fd);
> +
> +/**
>   * memory_region_get_ram_ptr: Get a pointer into a RAM memory region.
>   *
>   * Returns a host pointer to a RAM memory region (created with
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 5b6e1b8..2a9465d 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -105,9 +105,6 @@ RAMBlock *qemu_ram_alloc_resizeable(ram_addr_t size, ram_addr_t max_size,
>                                                      uint64_t length,
>                                                      void *host),
>                                      MemoryRegion *mr, Error **errp);
> -int qemu_get_ram_fd(ram_addr_t addr);
> -void qemu_set_ram_fd(ram_addr_t addr, int fd);
> -void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
>  void qemu_ram_free(RAMBlock *block);
>
>  int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp);
> diff --git a/memory.c b/memory.c
> index 0f52522..d6a4a68 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1626,13 +1626,26 @@ void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
>
>  int memory_region_get_fd(MemoryRegion *mr)
>  {
> -    if (mr->alias) {
> -        return memory_region_get_fd(mr->alias);
> +    int fd;
> +
> +    rcu_read_lock();
> +    while (mr->alias) {
> +        mr = mr->alias;
>      }
> +    fd = mr->ram_block->fd;
> +    rcu_read_unlock();
>
> -    assert(mr->ram_block);
> +    return fd;
> +}
>
> -    return qemu_get_ram_fd(memory_region_get_ram_addr(mr));
> +void memory_region_set_fd(MemoryRegion *mr, int fd)
> +{
> +    rcu_read_lock();
> +    while (mr->alias) {
> +        mr = mr->alias;
> +    }
> +    mr->ram_block->fd = fd;
> +    rcu_read_unlock();
>  }
>
>  void *memory_region_get_ram_ptr(MemoryRegion *mr)
> --
> 2.5.5
>
>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>




-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/4] memory: remove qemu_get_ram_fd, qemu_set_ram_fd, qemu_ram_block_host_ptr

[Paolo Bonzini]

On 26/05/2016 14:22, Marc-André Lureau wrote:
> Hi
> 
> On Thu, May 26, 2016 at 10:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Remove direct uses of ram_addr_t and optimize memory_region_{get,set}_fd
>> now that a MemoryRegion knows its RAMBlock directly.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  exec.c                  | 34 ----------------------------------
>>  hw/misc/ivshmem.c       |  5 ++---
>>  hw/virtio/vhost-user.c  | 15 +++++++--------
>>  include/exec/memory.h   | 11 +++++++++++
>>  include/exec/ram_addr.h |  3 ---
>>  memory.c                | 21 +++++++++++++++++----
>>  6 files changed, 37 insertions(+), 52 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index a3a93ae..3330e7d 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1815,40 +1815,6 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
>>  }
>>  #endif /* !_WIN32 */
>>
>> -int qemu_get_ram_fd(ram_addr_t addr)
>> -{
>> -    RAMBlock *block;
>> -    int fd;
>> -
>> -    rcu_read_lock();
>> -    block = qemu_get_ram_block(addr);
>> -    fd = block->fd;
>> -    rcu_read_unlock();
>> -    return fd;
>> -}
>> -
>> -void qemu_set_ram_fd(ram_addr_t addr, int fd)
>> -{
>> -    RAMBlock *block;
>> -
>> -    rcu_read_lock();
>> -    block = qemu_get_ram_block(addr);
>> -    block->fd = fd;
>> -    rcu_read_unlock();
>> -}
>> -
>> -void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
>> -{
>> -    RAMBlock *block;
>> -    void *ptr;
>> -
>> -    rcu_read_lock();
>> -    block = qemu_get_ram_block(addr);
>> -    ptr = ramblock_ptr(block, 0);
>> -    rcu_read_unlock();
>> -    return ptr;
>> -}
>> -
>>  /* Return a host pointer to ram allocated with qemu_ram_alloc.
>>   * This should not be used for general purpose DMA.  Use address_space_map
>>   * or address_space_rw instead. For local memory (e.g. video ram) that the
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index e40f23b..90be9f7 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -33,7 +33,6 @@
>>  #include "sysemu/hostmem.h"
>>  #include "sysemu/qtest.h"
>>  #include "qapi/visitor.h"
>> -#include "exec/ram_addr.h"
>>
>>  #include "hw/misc/ivshmem.h"
>>
>> @@ -533,7 +532,7 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
>>      }
>>      memory_region_init_ram_ptr(&s->server_bar2, OBJECT(s),
>>                                 "ivshmem.bar2", size, ptr);
>> -    qemu_set_ram_fd(memory_region_get_ram_addr(&s->server_bar2), fd);
>> +    memory_region_set_fd(&s->server_bar2, fd);
>>      s->ivshmem_bar2 = &s->server_bar2;
>>  }
>>
>> @@ -940,7 +939,7 @@ static void ivshmem_exit(PCIDevice *dev)
>>                               strerror(errno));
>>              }
>>
>> -            fd = qemu_get_ram_fd(memory_region_get_ram_addr(s->ivshmem_bar2));
>> +            fd = memory_region_get_fd(s->ivshmem_bar2);
>>              close(fd);
>>          }
>>
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index 5914e85..41908c0 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -248,17 +248,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
>>      for (i = 0; i < dev->mem->nregions; ++i) {
>>          struct vhost_memory_region *reg = dev->mem->regions + i;
>>          ram_addr_t ram_addr;
>> +        MemoryRegion *mr;
>>
>>          assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
>> -        qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
>> -                                &ram_addr);
>> -        fd = qemu_get_ram_fd(ram_addr);
>> +        mr = qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
>> +                                     &ram_addr);
>> +        fd = memory_region_get_fd(mr);
>>          if (fd > 0) {
>>              msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
>>              msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
>>              msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
>>              msg.payload.memory.regions[fd_num].mmap_offset = reg->userspace_addr -
>> -                (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
>> +                (uintptr_t) memory_region_get_ram_ptr(mr);
>>              assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
>>              fds[fd_num++] = fd;
>>          }
>> @@ -621,12 +622,10 @@ static bool vhost_user_can_merge(struct vhost_dev *dev,
>>      MemoryRegion *mr;
>>
>>      mr = qemu_ram_addr_from_host((void *)(uintptr_t)start1, &ram_addr);
>> -    assert(mr);
>> -    mfd = qemu_get_ram_fd(ram_addr);
>> +    mfd = memory_region_get_fd(mr);
>>
>>      mr = qemu_ram_addr_from_host((void *)(uintptr_t)start2, &ram_addr);
>> -    assert(mr);
>> -    rfd = qemu_get_ram_fd(ram_addr);
>> +    rfd = memory_region_get_fd(mr);
>>
> 
> You get rid of a few assert in the patch, any particular reason?

I don't think it's useful to assert non-NULL on a pointer that is
dereferenced immediately after.  Before this patch, "mr" was unused and
the assertion checked for the validity of ram_addr.  Now, "mr" is passed
directly to memory_region_get_fd.

Thanks,

Paolo

[Qemu-devel] [PATCH 2/4] exec: remove ram_addr argument from qemu_ram_block_from_host

[Paolo Bonzini]

Of the two callers, one does not use it, and the other can compute
it itself based on the other output argument (offset) and the RAMBlock.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                    | 13 ++++++-------
 include/exec/cpu-common.h |  2 +-
 migration/postcopy-ram.c  |  3 +--
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/exec.c b/exec.c
index 3330e7d..65bad53 100644
--- a/exec.c
+++ b/exec.c
@@ -1897,16 +1897,16 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
  * ram_addr_t.
  */
 RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
-                                   ram_addr_t *ram_addr,
                                    ram_addr_t *offset)
 {
     RAMBlock *block;
     uint8_t *host = ptr;
 
     if (xen_enabled()) {
+        ram_addr_t ram_addr;
         rcu_read_lock();
-        *ram_addr = xen_ram_addr_from_mapcache(ptr);
-        block = qemu_get_ram_block(*ram_addr);
+        ram_addr = xen_ram_addr_from_mapcache(ptr);
+        block = qemu_get_ram_block(ram_addr);
         if (block) {
             *offset = (host - block->host);
         }
@@ -1938,7 +1938,6 @@ found:
     if (round_offset) {
         *offset &= TARGET_PAGE_MASK;
     }
-    *ram_addr = block->offset + *offset;
     rcu_read_unlock();
     return block;
 }
@@ -1968,10 +1967,10 @@ RAMBlock *qemu_ram_block_by_name(const char *name)
 MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
 {
     RAMBlock *block;
-    ram_addr_t offset; /* Not used */
-
-    block = qemu_ram_block_from_host(ptr, false, ram_addr, &offset);
+    ram_addr_t offset;
 
+    block = qemu_ram_block_from_host(ptr, false, &offset);
+    *ram_addr = block->offset + offset;
     if (!block) {
         return NULL;
     }
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index a2c3b92..fc609ad 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -60,7 +60,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
 MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
 RAMBlock *qemu_ram_block_by_name(const char *name);
 RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
-                                   ram_addr_t *ram_addr, ram_addr_t *offset);
+                                   ram_addr_t *offset);
 void qemu_ram_set_idstr(RAMBlock *block, const char *name, DeviceState *dev);
 void qemu_ram_unset_idstr(RAMBlock *block);
 const char *qemu_ram_get_idstr(RAMBlock *rb);
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index fbd0064..cf7dcd2 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -407,7 +407,6 @@ static void *postcopy_ram_fault_thread(void *opaque)
 
     while (true) {
         ram_addr_t rb_offset;
-        ram_addr_t in_raspace;
         struct pollfd pfd[2];
 
         /*
@@ -459,7 +458,7 @@ static void *postcopy_ram_fault_thread(void *opaque)
 
         rb = qemu_ram_block_from_host(
                  (void *)(uintptr_t)msg.arg.pagefault.address,
-                 true, &in_raspace, &rb_offset);
+                 true, &rb_offset);
         if (!rb) {
             error_report("postcopy_ram_fault_thread: Fault outside guest: %"
                          PRIx64, (uint64_t)msg.arg.pagefault.address);
-- 
2.5.5

Re: [Qemu-devel] [PATCH 2/4] exec: remove ram_addr argument from qemu_ram_block_from_host

[Marc-André Lureau]

Hi

On Thu, May 26, 2016 at 10:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Of the two callers, one does not use it, and the other can compute
> it itself based on the other output argument (offset) and the RAMBlock.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                    | 13 ++++++-------
>  include/exec/cpu-common.h |  2 +-
>  migration/postcopy-ram.c  |  3 +--
>  3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 3330e7d..65bad53 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1897,16 +1897,16 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
>   * ram_addr_t.
>   */
>  RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
> -                                   ram_addr_t *ram_addr,
>                                     ram_addr_t *offset)
>  {
>      RAMBlock *block;
>      uint8_t *host = ptr;
>
>      if (xen_enabled()) {
> +        ram_addr_t ram_addr;
>          rcu_read_lock();
> -        *ram_addr = xen_ram_addr_from_mapcache(ptr);
> -        block = qemu_get_ram_block(*ram_addr);
> +        ram_addr = xen_ram_addr_from_mapcache(ptr);
> +        block = qemu_get_ram_block(ram_addr);
>          if (block) {
>              *offset = (host - block->host);
>          }
> @@ -1938,7 +1938,6 @@ found:
>      if (round_offset) {
>          *offset &= TARGET_PAGE_MASK;
>      }
> -    *ram_addr = block->offset + *offset;
>      rcu_read_unlock();
>      return block;
>  }
> @@ -1968,10 +1967,10 @@ RAMBlock *qemu_ram_block_by_name(const char *name)
>  MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
>  {
>      RAMBlock *block;
> -    ram_addr_t offset; /* Not used */
> -
> -    block = qemu_ram_block_from_host(ptr, false, ram_addr, &offset);
> +    ram_addr_t offset;
>
> +    block = qemu_ram_block_from_host(ptr, false, &offset);
> +    *ram_addr = block->offset + offset;
>      if (!block) {
>          return NULL;
>      }
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index a2c3b92..fc609ad 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -60,7 +60,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
>  MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
>  RAMBlock *qemu_ram_block_by_name(const char *name);
>  RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
> -                                   ram_addr_t *ram_addr, ram_addr_t *offset);
> +                                   ram_addr_t *offset);
>  void qemu_ram_set_idstr(RAMBlock *block, const char *name, DeviceState *dev);
>  void qemu_ram_unset_idstr(RAMBlock *block);
>  const char *qemu_ram_get_idstr(RAMBlock *rb);
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index fbd0064..cf7dcd2 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -407,7 +407,6 @@ static void *postcopy_ram_fault_thread(void *opaque)
>
>      while (true) {
>          ram_addr_t rb_offset;
> -        ram_addr_t in_raspace;
>          struct pollfd pfd[2];
>
>          /*
> @@ -459,7 +458,7 @@ static void *postcopy_ram_fault_thread(void *opaque)
>
>          rb = qemu_ram_block_from_host(
>                   (void *)(uintptr_t)msg.arg.pagefault.address,
> -                 true, &in_raspace, &rb_offset);
> +                 true, &rb_offset);
>          if (!rb) {
>              error_report("postcopy_ram_fault_thread: Fault outside guest: %"
>                           PRIx64, (uint64_t)msg.arg.pagefault.address);
> --
> 2.5.5
>
>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



-- 
Marc-André Lureau

[Qemu-devel] [PATCH 3/4] memory: split memory_region_from_host from qemu_ram_addr_from_host

[Paolo Bonzini]

Move the old qemu_ram_addr_from_host to memory_region_from_host and
make it return an offset within the region.  For qemu_ram_addr_from_host
return the ram_addr_t directly, similar to what it was before
commit 1b5ec23 ("memory: return MemoryRegion from qemu_ram_addr_from_host",
2013-07-04).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cputlb.c                  |  3 ++-
 exec.c                    | 10 +++++-----
 hw/virtio/vhost-user.c    | 16 +++++++---------
 include/exec/cpu-common.h |  2 +-
 include/exec/memory.h     | 20 ++++++++++++++++++++
 memory.c                  | 14 ++++++++++++--
 target-i386/kvm.c         |  6 ++++--
 7 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index 1ff6354..23c9b91 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -246,7 +246,8 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
 {
     ram_addr_t ram_addr;
 
-    if (qemu_ram_addr_from_host(ptr, &ram_addr) == NULL) {
+    ram_addr = qemu_ram_addr_from_host(ptr);
+    if (ram_addr == RAM_ADDR_INVALID) {
         fprintf(stderr, "Bad ram pointer %p\n", ptr);
         abort();
     }
diff --git a/exec.c b/exec.c
index 65bad53..7f62835 100644
--- a/exec.c
+++ b/exec.c
@@ -1964,18 +1964,17 @@ RAMBlock *qemu_ram_block_by_name(const char *name)
 
 /* Some of the softmmu routines need to translate from a host pointer
    (typically a TLB entry) back to a ram offset.  */
-MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
+ram_addr_t qemu_ram_addr_from_host(void *ptr)
 {
     RAMBlock *block;
     ram_addr_t offset;
 
     block = qemu_ram_block_from_host(ptr, false, &offset);
-    *ram_addr = block->offset + offset;
     if (!block) {
-        return NULL;
+        return RAM_ADDR_INVALID;
     }
 
-    return block->mr;
+    return block->offset + offset;
 }
 
 /* Called within RCU critical section.  */
@@ -2975,8 +2974,9 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
         MemoryRegion *mr;
         ram_addr_t addr1;
 
-        mr = qemu_ram_addr_from_host(buffer, &addr1);
+        mr = memory_region_from_host(buffer, &addr1);
         assert(mr != NULL);
+        addr1 += memory_region_get_ram_addr(mr);
         if (is_write) {
             invalidate_and_set_dirty(mr, addr1, access_len);
         }
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 41908c0..495e09f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -17,7 +17,6 @@
 #include "sysemu/kvm.h"
 #include "qemu/error-report.h"
 #include "qemu/sockets.h"
-#include "exec/ram_addr.h"
 #include "migration/migration.h"
 
 #include <sys/ioctl.h>
@@ -247,19 +246,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
 
     for (i = 0; i < dev->mem->nregions; ++i) {
         struct vhost_memory_region *reg = dev->mem->regions + i;
-        ram_addr_t ram_addr;
+        ram_addr_t offset;
         MemoryRegion *mr;
 
         assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
-        mr = qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
-                                     &ram_addr);
+        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
+                                     &offset);
         fd = memory_region_get_fd(mr);
         if (fd > 0) {
             msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
             msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
             msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
-            msg.payload.memory.regions[fd_num].mmap_offset = reg->userspace_addr -
-                (uintptr_t) memory_region_get_ram_ptr(mr);
+            msg.payload.memory.regions[fd_num].mmap_offset = offset;
             assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
             fds[fd_num++] = fd;
         }
@@ -617,14 +615,14 @@ static bool vhost_user_can_merge(struct vhost_dev *dev,
                                  uint64_t start1, uint64_t size1,
                                  uint64_t start2, uint64_t size2)
 {
-    ram_addr_t ram_addr;
+    ram_addr_t offset;
     int mfd, rfd;
     MemoryRegion *mr;
 
-    mr = qemu_ram_addr_from_host((void *)(uintptr_t)start1, &ram_addr);
+    mr = memory_region_from_host((void *)(uintptr_t)start1, &offset);
     mfd = memory_region_get_fd(mr);
 
-    mr = qemu_ram_addr_from_host((void *)(uintptr_t)start2, &ram_addr);
+    mr = memory_region_from_host((void *)(uintptr_t)start2, &offset);
     rfd = memory_region_get_fd(mr);
 
     return mfd == rfd;
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index fc609ad..aaee995 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -57,7 +57,7 @@ typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
 
 void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
 /* This should not be used by devices.  */
-MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
+ram_addr_t qemu_ram_addr_from_host(void *ptr);
 RAMBlock *qemu_ram_block_by_name(const char *name);
 RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
                                    ram_addr_t *offset);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 1678494..71a27ab 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -32,6 +32,8 @@
 #include "qom/object.h"
 #include "qemu/rcu.h"
 
+#define RAM_ADDR_INVALID (~(ram_addr_t)0)
+
 #define MAX_PHYS_ADDR_SPACE_BITS 62
 #define MAX_PHYS_ADDR            (((hwaddr)1 << MAX_PHYS_ADDR_SPACE_BITS) - 1)
 
@@ -678,6 +680,24 @@ int memory_region_get_fd(MemoryRegion *mr);
 void memory_region_set_fd(MemoryRegion *mr, int fd);
 
 /**
+ * memory_region_from_host: Convert a pointer into a RAM memory region
+ * and an offset within it.
+ *
+ * Given a host pointer inside a RAM memory region (created with
+ * memory_region_init_ram() or memory_region_init_ram_ptr()), return
+ * the MemoryRegion and the offset within it.
+ *
+ * Use with care; by the time this function returns, the returned pointer is
+ * not protected by RCU anymore.  If the caller is not within an RCU critical
+ * section and does not hold the iothread lock, it must have other means of
+ * protecting the pointer, such as a reference to the region that includes
+ * the incoming ram_addr_t.
+ *
+ * @mr: the memory region being queried.
+ */
+MemoryRegion *memory_region_from_host(void *ptr, ram_addr_t *offset);
+
+/**
  * memory_region_get_ram_ptr: Get a pointer into a RAM memory region.
  *
  * Returns a host pointer to a RAM memory region (created with
diff --git a/memory.c b/memory.c
index d6a4a68..f8085ea 100644
--- a/memory.c
+++ b/memory.c
@@ -33,8 +33,6 @@
 
 //#define DEBUG_UNASSIGNED
 
-#define RAM_ADDR_INVALID (~(ram_addr_t)0)
-
 static unsigned memory_region_transaction_depth;
 static bool memory_region_update_pending;
 static bool ioeventfd_update_pending;
@@ -1665,6 +1663,18 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
     return ptr + offset;
 }
 
+MemoryRegion *memory_region_from_host(void *ptr, ram_addr_t *offset)
+{
+    RAMBlock *block;
+
+    block = qemu_ram_block_from_host(ptr, false, offset);
+    if (!block) {
+        return NULL;
+    }
+
+    return block->mr;
+}
+
 ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
 {
     return mr->ram_block ? mr->ram_block->offset : RAM_ADDR_INVALID;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 7b3667a..abf50e6 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -411,7 +411,8 @@ int kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
 
     if ((env->mcg_cap & MCG_SER_P) && addr
         && (code == BUS_MCEERR_AR || code == BUS_MCEERR_AO)) {
-        if (qemu_ram_addr_from_host(addr, &ram_addr) == NULL ||
+        ram_addr = qemu_ram_addr_from_host(addr);
+        if (ram_addr == RAM_ADDR_INVALID ||
             !kvm_physical_memory_addr_from_host(c->kvm_state, addr, &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "
                     "QEMU itself instead of guest system!\n");
@@ -445,7 +446,8 @@ int kvm_arch_on_sigbus(int code, void *addr)
         hwaddr paddr;
 
         /* Hope we are lucky for AO MCE */
-        if (qemu_ram_addr_from_host(addr, &ram_addr) == NULL ||
+        ram_addr = qemu_ram_addr_from_host(addr);
+        if (ram_addr == RAM_ADDR_INVALID ||
             !kvm_physical_memory_addr_from_host(first_cpu->kvm_state,
                                                 addr, &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "
-- 
2.5.5

Re: [Qemu-devel] [PATCH 3/4] memory: split memory_region_from_host from qemu_ram_addr_from_host

[Marc-André Lureau]

Hi

On Thu, May 26, 2016 at 10:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Move the old qemu_ram_addr_from_host to memory_region_from_host and
> make it return an offset within the region.  For qemu_ram_addr_from_host
> return the ram_addr_t directly, similar to what it was before
> commit 1b5ec23 ("memory: return MemoryRegion from qemu_ram_addr_from_host",
> 2013-07-04).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cputlb.c                  |  3 ++-
>  exec.c                    | 10 +++++-----
>  hw/virtio/vhost-user.c    | 16 +++++++---------
>  include/exec/cpu-common.h |  2 +-
>  include/exec/memory.h     | 20 ++++++++++++++++++++
>  memory.c                  | 14 ++++++++++++--
>  target-i386/kvm.c         |  6 ++++--
>  7 files changed, 51 insertions(+), 20 deletions(-)
>
> diff --git a/cputlb.c b/cputlb.c
> index 1ff6354..23c9b91 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -246,7 +246,8 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>  {
>      ram_addr_t ram_addr;
>
> -    if (qemu_ram_addr_from_host(ptr, &ram_addr) == NULL) {
> +    ram_addr = qemu_ram_addr_from_host(ptr);
> +    if (ram_addr == RAM_ADDR_INVALID) {
>          fprintf(stderr, "Bad ram pointer %p\n", ptr);
>          abort();
>      }
> diff --git a/exec.c b/exec.c
> index 65bad53..7f62835 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1964,18 +1964,17 @@ RAMBlock *qemu_ram_block_by_name(const char *name)
>
>  /* Some of the softmmu routines need to translate from a host pointer
>     (typically a TLB entry) back to a ram offset.  */
> -MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
> +ram_addr_t qemu_ram_addr_from_host(void *ptr)
>  {
>      RAMBlock *block;
>      ram_addr_t offset;
>
>      block = qemu_ram_block_from_host(ptr, false, &offset);
> -    *ram_addr = block->offset + offset;
>      if (!block) {
> -        return NULL;
> +        return RAM_ADDR_INVALID;
>      }
>
> -    return block->mr;
> +    return block->offset + offset;
>  }
>
>  /* Called within RCU critical section.  */
> @@ -2975,8 +2974,9 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>          MemoryRegion *mr;
>          ram_addr_t addr1;
>
> -        mr = qemu_ram_addr_from_host(buffer, &addr1);
> +        mr = memory_region_from_host(buffer, &addr1);
>          assert(mr != NULL);
> +        addr1 += memory_region_get_ram_addr(mr);
>          if (is_write) {
>              invalidate_and_set_dirty(mr, addr1, access_len);
>          }
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 41908c0..495e09f 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -17,7 +17,6 @@
>  #include "sysemu/kvm.h"
>  #include "qemu/error-report.h"
>  #include "qemu/sockets.h"
> -#include "exec/ram_addr.h"
>  #include "migration/migration.h"
>
>  #include <sys/ioctl.h>
> @@ -247,19 +246,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
>
>      for (i = 0; i < dev->mem->nregions; ++i) {
>          struct vhost_memory_region *reg = dev->mem->regions + i;
> -        ram_addr_t ram_addr;
> +        ram_addr_t offset;
>          MemoryRegion *mr;
>
>          assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> -        mr = qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
> -                                     &ram_addr);
> +        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
> +                                     &offset);
>          fd = memory_region_get_fd(mr);
>          if (fd > 0) {
>              msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
>              msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
>              msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
> -            msg.payload.memory.regions[fd_num].mmap_offset = reg->userspace_addr -
> -                (uintptr_t) memory_region_get_ram_ptr(mr);
> +            msg.payload.memory.regions[fd_num].mmap_offset = offset;
>              assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
>              fds[fd_num++] = fd;
>          }
> @@ -617,14 +615,14 @@ static bool vhost_user_can_merge(struct vhost_dev *dev,
>                                   uint64_t start1, uint64_t size1,
>                                   uint64_t start2, uint64_t size2)
>  {
> -    ram_addr_t ram_addr;
> +    ram_addr_t offset;
>      int mfd, rfd;
>      MemoryRegion *mr;
>
> -    mr = qemu_ram_addr_from_host((void *)(uintptr_t)start1, &ram_addr);
> +    mr = memory_region_from_host((void *)(uintptr_t)start1, &offset);
>      mfd = memory_region_get_fd(mr);
>
> -    mr = qemu_ram_addr_from_host((void *)(uintptr_t)start2, &ram_addr);
> +    mr = memory_region_from_host((void *)(uintptr_t)start2, &offset);
>      rfd = memory_region_get_fd(mr);
>
>      return mfd == rfd;
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index fc609ad..aaee995 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -57,7 +57,7 @@ typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
>
>  void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
>  /* This should not be used by devices.  */
> -MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
> +ram_addr_t qemu_ram_addr_from_host(void *ptr);
>  RAMBlock *qemu_ram_block_by_name(const char *name);
>  RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
>                                     ram_addr_t *offset);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 1678494..71a27ab 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -32,6 +32,8 @@
>  #include "qom/object.h"
>  #include "qemu/rcu.h"
>
> +#define RAM_ADDR_INVALID (~(ram_addr_t)0)
> +
>  #define MAX_PHYS_ADDR_SPACE_BITS 62
>  #define MAX_PHYS_ADDR            (((hwaddr)1 << MAX_PHYS_ADDR_SPACE_BITS) - 1)
>
> @@ -678,6 +680,24 @@ int memory_region_get_fd(MemoryRegion *mr);
>  void memory_region_set_fd(MemoryRegion *mr, int fd);
>
>  /**
> + * memory_region_from_host: Convert a pointer into a RAM memory region
> + * and an offset within it.
> + *
> + * Given a host pointer inside a RAM memory region (created with
> + * memory_region_init_ram() or memory_region_init_ram_ptr()), return
> + * the MemoryRegion and the offset within it.
> + *
> + * Use with care; by the time this function returns, the returned pointer is
> + * not protected by RCU anymore.  If the caller is not within an RCU critical
> + * section and does not hold the iothread lock, it must have other means of
> + * protecting the pointer, such as a reference to the region that includes
> + * the incoming ram_addr_t.
> + *
> + * @mr: the memory region being queried.

@ptr: host pointer to look up
@offset: set to result offset within the MemoryRegion

> + */
> +MemoryRegion *memory_region_from_host(void *ptr, ram_addr_t *offset);
> +
> +/**
>   * memory_region_get_ram_ptr: Get a pointer into a RAM memory region.
>   *
>   * Returns a host pointer to a RAM memory region (created with
> diff --git a/memory.c b/memory.c
> index d6a4a68..f8085ea 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -33,8 +33,6 @@
>
>  //#define DEBUG_UNASSIGNED
>
> -#define RAM_ADDR_INVALID (~(ram_addr_t)0)
> -
>  static unsigned memory_region_transaction_depth;
>  static bool memory_region_update_pending;
>  static bool ioeventfd_update_pending;
> @@ -1665,6 +1663,18 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
>      return ptr + offset;
>  }
>
> +MemoryRegion *memory_region_from_host(void *ptr, ram_addr_t *offset)
> +{
> +    RAMBlock *block;
> +
> +    block = qemu_ram_block_from_host(ptr, false, offset);
> +    if (!block) {
> +        return NULL;
> +    }
> +
> +    return block->mr;
> +}
> +
>  ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
>  {
>      return mr->ram_block ? mr->ram_block->offset : RAM_ADDR_INVALID;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 7b3667a..abf50e6 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -411,7 +411,8 @@ int kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
>
>      if ((env->mcg_cap & MCG_SER_P) && addr
>          && (code == BUS_MCEERR_AR || code == BUS_MCEERR_AO)) {
> -        if (qemu_ram_addr_from_host(addr, &ram_addr) == NULL ||
> +        ram_addr = qemu_ram_addr_from_host(addr);
> +        if (ram_addr == RAM_ADDR_INVALID ||
>              !kvm_physical_memory_addr_from_host(c->kvm_state, addr, &paddr)) {
>              fprintf(stderr, "Hardware memory error for memory used by "
>                      "QEMU itself instead of guest system!\n");
> @@ -445,7 +446,8 @@ int kvm_arch_on_sigbus(int code, void *addr)
>          hwaddr paddr;
>
>          /* Hope we are lucky for AO MCE */
> -        if (qemu_ram_addr_from_host(addr, &ram_addr) == NULL ||
> +        ram_addr = qemu_ram_addr_from_host(addr);
> +        if (ram_addr == RAM_ADDR_INVALID ||
>              !kvm_physical_memory_addr_from_host(first_cpu->kvm_state,
>                                                  addr, &paddr)) {
>              fprintf(stderr, "Hardware memory error for memory used by "
> --
> 2.5.5
>
>
>


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



-- 
Marc-André Lureau

[Qemu-devel] [PATCH 4/4] exec: hide mr->ram_addr from qemu_get_ram_ptr users

[Paolo Bonzini]

Let users of qemu_get_ram_ptr and qemu_ram_ptr_length pass in an
address that is relative to the MemoryRegion.  This basically means
what address_space_translate returns.

Because the semantics of the second parameter change, rename the
function to qemu_map_ram_ptr.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                       | 57 +++++++++++++++++++-------------------------
 include/exec/memory.h        |  5 ++--
 memory.c                     |  4 ++--
 scripts/dump-guest-memory.py | 19 +++------------
 4 files changed, 31 insertions(+), 54 deletions(-)

diff --git a/exec.c b/exec.c
index 7f62835..4488821 100644
--- a/exec.c
+++ b/exec.c
@@ -1822,12 +1822,13 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
  *
  * Called within RCU critical section.
  */
-void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
+void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
 {
     RAMBlock *block = ram_block;
 
     if (block == NULL) {
         block = qemu_get_ram_block(addr);
+        addr -= block->offset;
     }
 
     if (xen_enabled() && block->host == NULL) {
@@ -1841,10 +1842,10 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
 
         block->host = xen_map_cache(block->offset, block->max_length, 1);
     }
-    return ramblock_ptr(block, addr - block->offset);
+    return ramblock_ptr(block, addr);
 }
 
-/* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
+/* Return a host pointer to guest's ram. Similar to qemu_map_ram_ptr
  * but takes a size argument.
  *
  * Called within RCU critical section.
@@ -1853,16 +1854,15 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
                                  hwaddr *size)
 {
     RAMBlock *block = ram_block;
-    ram_addr_t offset_inside_block;
     if (*size == 0) {
         return NULL;
     }
 
     if (block == NULL) {
         block = qemu_get_ram_block(addr);
+        addr -= block->offset;
     }
-    offset_inside_block = addr - block->offset;
-    *size = MIN(*size, block->max_length - offset_inside_block);
+    *size = MIN(*size, block->max_length - addr);
 
     if (xen_enabled() && block->host == NULL) {
         /* We need to check if the requested address is in the RAM
@@ -1876,7 +1876,7 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
         block->host = xen_map_cache(block->offset, block->max_length, 1);
     }
 
-    return ramblock_ptr(block, offset_inside_block);
+    return ramblock_ptr(block, addr);
 }
 
 /*
@@ -1986,13 +1986,13 @@ static void notdirty_mem_write(void *opaque, hwaddr ram_addr,
     }
     switch (size) {
     case 1:
-        stb_p(qemu_get_ram_ptr(NULL, ram_addr), val);
+        stb_p(qemu_map_ram_ptr(NULL, ram_addr), val);
         break;
     case 2:
-        stw_p(qemu_get_ram_ptr(NULL, ram_addr), val);
+        stw_p(qemu_map_ram_ptr(NULL, ram_addr), val);
         break;
     case 4:
-        stl_p(qemu_get_ram_ptr(NULL, ram_addr), val);
+        stl_p(qemu_map_ram_ptr(NULL, ram_addr), val);
         break;
     default:
         abort();
@@ -2454,6 +2454,8 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
                                      hwaddr length)
 {
     uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
+    addr += memory_region_get_ram_addr(mr);
+
     /* No early return if dirty_log_mask is or becomes 0, because
      * cpu_physical_memory_set_dirty_range will still call
      * xen_modified_memory.
@@ -2566,9 +2568,8 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
                 abort();
             }
         } else {
-            addr1 += memory_region_get_ram_addr(mr);
             /* RAM case */
-            ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
+            ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
             memcpy(ptr, buf, l);
             invalidate_and_set_dirty(mr, addr1, l);
         }
@@ -2659,8 +2660,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
             }
         } else {
             /* RAM case */
-            ptr = qemu_get_ram_ptr(mr->ram_block,
-                                   memory_region_get_ram_addr(mr) + addr1);
+            ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
             memcpy(buf, ptr, l);
         }
 
@@ -2743,9 +2743,8 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
               memory_region_is_romd(mr))) {
             l = memory_access_size(mr, l, addr1);
         } else {
-            addr1 += memory_region_get_ram_addr(mr);
             /* ROM/RAM case */
-            ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
+            ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
             switch (type) {
             case WRITE_DATA:
                 memcpy(ptr, buf, l);
@@ -2903,7 +2902,6 @@ void *address_space_map(AddressSpace *as,
     hwaddr done = 0;
     hwaddr l, xlat, base;
     MemoryRegion *mr, *this_mr;
-    ram_addr_t raddr;
     void *ptr;
 
     if (len == 0) {
@@ -2938,7 +2936,6 @@ void *address_space_map(AddressSpace *as,
     }
 
     base = xlat;
-    raddr = memory_region_get_ram_addr(mr);
 
     for (;;) {
         len -= l;
@@ -2957,7 +2954,7 @@ void *address_space_map(AddressSpace *as,
 
     memory_region_ref(mr);
     *plen = done;
-    ptr = qemu_ram_ptr_length(mr->ram_block, raddr + base, plen);
+    ptr = qemu_ram_ptr_length(mr->ram_block, base, plen);
     rcu_read_unlock();
 
     return ptr;
@@ -2976,7 +2973,6 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
 
         mr = memory_region_from_host(buffer, &addr1);
         assert(mr != NULL);
-        addr1 += memory_region_get_ram_addr(mr);
         if (is_write) {
             invalidate_and_set_dirty(mr, addr1, access_len);
         }
@@ -3042,8 +3038,7 @@ static inline uint32_t address_space_ldl_internal(AddressSpace *as, hwaddr addr,
 #endif
     } else {
         /* RAM case */
-        ptr = qemu_get_ram_ptr(mr->ram_block,
-                               memory_region_get_ram_addr(mr) + addr1);
+        ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = ldl_le_p(ptr);
@@ -3136,8 +3131,7 @@ static inline uint64_t address_space_ldq_internal(AddressSpace *as, hwaddr addr,
 #endif
     } else {
         /* RAM case */
-        ptr = qemu_get_ram_ptr(mr->ram_block,
-                               memory_region_get_ram_addr(mr) + addr1);
+        ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = ldq_le_p(ptr);
@@ -3250,8 +3244,7 @@ static inline uint32_t address_space_lduw_internal(AddressSpace *as,
 #endif
     } else {
         /* RAM case */
-        ptr = qemu_get_ram_ptr(mr->ram_block,
-                               memory_region_get_ram_addr(mr) + addr1);
+        ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = lduw_le_p(ptr);
@@ -3333,13 +3326,13 @@ void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
 
         r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
     } else {
-        addr1 += memory_region_get_ram_addr(mr);
-        ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
+        ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         stl_p(ptr, val);
 
         dirty_log_mask = memory_region_get_dirty_log_mask(mr);
         dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
-        cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
+        cpu_physical_memory_set_dirty_range(memory_region_get_ram_addr(mr) + addr,
+                                            4, dirty_log_mask);
         r = MEMTX_OK;
     }
     if (result) {
@@ -3388,8 +3381,7 @@ static inline void address_space_stl_internal(AddressSpace *as,
         r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
     } else {
         /* RAM case */
-        addr1 += memory_region_get_ram_addr(mr);
-        ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
+        ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             stl_le_p(ptr, val);
@@ -3498,8 +3490,7 @@ static inline void address_space_stw_internal(AddressSpace *as,
         r = memory_region_dispatch_write(mr, addr1, val, 2, attrs);
     } else {
         /* RAM case */
-        addr1 += memory_region_get_ram_addr(mr);
-        ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
+        ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             stw_le_p(ptr, val);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 71a27ab..4ab6800 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1393,7 +1393,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
 					MemoryRegion *mr);
 MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
                                     MemTxAttrs attrs, uint8_t *buf, int len);
-void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
+void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
 
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
@@ -1431,8 +1431,7 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
             l = len;
             mr = address_space_translate(as, addr, &addr1, &l, false);
             if (len == l && memory_access_is_direct(mr, false)) {
-                addr1 += memory_region_get_ram_addr(mr);
-                ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
+                ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
                 memcpy(buf, ptr, len);
             } else {
                 result = address_space_read_continue(as, addr, attrs, buf, len,
diff --git a/memory.c b/memory.c
index f8085ea..8ba496d 100644
--- a/memory.c
+++ b/memory.c
@@ -1657,10 +1657,10 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
         mr = mr->alias;
     }
     assert(mr->ram_block);
-    ptr = qemu_get_ram_ptr(mr->ram_block, memory_region_get_ram_addr(mr));
+    ptr = qemu_map_ram_ptr(mr->ram_block, offset);
     rcu_read_unlock();
 
-    return ptr + offset;
+    return ptr;
 }
 
 MemoryRegion *memory_region_from_host(void *ptr, ram_addr_t *offset)
diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
index eb24f78..9956fc0 100644
--- a/scripts/dump-guest-memory.py
+++ b/scripts/dump-guest-memory.py
@@ -328,23 +328,10 @@ def qlist_foreach(head, field_str):
         yield var
 
 
-def qemu_get_ram_block(ram_addr):
-    """Returns the RAMBlock struct to which the given address belongs."""
-
-    ram_blocks = gdb.parse_and_eval("ram_list.blocks")
-
-    for block in qlist_foreach(ram_blocks, "next"):
-        if (ram_addr - block["offset"]) < block["used_length"]:
-            return block
-
-    raise gdb.GdbError("Bad ram offset %x" % ram_addr)
-
-
-def qemu_get_ram_ptr(ram_addr):
+def qemu_map_ram_ptr(block, offset):
     """Returns qemu vaddr for given guest physical address."""
 
-    block = qemu_get_ram_block(ram_addr)
-    return block["host"] + (ram_addr - block["offset"])
+    return block["host"] + offset
 
 
 def memory_region_get_ram_ptr(memory_region):
@@ -352,7 +339,7 @@ def memory_region_get_ram_ptr(memory_region):
         return (memory_region_get_ram_ptr(memory_region["alias"].dereference())
                 + memory_region["alias_offset"])
 
-    return qemu_get_ram_ptr(memory_region["ram_block"]["offset"])
+    return qemu_map_ram_ptr(memory_region["ram_block"], 0)
 
 
 def get_guest_phys_blocks():
-- 
2.5.5