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

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

[Damjan Marion]

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 <damarion@cisco.com>
---
 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

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

[Nikolay Nikolaev]

[-- Attachment #1: Type: text/plain, Size: 5964 bytes --]

On Fri, Jun 27, 2014 at 12:01 AM, Damjan Marion <damarion@cisco.com> 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 <damarion@cisco.com>
> ---
>  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 <n.nikolaev@virtualopensystems.com>

[-- Attachment #2: Type: text/html, Size: 7542 bytes --]

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

[Michael S. Tsirkin]

On Fri, Jun 27, 2014 at 08:02:48AM +0300, Nikolay Nikolaev wrote:
> 
> 
> 
> On Fri, Jun 27, 2014 at 12:01 AM, Damjan Marion <damarion@cisco.com> 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 <damarion@cisco.com>
>     ---
>      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 <n.nikolaev@virtualopensystems.com>
> 


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

Thanks,

-- 
MST

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 <damarion@cisco.com>
> ---
>  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

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

[Michael S. Tsirkin]

On Fri, Jun 27, 2014 at 08:02:48AM +0300, Nikolay Nikolaev wrote:
> 
> 
> 
> On Fri, Jun 27, 2014 at 12:01 AM, Damjan Marion <damarion@cisco.com> 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 <damarion@cisco.com>
>     ---
>      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 <n.nikolaev@virtualopensystems.com>

Can you please stop sending HTML formatted mail to list?
It adds nothing at all to the conversation but, makes me
work extra to record your acks, piping the whole mail does not work.
Some hints on configuring various clients:
https://www.kernel.org/doc/Documentation/email-clients.txt

-- 
MST

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

[Michael S. Tsirkin]

Subject has a typo.
Fixed it up.


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

s/alredy/already/

>  - 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 <damarion@cisco.com>

Applied with these fixes.

> ---
>  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

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 <damarion@cisco.com>

Please add a unit test for this as well.


> ---
>  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