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

[Michael S. Tsirkin]

On Thu, Jun 26, 2014 at 11:51:51AM +0000, Damjan Marion (damarion) wrote:
> 
> On 26 Jun 2014, at 13:37, Michael S. Tsirkin <address@hidden> wrote:
> 
> > On Thu, Jun 26, 2014 at 11:35:13AM +0000, Damjan Marion (damarion) wrote:
> >> 
> >> On 26 Jun 2014, at 13:30, Michael S. Tsirkin <address@hidden> wrote:
> >> 
> >>> On Thu, Jun 26, 2014 at 12:22:59PM +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>
> >>>> ---
> >>> 
> >>> changelog?
> >> 
> >> Ok
> >> 
> >>> 
> >>> does not look like all comments have been addressed.
> >> 
> >> Like?
> > 
> > I'm sorry you will have to look for them on the list.
> 
> I replied to all comments on the v1 patch thread. 

I'd like to clarify, comments should be addressed
in the patch itself, it is not enough to address
them on hte mailing list thread.
Except when they are completely silly, you should
address them in commit log discussing design
or in code comments.


> - ua is needed, i gave you example
> - version bump, 3 of us (Nikolay) agreed that it is not needed before 2.1 is 
> released
> - ram_addr_t - I changed to uint64_t
> 
> What I missed is Nikolay’s proposal on the yesterday’s thread to rename 
> shm_offset to mmap_offset.
> 
> Can you be so kind and confirm that we are on the same page before i send 
> patch v3.
> 
> 
> > 
> >>> 
> >>>> 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..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..b9d7baa 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,16 +214,19 @@ 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 -
> >>>> +                    (uint64_t) 
> >>>> qemu_get_ram_block_host_ptr(reg->guest_phys_addr);
> >>>> +                fds[fd_num++] = fd;
> >>>>            }
> >>>>        }
> >>>> +        assert(fd_num <= VHOST_MEMORY_MAX_NREGIONS);
> >>>> 
> >>>>        msg.memory.nregions = fd_num;
> >>>> 
> >>>> 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
> >