| Message ID | 1464252584-30832-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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)
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(-)