qemu-devel
[Top][All Lists]
Advanced

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

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


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCH v3] vhost-user: fix regions provied with VHOST_USER_SET_MEM_TABLE message
Date: Fri, 27 Jun 2014 17:22:41 +0300

On Fri, Jun 27, 2014 at 08:02:48AM +0300, Nikolay Nikolaev wrote:
> 
> 
> 
> On Fri, Jun 27, 2014 at 12:01 AM, Damjan Marion <address@hidden> 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
> 
> 
>      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
> 
> 
> 
> Looks OK to me. Thanks Damjan.
> 
> Acked-By: Nikolay Nikolaev <address@hidden>
> 


Nikolay, please don't send HTML mail to list.
Plain text only.

Thanks,

-- 
MST



reply via email to

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