On Wed, Oct 7, 2015 at 5:58 PM, Marc-André Lureau <mlur...@redhat.com> wrote:
> Hi Thibaut
>
> ----- Original Message -----
>> On Thu, Oct 1, 2015 at 7:23 PM, <marcandre.lur...@redhat.com> wrote:
>> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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.
>>
