qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH 1/4] memory: remove qemu_get_ram_fd, qemu_set_ram_fd, qemu_ram_block_host_ptr
Date: Thu, 26 May 2016 17:37:32 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]