Re: [Qemu-devel] [PULL 21/38] vhost: use a function for each call

[Laurent Desnogues]

Hello,

There are some errors with gcc 4.4.7 due to the use of an anonymous
union in VhostUserMsg in declarations with initialization. Please see
below.

Thanks.

On Wed, Oct 21, 2015 at 12:27 PM, Michael S. Tsirkin <address@hidden> wrote:
> From: Marc-André Lureau <address@hidden>
>
> 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 <address@hidden>
> [Fix enum usage and MQ - Thibaut Collet]
> Signed-off-by: Thibaut Collet <address@hidden>
> Reviewed-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> Tested-by: Thibaut Collet <address@hidden>
> ---
>  include/hw/virtio/vhost-backend.h |  63 ++++-
>  include/hw/virtio/vhost.h         |  12 +-
>  hw/net/vhost_net.c                |  16 +-
>  hw/scsi/vhost-scsi.c              |   7 +-
>  hw/virtio/vhost-backend.c         | 132 +++++++++-
>  hw/virtio/vhost-user.c            | 522 
> ++++++++++++++++++++++----------------
>  hw/virtio/vhost.c                 |  36 +--
>  7 files changed, 509 insertions(+), 279 deletions(-)
>
[...]
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index fb3aa40..16d8e80 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
[...]
> @@ -190,231 +182,321 @@ 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,

gcc 4.4.7 on CentOS 6.7 doesn't like this use of anonymous union.

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

Ditto.

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

Ditto.

> +        .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_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_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,

Ditto.

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

Ditto.

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

Ditto.

Thanks,

Laurent

> +        .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,
> +    };
> +
> +    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;
> +    }
> +
> +    *u64 = msg.u64;
> +
> +    return 0;
> +}
> +
> +static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
> +{
> +    return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
> +}
> +
> +static int vhost_user_set_owner(struct vhost_dev *dev)
> +{
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_SET_OWNER,
> +        .flags = VHOST_USER_VERSION,
> +    };
> +
> +    vhost_user_write(dev, &msg, NULL, 0);
> +
> +    return 0;
> +}
> +
> +static int vhost_user_reset_device(struct vhost_dev *dev)
> +{
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_RESET_DEVICE,
> +        .flags = VHOST_USER_VERSION,
> +    };
> +
> +    vhost_user_write(dev, &msg, NULL, 0);
> +
>      return 0;
>  }
>
>  static int vhost_user_init(struct vhost_dev *dev, void *opaque)
>  {
> -    unsigned long long features;
> +    uint64_t features;
>      int err;
>
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>
>      dev->opaque = opaque;
>
> -    err = vhost_user_call(dev, VHOST_USER_GET_FEATURES, &features);
> +    err = vhost_user_get_features(dev, &features);
>      if (err < 0) {
>          return err;
>      }
> @@ -422,21 +504,22 @@ static int vhost_user_init(struct vhost_dev *dev, void 
> *opaque)
>      if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>          dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
>
> -        err = vhost_user_call(dev, VHOST_USER_GET_PROTOCOL_FEATURES, 
> &features);
> +        err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES,
> +                                 &features);
>          if (err < 0) {
>              return err;
>          }
>
>          dev->protocol_features = features & VHOST_USER_PROTOCOL_FEATURE_MASK;
> -        err = vhost_user_call(dev, VHOST_USER_SET_PROTOCOL_FEATURES,
> -                              &dev->protocol_features);
> +        err = vhost_user_set_protocol_features(dev, dev->protocol_features);
>          if (err < 0) {
>              return err;
>          }
>
>          /* query the max queues we support if backend supports Multiple 
> Queue */
>          if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)) {
> -            err = vhost_user_call(dev, VHOST_USER_GET_QUEUE_NUM, 
> &dev->max_queues);
> +            err = vhost_user_get_u64(dev, VHOST_USER_GET_QUEUE_NUM,
> +                                     &dev->max_queues);
>              if (err < 0) {
>                  return err;
>              }
> @@ -454,22 +537,6 @@ static int vhost_user_init(struct vhost_dev *dev, void 
> *opaque)
>      return 0;
>  }
>
> -static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
> -{
> -    struct vhost_vring_state state = {
> -        .index = dev->vq_index,
> -        .num   = enable,
> -    };
> -
> -    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> -
> -    if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ))) {
> -        return -1;
> -    }
> -
> -    return vhost_user_call(dev, VHOST_USER_SET_VRING_ENABLE, &state);
> -}
> -
>  static int vhost_user_cleanup(struct vhost_dev *dev)
>  {
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> @@ -501,12 +568,23 @@ static bool vhost_user_requires_shm_log(struct 
> vhost_dev *dev)
>
>  const VhostOps user_ops = {
>          .backend_type = VHOST_BACKEND_TYPE_USER,
> -        .vhost_call = vhost_user_call,
>          .vhost_backend_init = vhost_user_init,
>          .vhost_backend_cleanup = vhost_user_cleanup,
> -        .vhost_backend_get_vq_index = vhost_user_get_vq_index,
> -        .vhost_backend_set_vring_enable = vhost_user_set_vring_enable,
>          .vhost_backend_memslots_limit = vhost_user_memslots_limit,
> -        .vhost_set_log_base = vhost_set_log_base,
> +        .vhost_set_log_base = vhost_user_set_log_base,
> +        .vhost_set_mem_table = vhost_user_set_mem_table,
> +        .vhost_set_vring_addr = vhost_user_set_vring_addr,
> +        .vhost_set_vring_endian = vhost_user_set_vring_endian,
> +        .vhost_set_vring_num = vhost_user_set_vring_num,
> +        .vhost_set_vring_base = vhost_user_set_vring_base,
> +        .vhost_get_vring_base = vhost_user_get_vring_base,
> +        .vhost_set_vring_kick = vhost_user_set_vring_kick,
> +        .vhost_set_vring_call = vhost_user_set_vring_call,
> +        .vhost_set_features = vhost_user_set_features,
> +        .vhost_get_features = vhost_user_get_features,
> +        .vhost_set_owner = vhost_user_set_owner,
> +        .vhost_reset_device = vhost_user_reset_device,
> +        .vhost_get_vq_index = vhost_user_get_vq_index,
> +        .vhost_set_vring_enable = vhost_user_set_vring_enable,
>          .vhost_requires_shm_log = vhost_user_requires_shm_log,
>  };