[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] vhost-user: fix VHOST_USER_SET_MEM_TABLE
From: |
Damjan Marion (damarion) |
Subject: |
Re: [Qemu-devel] [PATCH] vhost-user: fix VHOST_USER_SET_MEM_TABLE |
Date: |
Thu, 26 Jun 2014 09:10:44 +0000 |
On 26 Jun 2014, at 10:01, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
>> ---
>> 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