[Qemu-devel] [PATCH] vhost-user: fix VHOST_USER_SET_MEM_TABLE

[Qemu-devel] [PATCH] vhost-user: fix VHOST_USER_SET_MEM_TABLE

[Damjan Marion]

Old code was affected by memory gaps which
resulted in buffer pointers pointing to
address outside of the mapped regions.

Signed-off-by: Damjan Marion <damarion@cisco.com>
---
 docs/specs/vhost-user.txt |  7 ++++---
 exec.c                    |  7 +++++++
 hw/virtio/vhost-user.c    | 22 +++++++++++++---------
 include/exec/ram_addr.h   |  1 +
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 2641390..c108d07 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 | shared mem offset |
+   -----------------------------------------------------------
 
    Guest address: a 64-bit guest address of the region
    Size: a 64-bit size
    User address: a 64-bit user address
+   Shared mem offset: 64-bit offset where region is located in the shared 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..0cef2d3 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 shm_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,16 @@ 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].shm_offset = reg->userspace_addr -
+                    (ram_addr_t) qemu_get_ram_block_host_ptr(reg->guest_phys_addr);
+                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] vhost-user: fix VHOST_USER_SET_MEM_TABLE

[Michael S. Tsirkin]

On Thu, Jun 26, 2014 at 09:55:03AM +0200, Damjan Marion wrote:
> Old code was affected by memory gaps which
> resulted in buffer pointers pointing to
> address outside of the mapped regions.
> 
> Signed-off-by: Damjan Marion <damarion@cisco.com>
> ---
>  docs/specs/vhost-user.txt |  7 ++++---
>  exec.c                    |  7 +++++++
>  hw/virtio/vhost-user.c    | 22 +++++++++++++---------
>  include/exec/ram_addr.h   |  1 +
>  4 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 2641390..c108d07 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 | shared mem offset |
> +   -----------------------------------------------------------
>  
>     Guest address: a 64-bit guest address of the region
>     Size: a 64-bit size
>     User address: a 64-bit user address
> +   Shared mem offset: 64-bit offset where region is located in the shared memory
>  

Why not replace user address with it?
Is user address useful by itself?
Or is it helpful for the old server?

>  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..0cef2d3 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 shm_offset;
>  } VhostUserMemoryRegion;
>  
>  typedef struct VhostUserMemory {

So protocol changes, will it work with the old server?
If not need to increment a version somewhere so it fails cleanly?
If yes how? and then we need to set a capability somewhere so new server can
discover that new field is available?

> @@ -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,16 @@ 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].shm_offset = reg->userspace_addr -
> +                    (ram_addr_t) qemu_get_ram_block_host_ptr(reg->guest_phys_addr);

Why cast to ram_addr_t here? I think you want uintptr_t.

> +                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] vhost-user: fix VHOST_USER_SET_MEM_TABLE

[Damjan Marion (damarion)]

On 26 Jun 2014, at 10:01, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Thu, Jun 26, 2014 at 09:55:03AM +0200, Damjan Marion wrote:
>> Old code was affected by memory gaps which
>> resulted in buffer pointers pointing to
>> address outside of the mapped regions.
>> 
>> Signed-off-by: Damjan Marion <damarion@cisco.com>
>> ---
>> docs/specs/vhost-user.txt |  7 ++++---
>> exec.c                    |  7 +++++++
>> hw/virtio/vhost-user.c    | 22 +++++++++++++---------
>> include/exec/ram_addr.h   |  1 +
>> 4 files changed, 25 insertions(+), 12 deletions(-)
>> 
>> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>> index 2641390..c108d07 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 | shared mem offset |
>> +   -----------------------------------------------------------
>> 
>>    Guest address: a 64-bit guest address of the region
>>    Size: a 64-bit size
>>    User address: a 64-bit user address
>> +   Shared mem offset: 64-bit offset where region is located in the shared memory
>> 

> Why not replace user address with it?
> Is user address useful by itself?
> Or is it helpful for the old server?

We need user address to convert buffer pointer from descriptor which we read directly from guest memory.
There is sample at 
https://github.com/virtualopensystems/vapp/blob/master/vhost_server.c

functon _map_user_addr().

> 
>> 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..0cef2d3 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 shm_offset;
>> } VhostUserMemoryRegion;
>> 
>> typedef struct VhostUserMemory {
> 
> So protocol changes, will it work with the old server?
> If not need to increment a version somewhere so it fails cleanly?
> If yes how? and then we need to set a capability somewhere so new server can
> discover that new field is available?

Does it really make a sense to keep old broken code, which doesn’t work if VM have more than 3 GB of RAM?
As Nikolay said yesterday: 
> On the other hand there's no wide adoption of
> the protocol so it's still not critical to change it.

I think we should just fix it and keep it as version 1.

> 
>> @@ -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,16 @@ 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].shm_offset = reg->userspace_addr -
>> +                    (ram_addr_t) qemu_get_ram_block_host_ptr(reg->guest_phys_addr);
> 
> Why cast to ram_addr_t here? I think you want uintptr_t.

Actually, it needs to be uint64_t as that is what we have on the left side.


>> +                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] vhost-user: fix VHOST_USER_SET_MEM_TABLE

[Paolo Bonzini]

Il 26/06/2014 11:10, Damjan Marion (damarion) ha scritto:
> Does it really make a sense to keep old broken code, which doesn’t work if VM have more than 3 GB of RAM?
> As Nikolay said yesterday:
>> > On the other hand there's no wide adoption of
>> > the protocol so it's still not critical to change it.
> I think we should just fix it and keep it as version 1.
>

I agree.  Anything that happens before 2.1 is final can stay as version 1.

Paolo