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

[Damjan Marion (damarion)]

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