[v7,12/24] vhost: use a function for each call

On Thu, Oct 1, 2015 at 7:23 PM,  <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Replace the generic vhost_call() by specific functions for each
> function call to help with type safety and changing arguments.
>
> While doing this, I found that "unsigned long long" and "uint64_t" were
> used interchangeably and causing compilation warnings, using uint64_t
> instead, as the vhost & protocol specifies.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/net/vhost_net.c                |  16 +-
>  hw/scsi/vhost-scsi.c              |   7 +-
>  hw/virtio/vhost-backend.c         | 124 ++++++++-
>  hw/virtio/vhost-user.c            | 518 ++++++++++++++++++++++----------------
>  hw/virtio/vhost.c                 |  36 +--
>  include/hw/virtio/vhost-backend.h |  63 ++++-
>  include/hw/virtio/vhost.h         |  12 +-
>  7 files changed, 501 insertions(+), 275 deletions(-)
>
[snip]
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index f1edd04..cd84f0c 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
[snip]
> @@ -190,231 +182,317 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
>              0 : -1;
>  }
>
> -static bool vhost_user_one_time_request(VhostUserRequest request)
> +static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
> +                                   struct vhost_log *log)
>  {
> -    switch (request) {
> -    case VHOST_USER_SET_OWNER:
> -    case VHOST_USER_RESET_DEVICE:
> -    case VHOST_USER_SET_MEM_TABLE:
> -    case VHOST_USER_GET_QUEUE_NUM:
> -        return true;
> -    default:
> -        return false;
> +    int fds[VHOST_MEMORY_MAX_NREGIONS];
> +    size_t fd_num = 0;
> +    bool shmfd = virtio_has_feature(dev->protocol_features,
> +                                    VHOST_USER_PROTOCOL_F_LOG_SHMFD);
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_SET_LOG_BASE,
> +        .flags = VHOST_USER_VERSION,
> +        .u64 = base,
> +        .size = sizeof(m.u64),
> +    };
> +
> +    if (shmfd && log->fd != -1) {
> +        fds[fd_num++] = log->fd;
>      }
> +
> +    vhost_user_write(dev, &msg, fds, fd_num);
> +
> +    if (shmfd) {
> +        msg.size = 0;
> +        if (vhost_user_read(dev, &msg) < 0) {
> +            return 0;
> +        }
> +
> +        if (msg.request != VHOST_USER_SET_LOG_BASE) {
> +            error_report("Received unexpected msg type. "
> +                         "Expected %d received %d",
> +                         VHOST_USER_SET_LOG_BASE, msg.request);
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
>  }
>
> -static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> -        void *arg)
> +static int vhost_user_set_mem_table(struct vhost_dev *dev,
> +                                    struct vhost_memory *mem)
>  {
> -    VhostUserMsg msg;
> -    VhostUserRequest msg_request;
> -    struct vhost_vring_file *file = 0;
> -    int need_reply = 0;
>      int fds[VHOST_MEMORY_MAX_NREGIONS];
>      int i, fd;
>      size_t fd_num = 0;
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_SET_MEM_TABLE,
> +        .flags = VHOST_USER_VERSION,
> +    };
>
> -    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> -
> -    /* only translate vhost ioctl requests */
> -    if (request > VHOST_USER_MAX) {
> -        msg_request = vhost_user_request_translate(request);
> -    } else {
> -        msg_request = request;
> +    for (i = 0; i < dev->mem->nregions; ++i) {
> +        struct vhost_memory_region *reg = dev->mem->regions + i;
> +        ram_addr_t ram_addr;
> +
> +        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> +        qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
> +                                &ram_addr);
> +        fd = qemu_get_ram_fd(ram_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].mmap_offset = reg->userspace_addr -
> +                (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
> +            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> +            fds[fd_num++] = fd;
> +        }
>      }
>
> -    /*
> -     * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE,
> -     * we just need send it once in the first time. For later such
> -     * request, we just ignore it.
> -     */
> -    if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0) {
> -        return 0;
> +    msg.memory.nregions = fd_num;
> +
> +    if (!fd_num) {
> +        error_report("Failed initializing vhost-user memory map, "
> +                     "consider using -object memory-backend-file share=on");
> +        return -1;
>      }
>
> -    msg.request = msg_request;
> -    msg.flags = VHOST_USER_VERSION;
> -    msg.size = 0;
> +    msg.size = sizeof(m.memory.nregions);
> +    msg.size += sizeof(m.memory.padding);
> +    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
>
> -    switch (msg_request) {
> -    case VHOST_USER_GET_FEATURES:
> -    case VHOST_USER_GET_PROTOCOL_FEATURES:
> -    case VHOST_USER_GET_QUEUE_NUM:
> -        need_reply = 1;
> -        break;
> +    vhost_user_write(dev, &msg, fds, fd_num);
>
> -    case VHOST_USER_SET_FEATURES:
> -    case VHOST_USER_SET_PROTOCOL_FEATURES:
> -        msg.u64 = *((__u64 *) arg);
> -        msg.size = sizeof(m.u64);
> -        break;
> +    return 0;
> +}
>
> -    case VHOST_USER_SET_OWNER:
> -    case VHOST_USER_RESET_DEVICE:
> -        break;
> +static int vhost_user_set_vring_addr(struct vhost_dev *dev,
> +                                     struct vhost_vring_addr *addr)
> +{
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_SET_VRING_ADDR,
> +        .flags = VHOST_USER_VERSION,
> +        .addr = *addr,
> +        .size = sizeof(*addr),
> +    };
>
> -    case VHOST_USER_SET_MEM_TABLE:
> -        for (i = 0; i < dev->mem->nregions; ++i) {
> -            struct vhost_memory_region *reg = dev->mem->regions + i;
> -            ram_addr_t ram_addr;
> -
> -            assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> -            qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr, &ram_addr);
> -            fd = qemu_get_ram_fd(ram_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].mmap_offset = reg->userspace_addr -
> -                    (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
> -                assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> -                fds[fd_num++] = fd;
> -            }
> -        }
> +    vhost_user_write(dev, &msg, NULL, 0);
>
> -        msg.memory.nregions = fd_num;
> +    return 0;
> +}
>
> -        if (!fd_num) {
> -            error_report("Failed initializing vhost-user memory map, "
> -                    "consider using -object memory-backend-file share=on");
> -            return -1;
> -        }
> +static int vhost_user_set_vring_endian(struct vhost_dev *dev,
> +                                       struct vhost_vring_state *ring)
> +{
> +    error_report("vhost-user trying to send unhandled ioctl");
> +    return -1;
> +}
>
> -        msg.size = sizeof(m.memory.nregions);
> -        msg.size += sizeof(m.memory.padding);
> -        msg.size += fd_num * sizeof(VhostUserMemoryRegion);
> -
> -        break;
> -
> -    case VHOST_USER_SET_LOG_FD:
> -        fds[fd_num++] = *((int *) arg);
> -        break;
> -
> -    case VHOST_USER_SET_VRING_NUM:
> -    case VHOST_USER_SET_VRING_BASE:
> -    case VHOST_USER_SET_VRING_ENABLE:
> -        memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> -        msg.size = sizeof(m.state);
> -        break;
> -
> -    case VHOST_USER_GET_VRING_BASE:
> -        memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> -        msg.size = sizeof(m.state);
> -        need_reply = 1;
> -        break;
> -
> -    case VHOST_USER_SET_VRING_ADDR:
> -        memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
> -        msg.size = sizeof(m.addr);
> -        break;
> -
> -    case VHOST_USER_SET_VRING_KICK:
> -    case VHOST_USER_SET_VRING_CALL:
> -    case VHOST_USER_SET_VRING_ERR:
> -        file = arg;
> -        msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
> -        msg.size = sizeof(m.u64);
> -        if (ioeventfd_enabled() && file->fd > 0) {
> -            fds[fd_num++] = file->fd;
> -        } else {
> -            msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
> -        }
> -        break;
> -    default:
> -        error_report("vhost-user trying to send unhandled ioctl");
> +static int vhost_set_vring(struct vhost_dev *dev,
> +                           unsigned long int request,
> +                           struct vhost_vring_state *ring)
> +{
> +    VhostUserMsg msg = {
> +        .request = request,
> +        .flags = VHOST_USER_VERSION,
> +        .state = *ring,
> +        .size = sizeof(*ring),
> +    };
> +
> +    vhost_user_write(dev, &msg, NULL, 0);
> +
> +    return 0;
> +}
> +
> +static int vhost_user_set_vring_num(struct vhost_dev *dev,
> +                                    struct vhost_vring_state *ring)
> +{
> +    return vhost_set_vring(dev, VHOST_SET_VRING_NUM, ring);

It is not the correct vhost user message request.
VHOST_SET_VRING_NUM is the IO for the kernel.
The correct modification is
+    return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);

