On 26 Jun 2014, at 10:01, Michael S. Tsirkin <m...@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 <damar...@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