Re: [Qemu-devel] [PATCH v3] vhost-user: fix regions provied with VHOST_USER_SET_MEM_TABLE message

[Michael S. Tsirkin]

On Thu, Jun 26, 2014 at 11:01:32PM +0200, Damjan Marion wrote:
> Old code was affected by memory gaps which resulted in buffer pointers
> pointing to address outside of the mapped regions.
> 
> Here we are introducing following changes:
>  - new function qemu_get_ram_block_host_ptr() returns host pointer
>    to the ram block, it is needed to calculate offset of specific
>    region in the host memory
>  - new field mmap_offset is added to the VhostUserMemoryRegion. It
>    contains offset where specific region starts in the mapped memory.
>    As there is stil no wider adoption of vhost-user agreement was made
>    that we will not bump version number due to this change
>  - other fileds in VhostUserMemoryRegion struct are not changed, as
>    they are all needed for usermode app implementation
>  - region data is not taken from ram_list.blocks anymore, instead we
>    use region data which is alredy calculated for use in vhost-net
>  - Now multiple regions can have same FD and user applicaton can call
>    mmap() multiple times with the same FD but with different offset
>    (user needs to take care for offset page alignment)
> 
> Signed-off-by: Damjan Marion <address@hidden>
> ---
>  docs/specs/vhost-user.txt |  7 ++++---
>  exec.c                    |  7 +++++++
>  hw/virtio/vhost-user.c    | 23 ++++++++++++++---------
>  include/exec/ram_addr.h   |  1 +
>  4 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 2641390..6abb697 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -78,13 +78,14 @@ Depending on the request type, payload can be:
>     Padding: 32-bit
>  
>     A region is:
> -   ---------------------------------------
> -   | guest address | size | user address |
> -   ---------------------------------------
> +   -----------------------------------------------------
> +   | guest address | size | user address | mmap offset |
> +   -----------------------------------------------------
>  
>     Guest address: a 64-bit guest address of the region
>     Size: a 64-bit size
>     User address: a 64-bit user address
> +   mmmap offset: 64-bit offset where region starts in the mapped memory

mmap I guess?

>  
>  
>  In QEMU the vhost-user message is implemented with the following struct:
> diff --git a/exec.c b/exec.c
> index c849405..a94c583 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1456,6 +1456,13 @@ int qemu_get_ram_fd(ram_addr_t addr)
>      return block->fd;
>  }
>  
> +void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
> +{
> +    RAMBlock *block = qemu_get_ram_block(addr);
> +
> +    return block->host;
> +}
> +
>  /* Return a host pointer to ram allocated with qemu_ram_alloc.
>     With the exception of the softmmu code in this file, this should
>     only be used for local memory (e.g. video ram) that the device owns,
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 0df6a93..38e5806 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -14,6 +14,7 @@
>  #include "sysemu/kvm.h"
>  #include "qemu/error-report.h"
>  #include "qemu/sockets.h"
> +#include "exec/ram_addr.h"
>  
>  #include <fcntl.h>
>  #include <unistd.h>
> @@ -47,6 +48,7 @@ typedef struct VhostUserMemoryRegion {
>      uint64_t guest_phys_addr;
>      uint64_t memory_size;
>      uint64_t userspace_addr;
> +    uint64_t mmap_offset;
>  } VhostUserMemoryRegion;
>  
>  typedef struct VhostUserMemory {
> @@ -183,10 +185,10 @@ static int vhost_user_call(struct vhost_dev *dev, 
> unsigned long int request,
>  {
>      VhostUserMsg msg;
>      VhostUserRequest msg_request;
> -    RAMBlock *block = 0;
>      struct vhost_vring_file *file = 0;
>      int need_reply = 0;
>      int fds[VHOST_MEMORY_MAX_NREGIONS];
> +    int i, fd;
>      size_t fd_num = 0;
>  
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> @@ -212,14 +214,17 @@ static int vhost_user_call(struct vhost_dev *dev, 
> unsigned long int request,
>          break;
>  
>      case VHOST_SET_MEM_TABLE:
> -        QTAILQ_FOREACH(block, &ram_list.blocks, next)
> -        {
> -            if (block->fd > 0) {
> -                msg.memory.regions[fd_num].userspace_addr =
> -                    (uintptr_t) block->host;
> -                msg.memory.regions[fd_num].memory_size = block->length;
> -                msg.memory.regions[fd_num].guest_phys_addr = block->offset;
> -                fds[fd_num++] = block->fd;
> +        for (i = 0; i < dev->mem->nregions; ++i) {
> +            struct vhost_memory_region *reg = dev->mem->regions + i;
> +            fd = qemu_get_ram_fd(reg->guest_phys_addr);
> +            if (fd > 0) {
> +                msg.memory.regions[fd_num].userspace_addr = 
> reg->userspace_addr;
> +                msg.memory.regions[fd_num].memory_size  = reg->memory_size;
> +                msg.memory.regions[fd_num].guest_phys_addr = 
> reg->guest_phys_addr;
> +                msg.memory.regions[fd_num].mmap_offset = reg->userspace_addr 
> -
> +                    (uintptr_t) 
> qemu_get_ram_block_host_ptr(reg->guest_phys_addr);
> +                assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> +                fds[fd_num++] = fd;
>              }
>          }
>  
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 55ca676..e9eb831 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -29,6 +29,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void 
> *host,
>                                     MemoryRegion *mr);
>  ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr);
>  int qemu_get_ram_fd(ram_addr_t addr);
> +void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
>  void *qemu_get_ram_ptr(ram_addr_t addr);
>  void qemu_ram_free(ram_addr_t addr);
>  void qemu_ram_free_from_ptr(ram_addr_t addr);
> -- 
> 1.9.1