> +}
> +
> +static int vhost_user_set_vring_base(struct vhost_dev *dev,
> +                                     struct vhost_vring_state *ring)
> +{
> +    return vhost_set_vring(dev, VHOST_SET_VRING_BASE, ring);

It is not the correct vhost user message request.
VHOST_SET_VRING_BASE is the IO for the kernel.
The correct modification is
+    return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);

> +}
> +
> +static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
> +{
> +    struct vhost_vring_state state = {
> +        .index = dev->vq_index,
> +        .num   = enable,
> +    };
> +
> +    if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ))) {
>          return -1;
> -        break;
>      }
>
> -    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
> +    return vhost_set_vring(dev, VHOST_USER_SET_VRING_ENABLE, &state);
> +}
> +
> +
> +static int vhost_user_get_vring_base(struct vhost_dev *dev,
> +                                     struct vhost_vring_state *ring)
> +{
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_GET_VRING_BASE,
> +        .flags = VHOST_USER_VERSION,
> +        .state = *ring,
> +        .size = sizeof(*ring),
> +    };
> +
> +    vhost_user_write(dev, &msg, NULL, 0);
> +
> +    if (vhost_user_read(dev, &msg) < 0) {
>          return 0;
>      }
>
> -    if (need_reply) {
> -        if (vhost_user_read(dev, &msg) < 0) {
> -            return 0;
> -        }
> -
> -        if (msg_request != msg.request) {
> -            error_report("Received unexpected msg type."
> -                    " Expected %d received %d", msg_request, msg.request);
> -            return -1;
> -        }
> +    if (msg.request != VHOST_USER_GET_VRING_BASE) {
> +        error_report("Received unexpected msg type. Expected %d received %d",
> +                     VHOST_USER_GET_VRING_BASE, msg.request);
> +        return -1;
> +    }
>
> -        switch (msg_request) {
> -        case VHOST_USER_GET_FEATURES:
> -        case VHOST_USER_GET_PROTOCOL_FEATURES:
> -        case VHOST_USER_GET_QUEUE_NUM:
> -            if (msg.size != sizeof(m.u64)) {
> -                error_report("Received bad msg size.");
> -                return -1;
> -            }
> -            *((__u64 *) arg) = msg.u64;
> -            break;
> -        case VHOST_USER_GET_VRING_BASE:
> -            if (msg.size != sizeof(m.state)) {
> -                error_report("Received bad msg size.");
> -                return -1;
> -            }
> -            memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
> -            break;
> -        default:
> -            error_report("Received unexpected msg type.");
> -            return -1;
> -            break;
> -        }
> +    if (msg.size != sizeof(m.state)) {
> +        error_report("Received bad msg size.");
> +        return -1;
>      }
>
> +    *ring = msg.state;
> +
>      return 0;
>  }
>
> -static int vhost_set_log_base(struct vhost_dev *dev, uint64_t base,
> -                              struct vhost_log *log)
> +static int vhost_set_vring_file(struct vhost_dev *dev,
> +                                VhostUserRequest request,
> +                                struct vhost_vring_file *file)
>  {
>      int fds[VHOST_MEMORY_MAX_NREGIONS];
>      size_t fd_num = 0;
> -    bool shmfd = virtio_has_feature(dev->protocol_features,
> -                                    VHOST_USER_PROTOCOL_F_LOG_SHMFD);
>      VhostUserMsg msg = {
> -        .request = VHOST_USER_SET_LOG_BASE,
> +        .request = request,
>          .flags = VHOST_USER_VERSION,
> -        .u64 = base,
> +        .u64 = file->index & VHOST_USER_VRING_IDX_MASK,
>          .size = sizeof(m.u64),
>      };
>
> -    if (shmfd && log->fd != -1) {
> -        fds[fd_num++] = log->fd;
> +    if (ioeventfd_enabled() && file->fd > 0) {
> +        fds[fd_num++] = file->fd;
> +    } else {
> +        msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
>      }
>
>      vhost_user_write(dev, &msg, fds, fd_num);
>
> -    if (shmfd) {
> -        msg.size = 0;
> -        if (vhost_user_read(dev, &msg) < 0) {
> -            return 0;
> -        }
> +    return 0;
> +}
>
> -        if (msg.request != VHOST_USER_SET_LOG_BASE) {
> -            error_report("Received unexpected msg type. "
> -                         "Expected %d received %d",
> -                         VHOST_USER_SET_LOG_BASE, msg.request);
> -            return -1;
> -        }
> +static int vhost_user_set_vring_kick(struct vhost_dev *dev,
> +                                     struct vhost_vring_file *file)
> +{
> +    return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_KICK, file);
> +}
> +
> +static int vhost_user_set_vring_call(struct vhost_dev *dev,
> +                                     struct vhost_vring_file *file)
> +{
> +    return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
> +}
> +
> +static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
> +{
> +    VhostUserMsg msg = {
> +        .request = request,
> +        .flags = VHOST_USER_VERSION,
> +        .u64 = u64,
> +        .size = sizeof(m.u64),
> +    };
> +
> +    vhost_user_write(dev, &msg, NULL, 0);
> +
> +    return 0;
> +}
> +
> +static int vhost_user_set_features(struct vhost_dev *dev,
> +                                   uint64_t features)
> +{
> +    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
> +}
> +
> +static int vhost_user_set_protocol_features(struct vhost_dev *dev,
> +                                            uint64_t features)
> +{
> +    return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES, features);
> +}
> +
> +static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
> +{
> +    VhostUserMsg msg = {
> +        .request = request,
> +        .flags = VHOST_USER_VERSION,
> +    };
> +

With multiqueue there are an issue with this implementation. The
VHOST_USER_GET_QUEUE_NUM message is sent through this function and it
is a one time message.
For the queue index different from 0 the vhost_user_write returns
immediately without sending the request to the backend.
Then the vhost_user_read never returns and QEMU is blocked.
I suggest to add the following test before calling the
vhost_user_write function to avoid this issue:

+    if (vhost_user_one_time_request(request) && dev->vq_index != 0) {
+        return 0;
+    }
+

> +    vhost_user_write(dev, &msg, NULL, 0);
> +
> +    if (vhost_user_read(dev, &msg) < 0) {
> +        return 0;
> +    }
> +
> +    if (msg.request != request) {
> +        error_report("Received unexpected msg type. Expected %d received %d",
> +                     request, msg.request);
> +        return -1;
> +    }
> +
> +    if (msg.size != sizeof(m.u64)) {
> +        error_report("Received bad msg size.");
> +        return -1;
>      }
>
[snip]

The attached file is the fixup to apply to this patch.

Regards.

Thibaut.

Hi Thibaut

----- Original Message -----
> On Thu, Oct 1, 2015 at 7:23 PM,  <marcandre.lureau@redhat.com> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Replace the generic vhost_call() by specific functions for each
> > function call to help with type safety and changing arguments.
> >
> > While doing this, I found that "unsigned long long" and "uint64_t" were
> > used interchangeably and causing compilation warnings, using uint64_t
> > instead, as the vhost & protocol specifies.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/net/vhost_net.c                |  16 +-
> >  hw/scsi/vhost-scsi.c              |   7 +-
> >  hw/virtio/vhost-backend.c         | 124 ++++++++-
> >  hw/virtio/vhost-user.c            | 518
> >  ++++++++++++++++++++++----------------
> >  hw/virtio/vhost.c                 |  36 +--
> >  include/hw/virtio/vhost-backend.h |  63 ++++-
> >  include/hw/virtio/vhost.h         |  12 +-
> >  7 files changed, 501 insertions(+), 275 deletions(-)
> >
> [snip]
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index f1edd04..cd84f0c 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> [snip]
> > @@ -190,231 +182,317 @@ static int vhost_user_write(struct vhost_dev *dev,
> > VhostUserMsg *msg,
> >              0 : -1;
> >  }
> >
> > -static bool vhost_user_one_time_request(VhostUserRequest request)
> > +static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
> > +                                   struct vhost_log *log)
> >  {
> > -    switch (request) {
> > -    case VHOST_USER_SET_OWNER:
> > -    case VHOST_USER_RESET_DEVICE:
> > -    case VHOST_USER_SET_MEM_TABLE:
> > -    case VHOST_USER_GET_QUEUE_NUM:
> > -        return true;
> > -    default:
> > -        return false;
> > +    int fds[VHOST_MEMORY_MAX_NREGIONS];
> > +    size_t fd_num = 0;
> > +    bool shmfd = virtio_has_feature(dev->protocol_features,
> > +                                    VHOST_USER_PROTOCOL_F_LOG_SHMFD);
> > +    VhostUserMsg msg = {
> > +        .request = VHOST_USER_SET_LOG_BASE,
> > +        .flags = VHOST_USER_VERSION,
> > +        .u64 = base,
> > +        .size = sizeof(m.u64),
> > +    };
> > +
> > +    if (shmfd && log->fd != -1) {
> > +        fds[fd_num++] = log->fd;
> >      }
> > +
> > +    vhost_user_write(dev, &msg, fds, fd_num);
> > +
> > +    if (shmfd) {
> > +        msg.size = 0;
> > +        if (vhost_user_read(dev, &msg) < 0) {
> > +            return 0;
> > +        }
> > +
> > +        if (msg.request != VHOST_USER_SET_LOG_BASE) {
> > +            error_report("Received unexpected msg type. "
> > +                         "Expected %d received %d",
> > +                         VHOST_USER_SET_LOG_BASE, msg.request);
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    return 0;
> >  }
> >
> > -static int vhost_user_call(struct vhost_dev *dev, unsigned long int
> > request,
> > -        void *arg)
> > +static int vhost_user_set_mem_table(struct vhost_dev *dev,
> > +                                    struct vhost_memory *mem)
> >  {
> > -    VhostUserMsg msg;
> > -    VhostUserRequest msg_request;
> > -    struct vhost_vring_file *file = 0;
> > -    int need_reply = 0;
> >      int fds[VHOST_MEMORY_MAX_NREGIONS];
> >      int i, fd;
> >      size_t fd_num = 0;
> > +    VhostUserMsg msg = {
> > +        .request = VHOST_USER_SET_MEM_TABLE,
> > +        .flags = VHOST_USER_VERSION,
> > +    };
> >
> > -    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> > -
> > -    /* only translate vhost ioctl requests */
> > -    if (request > VHOST_USER_MAX) {
> > -        msg_request = vhost_user_request_translate(request);
> > -    } else {
> > -        msg_request = request;
> > +    for (i = 0; i < dev->mem->nregions; ++i) {
> > +        struct vhost_memory_region *reg = dev->mem->regions + i;
> > +        ram_addr_t ram_addr;
> > +
> > +        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> > +        qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
> > +                                &ram_addr);
> > +        fd = qemu_get_ram_fd(ram_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].mmap_offset = reg->userspace_addr -
> > +                (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
> > +            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> > +            fds[fd_num++] = fd;
> > +        }
> >      }
> >
> > -    /*
> > -     * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE,
> > -     * we just need send it once in the first time. For later such
> > -     * request, we just ignore it.
> > -     */
> > -    if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0) {
> > -        return 0;
> > +    msg.memory.nregions = fd_num;
> > +
> > +    if (!fd_num) {
> > +        error_report("Failed initializing vhost-user memory map, "
> > +                     "consider using -object memory-backend-file
> > share=on");
> > +        return -1;
> >      }
> >
> > -    msg.request = msg_request;
> > -    msg.flags = VHOST_USER_VERSION;
> > -    msg.size = 0;
> > +    msg.size = sizeof(m.memory.nregions);
> > +    msg.size += sizeof(m.memory.padding);
> > +    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
> >
> > -    switch (msg_request) {
> > -    case VHOST_USER_GET_FEATURES:
> > -    case VHOST_USER_GET_PROTOCOL_FEATURES:
> > -    case VHOST_USER_GET_QUEUE_NUM:
> > -        need_reply = 1;
> > -        break;
> > +    vhost_user_write(dev, &msg, fds, fd_num);
> >
> > -    case VHOST_USER_SET_FEATURES:
> > -    case VHOST_USER_SET_PROTOCOL_FEATURES:
> > -        msg.u64 = *((__u64 *) arg);
> > -        msg.size = sizeof(m.u64);
> > -        break;
> > +    return 0;
> > +}
> >
> > -    case VHOST_USER_SET_OWNER:
> > -    case VHOST_USER_RESET_DEVICE:
> > -        break;
> > +static int vhost_user_set_vring_addr(struct vhost_dev *dev,
> > +                                     struct vhost_vring_addr *addr)
> > +{
> > +    VhostUserMsg msg = {
> > +        .request = VHOST_USER_SET_VRING_ADDR,
> > +        .flags = VHOST_USER_VERSION,
> > +        .addr = *addr,
> > +        .size = sizeof(*addr),
> > +    };
> >
> > -    case VHOST_USER_SET_MEM_TABLE:
> > -        for (i = 0; i < dev->mem->nregions; ++i) {
> > -            struct vhost_memory_region *reg = dev->mem->regions + i;
> > -            ram_addr_t ram_addr;
> > -
> > -            assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> > -            qemu_ram_addr_from_host((void
> > *)(uintptr_t)reg->userspace_addr, &ram_addr);
> > -            fd = qemu_get_ram_fd(ram_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].mmap_offset =
> > reg->userspace_addr -
> > -                    (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
> > -                assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> > -                fds[fd_num++] = fd;
> > -            }
> > -        }
> > +    vhost_user_write(dev, &msg, NULL, 0);
> >
> > -        msg.memory.nregions = fd_num;
> > +    return 0;
> > +}
> >
> > -        if (!fd_num) {
> > -            error_report("Failed initializing vhost-user memory map, "
> > -                    "consider using -object memory-backend-file
> > share=on");
> > -            return -1;
> > -        }
> > +static int vhost_user_set_vring_endian(struct vhost_dev *dev,
> > +                                       struct vhost_vring_state *ring)
> > +{
> > +    error_report("vhost-user trying to send unhandled ioctl");
> > +    return -1;
> > +}
> >
> > -        msg.size = sizeof(m.memory.nregions);
> > -        msg.size += sizeof(m.memory.padding);
> > -        msg.size += fd_num * sizeof(VhostUserMemoryRegion);
> > -
> > -        break;
> > -
> > -    case VHOST_USER_SET_LOG_FD:
> > -        fds[fd_num++] = *((int *) arg);
> > -        break;
> > -
> > -    case VHOST_USER_SET_VRING_NUM:
> > -    case VHOST_USER_SET_VRING_BASE:
> > -    case VHOST_USER_SET_VRING_ENABLE:
> > -        memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> > -        msg.size = sizeof(m.state);
> > -        break;
> > -
> > -    case VHOST_USER_GET_VRING_BASE:
> > -        memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> > -        msg.size = sizeof(m.state);
> > -        need_reply = 1;
> > -        break;
> > -
> > -    case VHOST_USER_SET_VRING_ADDR:
> > -        memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
> > -        msg.size = sizeof(m.addr);
> > -        break;
> > -
> > -    case VHOST_USER_SET_VRING_KICK:
> > -    case VHOST_USER_SET_VRING_CALL:
> > -    case VHOST_USER_SET_VRING_ERR:
> > -        file = arg;
> > -        msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
> > -        msg.size = sizeof(m.u64);
> > -        if (ioeventfd_enabled() && file->fd > 0) {
> > -            fds[fd_num++] = file->fd;
> > -        } else {
> > -            msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
> > -        }
> > -        break;
> > -    default:
> > -        error_report("vhost-user trying to send unhandled ioctl");
> > +static int vhost_set_vring(struct vhost_dev *dev,
> > +                           unsigned long int request,
> > +                           struct vhost_vring_state *ring)
> > +{
> > +    VhostUserMsg msg = {
> > +        .request = request,
> > +        .flags = VHOST_USER_VERSION,
> > +        .state = *ring,
> > +        .size = sizeof(*ring),
> > +    };
> > +
> > +    vhost_user_write(dev, &msg, NULL, 0);
> > +
> > +    return 0;
> > +}
> > +
> > +static int vhost_user_set_vring_num(struct vhost_dev *dev,
> > +                                    struct vhost_vring_state *ring)
> > +{
> > +    return vhost_set_vring(dev, VHOST_SET_VRING_NUM, ring);
> 
> It is not the correct vhost user message request.
> VHOST_SET_VRING_NUM is the IO for the kernel.
> The correct modification is
> +    return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> 

Something I missed during rebase, thanks

> > +}
> > +
> > +static int vhost_user_set_vring_base(struct vhost_dev *dev,
> > +                                     struct vhost_vring_state *ring)
> > +{
> > +    return vhost_set_vring(dev, VHOST_SET_VRING_BASE, ring);
> 
> It is not the correct vhost user message request.
> VHOST_SET_VRING_BASE is the IO for the kernel.
> The correct modification is
> +    return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);

hic

> 
> > +}
> > +
> > +static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
> > +{
> > +    struct vhost_vring_state state = {
> > +        .index = dev->vq_index,
> > +        .num   = enable,
> > +    };
> > +
> > +    if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ))) {
> >          return -1;
> > -        break;
> >      }
> >
> > -    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
> > +    return vhost_set_vring(dev, VHOST_USER_SET_VRING_ENABLE, &state);
> > +}
> > +
> > +
> > +static int vhost_user_get_vring_base(struct vhost_dev *dev,
> > +                                     struct vhost_vring_state *ring)
> > +{
> > +    VhostUserMsg msg = {
> > +        .request = VHOST_USER_GET_VRING_BASE,
> > +        .flags = VHOST_USER_VERSION,
> > +        .state = *ring,
> > +        .size = sizeof(*ring),
> > +    };
> > +
> > +    vhost_user_write(dev, &msg, NULL, 0);
> > +
> > +    if (vhost_user_read(dev, &msg) < 0) {
> >          return 0;
> >      }
> >
> > -    if (need_reply) {
> > -        if (vhost_user_read(dev, &msg) < 0) {
> > -            return 0;
> > -        }
> > -
> > -        if (msg_request != msg.request) {
> > -            error_report("Received unexpected msg type."
> > -                    " Expected %d received %d", msg_request, msg.request);
> > -            return -1;
> > -        }
> > +    if (msg.request != VHOST_USER_GET_VRING_BASE) {
> > +        error_report("Received unexpected msg type. Expected %d received
> > %d",
> > +                     VHOST_USER_GET_VRING_BASE, msg.request);
> > +        return -1;
> > +    }
> >
> > -        switch (msg_request) {
> > -        case VHOST_USER_GET_FEATURES:
> > -        case VHOST_USER_GET_PROTOCOL_FEATURES:
> > -        case VHOST_USER_GET_QUEUE_NUM:
> > -            if (msg.size != sizeof(m.u64)) {
> > -                error_report("Received bad msg size.");
> > -                return -1;
> > -            }
> > -            *((__u64 *) arg) = msg.u64;
> > -            break;
> > -        case VHOST_USER_GET_VRING_BASE:
> > -            if (msg.size != sizeof(m.state)) {
> > -                error_report("Received bad msg size.");
> > -                return -1;
> > -            }
> > -            memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
> > -            break;
> > -        default:
> > -            error_report("Received unexpected msg type.");
> > -            return -1;
> > -            break;
> > -        }
> > +    if (msg.size != sizeof(m.state)) {
> > +        error_report("Received bad msg size.");
> > +        return -1;
> >      }
> >
> > +    *ring = msg.state;
> > +
> >      return 0;
> >  }
> >
> > -static int vhost_set_log_base(struct vhost_dev *dev, uint64_t base,
> > -                              struct vhost_log *log)
> > +static int vhost_set_vring_file(struct vhost_dev *dev,
> > +                                VhostUserRequest request,
> > +                                struct vhost_vring_file *file)
> >  {
> >      int fds[VHOST_MEMORY_MAX_NREGIONS];
> >      size_t fd_num = 0;
> > -    bool shmfd = virtio_has_feature(dev->protocol_features,
> > -                                    VHOST_USER_PROTOCOL_F_LOG_SHMFD);
> >      VhostUserMsg msg = {
> > -        .request = VHOST_USER_SET_LOG_BASE,
> > +        .request = request,
> >          .flags = VHOST_USER_VERSION,
> > -        .u64 = base,
> > +        .u64 = file->index & VHOST_USER_VRING_IDX_MASK,
> >          .size = sizeof(m.u64),
> >      };
> >
> > -    if (shmfd && log->fd != -1) {
> > -        fds[fd_num++] = log->fd;
> > +    if (ioeventfd_enabled() && file->fd > 0) {
> > +        fds[fd_num++] = file->fd;
> > +    } else {
> > +        msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
> >      }
> >
> >      vhost_user_write(dev, &msg, fds, fd_num);
> >
> > -    if (shmfd) {
> > -        msg.size = 0;
> > -        if (vhost_user_read(dev, &msg) < 0) {
> > -            return 0;
> > -        }
> > +    return 0;
> > +}
> >
> > -        if (msg.request != VHOST_USER_SET_LOG_BASE) {
> > -            error_report("Received unexpected msg type. "
> > -                         "Expected %d received %d",
> > -                         VHOST_USER_SET_LOG_BASE, msg.request);
> > -            return -1;
> > -        }
> > +static int vhost_user_set_vring_kick(struct vhost_dev *dev,
> > +                                     struct vhost_vring_file *file)
> > +{
> > +    return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_KICK, file);
> > +}
> > +
> > +static int vhost_user_set_vring_call(struct vhost_dev *dev,
> > +                                     struct vhost_vring_file *file)
> > +{
> > +    return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
> > +}
> > +
> > +static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t
> > u64)
> > +{
> > +    VhostUserMsg msg = {
> > +        .request = request,
> > +        .flags = VHOST_USER_VERSION,
> > +        .u64 = u64,
> > +        .size = sizeof(m.u64),
> > +    };
> > +
> > +    vhost_user_write(dev, &msg, NULL, 0);
> > +
> > +    return 0;
> > +}
> > +
> > +static int vhost_user_set_features(struct vhost_dev *dev,
> > +                                   uint64_t features)
> > +{
> > +    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
> > +}
> > +
> > +static int vhost_user_set_protocol_features(struct vhost_dev *dev,
> > +                                            uint64_t features)
> > +{
> > +    return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES,
> > features);
> > +}
> > +
> > +static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t
> > *u64)
> > +{
> > +    VhostUserMsg msg = {
> > +        .request = request,
> > +        .flags = VHOST_USER_VERSION,
> > +    };
> > +
> 
> With multiqueue there are an issue with this implementation. The
> VHOST_USER_GET_QUEUE_NUM message is sent through this function and it
> is a one time message.
> For the queue index different from 0 the vhost_user_write returns
> immediately without sending the request to the backend.
> Then the vhost_user_read never returns and QEMU is blocked.
> I suggest to add the following test before calling the
> vhost_user_write function to avoid this issue:
> 
> +    if (vhost_user_one_time_request(request) && dev->vq_index != 0) {
> +        return 0;
> +    }
> +

Hmm, there are no tests for multi-queue? I thought we had some, how did you test it then?

thanks a lot for finding the issue!

> 
> > +    vhost_user_write(dev, &msg, NULL, 0);
> > +
> > +    if (vhost_user_read(dev, &msg) < 0) {
> > +        return 0;
> > +    }
> > +
> > +    if (msg.request != request) {
> > +        error_report("Received unexpected msg type. Expected %d received
> > %d",
> > +                     request, msg.request);
> > +        return -1;
> > +    }
> > +
> > +    if (msg.size != sizeof(m.u64)) {
> > +        error_report("Received bad msg size.");
> > +        return -1;
> >      }
> >
> [snip]
> 
> The attached file is the fixup to apply to this patch.
> 
> Regards.
> 
> Thibaut.
>

On Wed, Oct 7, 2015 at 5:58 PM, Marc-André Lureau <mlureau@redhat.com> wrote:
> Hi Thibaut
>
> ----- Original Message -----
>> On Thu, Oct 1, 2015 at 7:23 PM,  <marcandre.lureau@redhat.com> wrote:
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Replace the generic vhost_call() by specific functions for each
>> > function call to help with type safety and changing arguments.
>> >
>> > While doing this, I found that "unsigned long long" and "uint64_t" were
>> > used interchangeably and causing compilation warnings, using uint64_t
>> > instead, as the vhost & protocol specifies.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  hw/net/vhost_net.c                |  16 +-
>> >  hw/scsi/vhost-scsi.c              |   7 +-
>> >  hw/virtio/vhost-backend.c         | 124 ++++++++-
>> >  hw/virtio/vhost-user.c            | 518
>> >  ++++++++++++++++++++++----------------
>> >  hw/virtio/vhost.c                 |  36 +--
>> >  include/hw/virtio/vhost-backend.h |  63 ++++-
>> >  include/hw/virtio/vhost.h         |  12 +-
>> >  7 files changed, 501 insertions(+), 275 deletions(-)
>> >
>> [snip]
>> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> > index f1edd04..cd84f0c 100644
>> > --- a/hw/virtio/vhost-user.c
>> > +++ b/hw/virtio/vhost-user.c
>> [snip]
>> > @@ -190,231 +182,317 @@ static int vhost_user_write(struct vhost_dev *dev,
>> > VhostUserMsg *msg,
>> >              0 : -1;
>> >  }
>> >
>> > -static bool vhost_user_one_time_request(VhostUserRequest request)
>> > +static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
>> > +                                   struct vhost_log *log)
>> >  {
>> > -    switch (request) {
>> > -    case VHOST_USER_SET_OWNER:
>> > -    case VHOST_USER_RESET_DEVICE:
>> > -    case VHOST_USER_SET_MEM_TABLE:
>> > -    case VHOST_USER_GET_QUEUE_NUM:
>> > -        return true;
>> > -    default:
>> > -        return false;
>> > +    int fds[VHOST_MEMORY_MAX_NREGIONS];
>> > +    size_t fd_num = 0;
>> > +    bool shmfd = virtio_has_feature(dev->protocol_features,
>> > +                                    VHOST_USER_PROTOCOL_F_LOG_SHMFD);
>> > +    VhostUserMsg msg = {
>> > +        .request = VHOST_USER_SET_LOG_BASE,
>> > +        .flags = VHOST_USER_VERSION,
>> > +        .u64 = base,
>> > +        .size = sizeof(m.u64),
>> > +    };
>> > +
>> > +    if (shmfd && log->fd != -1) {
>> > +        fds[fd_num++] = log->fd;
>> >      }
>> > +
>> > +    vhost_user_write(dev, &msg, fds, fd_num);
>> > +
>> > +    if (shmfd) {
>> > +        msg.size = 0;
>> > +        if (vhost_user_read(dev, &msg) < 0) {
>> > +            return 0;
>> > +        }
>> > +
>> > +        if (msg.request != VHOST_USER_SET_LOG_BASE) {
>> > +            error_report("Received unexpected msg type. "
>> > +                         "Expected %d received %d",
>> > +                         VHOST_USER_SET_LOG_BASE, msg.request);
>> > +            return -1;
>> > +        }
>> > +    }
>> > +
>> > +    return 0;
>> >  }
>> >
>> > -static int vhost_user_call(struct vhost_dev *dev, unsigned long int
>> > request,
>> > -        void *arg)
>> > +static int vhost_user_set_mem_table(struct vhost_dev *dev,
>> > +                                    struct vhost_memory *mem)
>> >  {
>> > -    VhostUserMsg msg;
>> > -    VhostUserRequest msg_request;
>> > -    struct vhost_vring_file *file = 0;
>> > -    int need_reply = 0;
>> >      int fds[VHOST_MEMORY_MAX_NREGIONS];
>> >      int i, fd;
>> >      size_t fd_num = 0;
>> > +    VhostUserMsg msg = {
>> > +        .request = VHOST_USER_SET_MEM_TABLE,
>> > +        .flags = VHOST_USER_VERSION,
>> > +    };
>> >
>> > -    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>> > -
>> > -    /* only translate vhost ioctl requests */
>> > -    if (request > VHOST_USER_MAX) {
>> > -        msg_request = vhost_user_request_translate(request);
>> > -    } else {
>> > -        msg_request = request;
>> > +    for (i = 0; i < dev->mem->nregions; ++i) {
>> > +        struct vhost_memory_region *reg = dev->mem->regions + i;
>> > +        ram_addr_t ram_addr;
>> > +
>> > +        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
>> > +        qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
>> > +                                &ram_addr);
>> > +        fd = qemu_get_ram_fd(ram_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].mmap_offset = reg->userspace_addr -
>> > +                (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
>> > +            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
>> > +            fds[fd_num++] = fd;
>> > +        }
>> >      }
>> >
>> > -    /*
>> > -     * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE,
>> > -     * we just need send it once in the first time. For later such
>> > -     * request, we just ignore it.
>> > -     */
>> > -    if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0) {
>> > -        return 0;
>> > +    msg.memory.nregions = fd_num;
>> > +
>> > +    if (!fd_num) {
>> > +        error_report("Failed initializing vhost-user memory map, "
>> > +                     "consider using -object memory-backend-file
>> > share=on");
>> > +        return -1;
>> >      }
>> >
>> > -    msg.request = msg_request;
>> > -    msg.flags = VHOST_USER_VERSION;
>> > -    msg.size = 0;
>> > +    msg.size = sizeof(m.memory.nregions);
>> > +    msg.size += sizeof(m.memory.padding);
>> > +    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
>> >
>> > -    switch (msg_request) {
>> > -    case VHOST_USER_GET_FEATURES:
>> > -    case VHOST_USER_GET_PROTOCOL_FEATURES:
>> > -    case VHOST_USER_GET_QUEUE_NUM:
>> > -        need_reply = 1;
>> > -        break;
>> > +    vhost_user_write(dev, &msg, fds, fd_num);
>> >
>> > -    case VHOST_USER_SET_FEATURES:
>> > -    case VHOST_USER_SET_PROTOCOL_FEATURES:
>> > -        msg.u64 = *((__u64 *) arg);
>> > -        msg.size = sizeof(m.u64);
>> > -        break;
>> > +    return 0;
>> > +}
>> >
>> > -    case VHOST_USER_SET_OWNER:
>> > -    case VHOST_USER_RESET_DEVICE:
>> > -        break;
>> > +static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>> > +                                     struct vhost_vring_addr *addr)
>> > +{
>> > +    VhostUserMsg msg = {
>> > +        .request = VHOST_USER_SET_VRING_ADDR,
>> > +        .flags = VHOST_USER_VERSION,
>> > +        .addr = *addr,
>> > +        .size = sizeof(*addr),
>> > +    };
>> >
>> > -    case VHOST_USER_SET_MEM_TABLE:
>> > -        for (i = 0; i < dev->mem->nregions; ++i) {
>> > -            struct vhost_memory_region *reg = dev->mem->regions + i;
>> > -            ram_addr_t ram_addr;
>> > -
>> > -            assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
>> > -            qemu_ram_addr_from_host((void
>> > *)(uintptr_t)reg->userspace_addr, &ram_addr);
>> > -            fd = qemu_get_ram_fd(ram_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].mmap_offset =
>> > reg->userspace_addr -
>> > -                    (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
>> > -                assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
>> > -                fds[fd_num++] = fd;
>> > -            }
>> > -        }
>> > +    vhost_user_write(dev, &msg, NULL, 0);
>> >
>> > -        msg.memory.nregions = fd_num;
>> > +    return 0;
>> > +}
>> >
>> > -        if (!fd_num) {
>> > -            error_report("Failed initializing vhost-user memory map, "
>> > -                    "consider using -object memory-backend-file
>> > share=on");
>> > -            return -1;
>> > -        }
>> > +static int vhost_user_set_vring_endian(struct vhost_dev *dev,
>> > +                                       struct vhost_vring_state *ring)
>> > +{
>> > +    error_report("vhost-user trying to send unhandled ioctl");
>> > +    return -1;
>> > +}
>> >
>> > -        msg.size = sizeof(m.memory.nregions);
>> > -        msg.size += sizeof(m.memory.padding);
>> > -        msg.size += fd_num * sizeof(VhostUserMemoryRegion);
>> > -
>> > -        break;
>> > -
>> > -    case VHOST_USER_SET_LOG_FD:
>> > -        fds[fd_num++] = *((int *) arg);
>> > -        break;
>> > -
>> > -    case VHOST_USER_SET_VRING_NUM:
>> > -    case VHOST_USER_SET_VRING_BASE:
>> > -    case VHOST_USER_SET_VRING_ENABLE:
>> > -        memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
>> > -        msg.size = sizeof(m.state);
>> > -        break;
>> > -
>> > -    case VHOST_USER_GET_VRING_BASE:
>> > -        memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
>> > -        msg.size = sizeof(m.state);
>> > -        need_reply = 1;
>> > -        break;
>> > -
>> > -    case VHOST_USER_SET_VRING_ADDR:
>> > -        memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
>> > -        msg.size = sizeof(m.addr);
>> > -        break;
>> > -
>> > -    case VHOST_USER_SET_VRING_KICK:
>> > -    case VHOST_USER_SET_VRING_CALL:
>> > -    case VHOST_USER_SET_VRING_ERR:
>> > -        file = arg;
>> > -        msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
>> > -        msg.size = sizeof(m.u64);
>> > -        if (ioeventfd_enabled() && file->fd > 0) {
>> > -            fds[fd_num++] = file->fd;
>> > -        } else {
>> > -            msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
>> > -        }
>> > -        break;
>> > -    default:
>> > -        error_report("vhost-user trying to send unhandled ioctl");
>> > +static int vhost_set_vring(struct vhost_dev *dev,
>> > +                           unsigned long int request,
>> > +                           struct vhost_vring_state *ring)
>> > +{
>> > +    VhostUserMsg msg = {
>> > +        .request = request,
>> > +        .flags = VHOST_USER_VERSION,
>> > +        .state = *ring,
>> > +        .size = sizeof(*ring),
>> > +    };
>> > +
>> > +    vhost_user_write(dev, &msg, NULL, 0);
>> > +
>> > +    return 0;
>> > +}
>> > +
>> > +static int vhost_user_set_vring_num(struct vhost_dev *dev,
>> > +                                    struct vhost_vring_state *ring)
>> > +{
>> > +    return vhost_set_vring(dev, VHOST_SET_VRING_NUM, ring);
>>
>> It is not the correct vhost user message request.
>> VHOST_SET_VRING_NUM is the IO for the kernel.
>> The correct modification is
>> +    return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
>>
>
> Something I missed during rebase, thanks
>
>> > +}
>> > +
>> > +static int vhost_user_set_vring_base(struct vhost_dev *dev,
>> > +                                     struct vhost_vring_state *ring)
>> > +{
>> > +    return vhost_set_vring(dev, VHOST_SET_VRING_BASE, ring);
>>
>> It is not the correct vhost user message request.
>> VHOST_SET_VRING_BASE is the IO for the kernel.
>> The correct modification is
>> +    return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
>
> hic
>
>>
>> > +}
>> > +
>> > +static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
>> > +{
>> > +    struct vhost_vring_state state = {
>> > +        .index = dev->vq_index,
>> > +        .num   = enable,
>> > +    };
>> > +
>> > +    if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ))) {
>> >          return -1;
>> > -        break;
>> >      }
>> >
>> > -    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
>> > +    return vhost_set_vring(dev, VHOST_USER_SET_VRING_ENABLE, &state);
>> > +}
>> > +
>> > +
>> > +static int vhost_user_get_vring_base(struct vhost_dev *dev,
>> > +                                     struct vhost_vring_state *ring)
>> > +{
>> > +    VhostUserMsg msg = {
>> > +        .request = VHOST_USER_GET_VRING_BASE,
>> > +        .flags = VHOST_USER_VERSION,
>> > +        .state = *ring,
>> > +        .size = sizeof(*ring),
>> > +    };
>> > +
>> > +    vhost_user_write(dev, &msg, NULL, 0);
>> > +
>> > +    if (vhost_user_read(dev, &msg) < 0) {
>> >          return 0;
>> >      }
>> >
>> > -    if (need_reply) {
>> > -        if (vhost_user_read(dev, &msg) < 0) {
>> > -            return 0;
>> > -        }
>> > -
>> > -        if (msg_request != msg.request) {
>> > -            error_report("Received unexpected msg type."
>> > -                    " Expected %d received %d", msg_request, msg.request);
>> > -            return -1;
>> > -        }
>> > +    if (msg.request != VHOST_USER_GET_VRING_BASE) {
>> > +        error_report("Received unexpected msg type. Expected %d received
>> > %d",
>> > +                     VHOST_USER_GET_VRING_BASE, msg.request);
>> > +        return -1;
>> > +    }
>> >
>> > -        switch (msg_request) {
>> > -        case VHOST_USER_GET_FEATURES:
>> > -        case VHOST_USER_GET_PROTOCOL_FEATURES:
>> > -        case VHOST_USER_GET_QUEUE_NUM:
>> > -            if (msg.size != sizeof(m.u64)) {
>> > -                error_report("Received bad msg size.");
>> > -                return -1;
>> > -            }
>> > -            *((__u64 *) arg) = msg.u64;
>> > -            break;
>> > -        case VHOST_USER_GET_VRING_BASE:
>> > -            if (msg.size != sizeof(m.state)) {
>> > -                error_report("Received bad msg size.");
>> > -                return -1;
>> > -            }
>> > -            memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
>> > -            break;
>> > -        default:
>> > -            error_report("Received unexpected msg type.");
>> > -            return -1;
>> > -            break;
>> > -        }
>> > +    if (msg.size != sizeof(m.state)) {
>> > +        error_report("Received bad msg size.");
>> > +        return -1;
>> >      }
>> >
>> > +    *ring = msg.state;
>> > +
>> >      return 0;
>> >  }
>> >
>> > -static int vhost_set_log_base(struct vhost_dev *dev, uint64_t base,
>> > -                              struct vhost_log *log)
>> > +static int vhost_set_vring_file(struct vhost_dev *dev,
>> > +                                VhostUserRequest request,
>> > +                                struct vhost_vring_file *file)
>> >  {
>> >      int fds[VHOST_MEMORY_MAX_NREGIONS];
>> >      size_t fd_num = 0;
>> > -    bool shmfd = virtio_has_feature(dev->protocol_features,
>> > -                                    VHOST_USER_PROTOCOL_F_LOG_SHMFD);
>> >      VhostUserMsg msg = {
>> > -        .request = VHOST_USER_SET_LOG_BASE,
>> > +        .request = request,
>> >          .flags = VHOST_USER_VERSION,
>> > -        .u64 = base,
>> > +        .u64 = file->index & VHOST_USER_VRING_IDX_MASK,
>> >          .size = sizeof(m.u64),
>> >      };
>> >
>> > -    if (shmfd && log->fd != -1) {
>> > -        fds[fd_num++] = log->fd;
>> > +    if (ioeventfd_enabled() && file->fd > 0) {
>> > +        fds[fd_num++] = file->fd;
>> > +    } else {
>> > +        msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
>> >      }
>> >
>> >      vhost_user_write(dev, &msg, fds, fd_num);
>> >
>> > -    if (shmfd) {
>> > -        msg.size = 0;
>> > -        if (vhost_user_read(dev, &msg) < 0) {
>> > -            return 0;
>> > -        }
>> > +    return 0;
>> > +}
>> >
>> > -        if (msg.request != VHOST_USER_SET_LOG_BASE) {
>> > -            error_report("Received unexpected msg type. "
>> > -                         "Expected %d received %d",
>> > -                         VHOST_USER_SET_LOG_BASE, msg.request);
>> > -            return -1;
>> > -        }
>> > +static int vhost_user_set_vring_kick(struct vhost_dev *dev,
>> > +                                     struct vhost_vring_file *file)
>> > +{
>> > +    return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_KICK, file);
>> > +}
>> > +
>> > +static int vhost_user_set_vring_call(struct vhost_dev *dev,
>> > +                                     struct vhost_vring_file *file)
>> > +{
>> > +    return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
>> > +}
>> > +
>> > +static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t
>> > u64)
>> > +{
>> > +    VhostUserMsg msg = {
>> > +        .request = request,
>> > +        .flags = VHOST_USER_VERSION,
>> > +        .u64 = u64,
>> > +        .size = sizeof(m.u64),
>> > +    };
>> > +
>> > +    vhost_user_write(dev, &msg, NULL, 0);
>> > +
>> > +    return 0;
>> > +}
>> > +
>> > +static int vhost_user_set_features(struct vhost_dev *dev,
>> > +                                   uint64_t features)
>> > +{
>> > +    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
>> > +}
>> > +
>> > +static int vhost_user_set_protocol_features(struct vhost_dev *dev,
>> > +                                            uint64_t features)
>> > +{
>> > +    return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES,
>> > features);
>> > +}
>> > +
>> > +static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t
>> > *u64)
>> > +{
>> > +    VhostUserMsg msg = {
>> > +        .request = request,
>> > +        .flags = VHOST_USER_VERSION,
>> > +    };
>> > +
>>
>> With multiqueue there are an issue with this implementation. The
>> VHOST_USER_GET_QUEUE_NUM message is sent through this function and it
>> is a one time message.
>> For the queue index different from 0 the vhost_user_write returns
>> immediately without sending the request to the backend.
>> Then the vhost_user_read never returns and QEMU is blocked.
>> I suggest to add the following test before calling the
>> vhost_user_write function to avoid this issue:
>>
>> +    if (vhost_user_one_time_request(request) && dev->vq_index != 0) {
>> +        return 0;
>> +    }
>> +
>
> Hmm, there are no tests for multi-queue? I thought we had some, how did you test it then?

For other reason I have launched a VM with multiqueue with your latest
patch series and I have encountered the bug. (easy to see as QEMU is
blocked for ever and the VM is not launched).
I will test the migration with multiqueue in order to check everything.

>
> thanks a lot for finding the issue!
>
>>
>> > +    vhost_user_write(dev, &msg, NULL, 0);
>> > +
>> > +    if (vhost_user_read(dev, &msg) < 0) {
>> > +        return 0;
>> > +    }
>> > +
>> > +    if (msg.request != request) {
>> > +        error_report("Received unexpected msg type. Expected %d received
>> > %d",
>> > +                     request, msg.request);
>> > +        return -1;
>> > +    }
>> > +
>> > +    if (msg.size != sizeof(m.u64)) {
>> > +        error_report("Received bad msg size.");
>> > +        return -1;
>> >      }
>> >
>> [snip]
>>
>> The attached file is the fixup to apply to this patch.
>>
>> Regards.
>>
>> Thibaut.
>>

Hi

----- Original Message -----
> >
> > Hmm, there are no tests for multi-queue? I thought we had some, how did you
> > test it then?
> 
> For other reason I have launched a VM with multiqueue with your latest
> patch series and I have encountered the bug. (easy to see as QEMU is
> blocked for ever and the VM is not launched).
> I will test the migration with multiqueue in order to check everything.

thanks, remember that migration requires backend support though.(do you have preliminary patches for dpdk?)