Re: [Qemu-devel] vhost-user: broken mem regions

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jun 25, 2014 at 05:13:21PM +0300, Nikolay Nikolaev wrote:
> 
> 
> 
> On Wed, Jun 25, 2014 at 5:06 PM, Damjan Marion (damarion) <damarion@cisco.com>
> wrote:
> 
> 
>     On 25 Jun 2014, at 15:53, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     > On Wed, Jun 25, 2014 at 01:45:09PM +0000, Damjan Marion (damarion) wrote:
>     >>
>     >> Michael,
>     >>
>     >> there is another issue with commited vhost-user code.
>     >
>     > I'm answering just this once, but I have a policy against
>     > answering off-list mail.
>     > Pls send follow-ups to the list.
> 
>     ok, sorry, adding list...
> 
>     >
>     >> If there is bigger mem allocation (i.e. 4G or 6G of RAM) then
>     >> we have memory gap and then it happens that buffer pointer points to
>     >> memory which is not mmaped. I already filled bug report:
>     >>
>     >> https://bugs.launchpad.net/qemu/+bug/1333688
>     >
>     > FYI I mostly ignore launchpad.
>     > Because of the unfortunate association with Ubuntu, most bugs
>     > there are distro-specific.
>     >
>     >> Bellow is my proposed change to the code. Two things:
>     >> - it will require changes on the user side also
>     >
>     > why would it?
>     > format seems unchanged, right?
> 
>     yes, but it will happen that multiple regions have same FD so call to mmap
>     ()
>     should look different, I’m still playing with this on user side...
> 
> but then you shoudl somehow accumulate the sizes and send just a single fd,
> something along these lines.


Not going to work, these regions might not be contigious
(e.g. if a higher priority region hides RAM).
Just deal with the fact that same fd can appear
multiple times. I don't see why it's a big deal anyway.


> 
>     >
>     >> - not sure if qemu_get_ram_fd() is the best way to get FD
>     >
>     > Paolo, what do you think?
>     >
>     >> Any comments or better idea how to fix this?
>     >>
>     >> Thanks,
>     >>
>     >> Damjan
>     >>
>     >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>     >> index 0df6a93..89fe5c7 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>
>     >> @@ -183,10 +184,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 +213,14 @@ 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->
>     memory_size;
>     >> +                fds[fd_num++] = fd;
>     >>             }
>     >>         }
>     >>
> 
> 
>