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);
> +}
> +
> +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.
From effefe37d6ffa929657d788873311e026a5c0c80 Mon Sep 17 00:00:00 2001
From: Thibaut Collet <thibaut.col...@6wind.com>
Date: Wed, 7 Oct 2015 17:41:42 +0200
Subject: [PATCH] FIXUP for vhost: use a function for each call
Signed-off-by: Thibaut Collet <thibaut.col...@6wind.com>
---
hw/virtio/vhost-user.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index bbf5eeb..8b4ea0f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -315,13 +315,13 @@ static int vhost_set_vring(struct vhost_dev *dev,
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);
+ 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);
+ return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
}
static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
@@ -440,6 +440,10 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
.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) {
--
2.1.4
