On 26/05/2016 14:22, Marc-André Lureau wrote:
> Hi
>
> On Thu, May 26, 2016 at 10:49 AM, Paolo Bonzini <pbonz...@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 <pbonz...@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
