From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37598) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zjr73-0005ZT-Qv for qemu-devel@nongnu.org; Wed, 07 Oct 2015 11:58:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zjr6z-0003r2-Jn for qemu-devel@nongnu.org; Wed, 07 Oct 2015 11:58:41 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:42895) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zjr6z-0003qi-91 for qemu-devel@nongnu.org; Wed, 07 Oct 2015 11:58:37 -0400 Date: Wed, 7 Oct 2015 11:58:12 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1858575074.26311858.1444233492070.JavaMail.zimbra@redhat.com> In-Reply-To: References: <1443720248-15482-1-git-send-email-marcandre.lureau@redhat.com> <1443720248-15482-13-git-send-email-marcandre.lureau@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v7 12/24] vhost: use a function for each call List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thibaut Collet Cc: Linhaifeng , "Michael S. Tsirkin" , Jason Wang , qemu-devel , Paolo Bonzini , marcandre lureau Hi Thibaut ----- Original Message ----- > On Thu, Oct 1, 2015 at 7:23 PM, wrote: > > From: Marc-Andr=C3=A9 Lureau > > > > 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=C3=A9 Lureau > > --- > > 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 *d= ev, > > 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 bas= e, > > + 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 =3D 0; > > + bool shmfd =3D virtio_has_feature(dev->protocol_features, > > + VHOST_USER_PROTOCOL_F_LOG_SHMFD); > > + VhostUserMsg msg =3D { > > + .request =3D VHOST_USER_SET_LOG_BASE, > > + .flags =3D VHOST_USER_VERSION, > > + .u64 =3D base, > > + .size =3D sizeof(m.u64), > > + }; > > + > > + if (shmfd && log->fd !=3D -1) { > > + fds[fd_num++] =3D log->fd; > > } > > + > > + vhost_user_write(dev, &msg, fds, fd_num); > > + > > + if (shmfd) { > > + msg.size =3D 0; > > + if (vhost_user_read(dev, &msg) < 0) { > > + return 0; > > + } > > + > > + if (msg.request !=3D 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 =3D 0; > > - int need_reply =3D 0; > > int fds[VHOST_MEMORY_MAX_NREGIONS]; > > int i, fd; > > size_t fd_num =3D 0; > > + VhostUserMsg msg =3D { > > + .request =3D VHOST_USER_SET_MEM_TABLE, > > + .flags =3D VHOST_USER_VERSION, > > + }; > > > > - assert(dev->vhost_ops->backend_type =3D=3D VHOST_BACKEND_TYPE_USER= ); > > - > > - /* only translate vhost ioctl requests */ > > - if (request > VHOST_USER_MAX) { > > - msg_request =3D vhost_user_request_translate(request); > > - } else { > > - msg_request =3D request; > > + for (i =3D 0; i < dev->mem->nregions; ++i) { > > + struct vhost_memory_region *reg =3D dev->mem->regions + i; > > + ram_addr_t ram_addr; > > + > > + assert((uintptr_t)reg->userspace_addr =3D=3D reg->userspace_ad= dr); > > + qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr= , > > + &ram_addr); > > + fd =3D qemu_get_ram_fd(ram_addr); > > + if (fd > 0) { > > + msg.memory.regions[fd_num].userspace_addr =3D > > reg->userspace_addr; > > + msg.memory.regions[fd_num].memory_size =3D reg->memory_si= ze; > > + msg.memory.regions[fd_num].guest_phys_addr =3D > > reg->guest_phys_addr; > > + msg.memory.regions[fd_num].mmap_offset =3D reg->userspace_= addr - > > + (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr); > > + assert(fd_num < VHOST_MEMORY_MAX_NREGIONS); > > + fds[fd_num++] =3D 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 !=3D= 0) { > > - return 0; > > + msg.memory.nregions =3D fd_num; > > + > > + if (!fd_num) { > > + error_report("Failed initializing vhost-user memory map, " > > + "consider using -object memory-backend-file > > share=3Don"); > > + return -1; > > } > > > > - msg.request =3D msg_request; > > - msg.flags =3D VHOST_USER_VERSION; > > - msg.size =3D 0; > > + msg.size =3D sizeof(m.memory.nregions); > > + msg.size +=3D sizeof(m.memory.padding); > > + msg.size +=3D 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 =3D 1; > > - break; > > + vhost_user_write(dev, &msg, fds, fd_num); > > > > - case VHOST_USER_SET_FEATURES: > > - case VHOST_USER_SET_PROTOCOL_FEATURES: > > - msg.u64 =3D *((__u64 *) arg); > > - msg.size =3D 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 =3D { > > + .request =3D VHOST_USER_SET_VRING_ADDR, > > + .flags =3D VHOST_USER_VERSION, > > + .addr =3D *addr, > > + .size =3D sizeof(*addr), > > + }; > > > > - case VHOST_USER_SET_MEM_TABLE: > > - for (i =3D 0; i < dev->mem->nregions; ++i) { > > - struct vhost_memory_region *reg =3D dev->mem->regions + i; > > - ram_addr_t ram_addr; > > - > > - assert((uintptr_t)reg->userspace_addr =3D=3D reg->userspac= e_addr); > > - qemu_ram_addr_from_host((void > > *)(uintptr_t)reg->userspace_addr, &ram_addr); > > - fd =3D qemu_get_ram_fd(ram_addr); > > - if (fd > 0) { > > - msg.memory.regions[fd_num].userspace_addr =3D > > reg->userspace_addr; > > - msg.memory.regions[fd_num].memory_size =3D > > reg->memory_size; > > - msg.memory.regions[fd_num].guest_phys_addr =3D > > reg->guest_phys_addr; > > - msg.memory.regions[fd_num].mmap_offset =3D > > reg->userspace_addr - > > - (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr); > > - assert(fd_num < VHOST_MEMORY_MAX_NREGIONS); > > - fds[fd_num++] =3D fd; > > - } > > - } > > + vhost_user_write(dev, &msg, NULL, 0); > > > > - msg.memory.nregions =3D fd_num; > > + return 0; > > +} > > > > - if (!fd_num) { > > - error_report("Failed initializing vhost-user memory map, " > > - "consider using -object memory-backend-file > > share=3Don"); > > - 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 =3D sizeof(m.memory.nregions); > > - msg.size +=3D sizeof(m.memory.padding); > > - msg.size +=3D fd_num * sizeof(VhostUserMemoryRegion); > > - > > - break; > > - > > - case VHOST_USER_SET_LOG_FD: > > - fds[fd_num++] =3D *((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 =3D sizeof(m.state); > > - break; > > - > > - case VHOST_USER_GET_VRING_BASE: > > - memcpy(&msg.state, arg, sizeof(struct vhost_vring_state)); > > - msg.size =3D sizeof(m.state); > > - need_reply =3D 1; > > - break; > > - > > - case VHOST_USER_SET_VRING_ADDR: > > - memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr)); > > - msg.size =3D sizeof(m.addr); > > - break; > > - > > - case VHOST_USER_SET_VRING_KICK: > > - case VHOST_USER_SET_VRING_CALL: > > - case VHOST_USER_SET_VRING_ERR: > > - file =3D arg; > > - msg.u64 =3D file->index & VHOST_USER_VRING_IDX_MASK; > > - msg.size =3D sizeof(m.u64); > > - if (ioeventfd_enabled() && file->fd > 0) { > > - fds[fd_num++] =3D file->fd; > > - } else { > > - msg.u64 |=3D 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 =3D { > > + .request =3D request, > > + .flags =3D VHOST_USER_VERSION, > > + .state =3D *ring, > > + .size =3D 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); >=20 > 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); >=20 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); >=20 > 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 >=20 > > +} > > + > > +static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enab= le) > > +{ > > + struct vhost_vring_state state =3D { > > + .index =3D dev->vq_index, > > + .num =3D 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 =3D { > > + .request =3D VHOST_USER_GET_VRING_BASE, > > + .flags =3D VHOST_USER_VERSION, > > + .state =3D *ring, > > + .size =3D 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 !=3D msg.request) { > > - error_report("Received unexpected msg type." > > - " Expected %d received %d", msg_request, msg.reque= st); > > - return -1; > > - } > > + if (msg.request !=3D VHOST_USER_GET_VRING_BASE) { > > + error_report("Received unexpected msg type. Expected %d receiv= ed > > %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 !=3D sizeof(m.u64)) { > > - error_report("Received bad msg size."); > > - return -1; > > - } > > - *((__u64 *) arg) =3D msg.u64; > > - break; > > - case VHOST_USER_GET_VRING_BASE: > > - if (msg.size !=3D 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 !=3D sizeof(m.state)) { > > + error_report("Received bad msg size."); > > + return -1; > > } > > > > + *ring =3D 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 =3D 0; > > - bool shmfd =3D virtio_has_feature(dev->protocol_features, > > - VHOST_USER_PROTOCOL_F_LOG_SHMFD); > > VhostUserMsg msg =3D { > > - .request =3D VHOST_USER_SET_LOG_BASE, > > + .request =3D request, > > .flags =3D VHOST_USER_VERSION, > > - .u64 =3D base, > > + .u64 =3D file->index & VHOST_USER_VRING_IDX_MASK, > > .size =3D sizeof(m.u64), > > }; > > > > - if (shmfd && log->fd !=3D -1) { > > - fds[fd_num++] =3D log->fd; > > + if (ioeventfd_enabled() && file->fd > 0) { > > + fds[fd_num++] =3D file->fd; > > + } else { > > + msg.u64 |=3D VHOST_USER_VRING_NOFD_MASK; > > } > > > > vhost_user_write(dev, &msg, fds, fd_num); > > > > - if (shmfd) { > > - msg.size =3D 0; > > - if (vhost_user_read(dev, &msg) < 0) { > > - return 0; > > - } > > + return 0; > > +} > > > > - if (msg.request !=3D 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, uint= 64_t > > u64) > > +{ > > + VhostUserMsg msg =3D { > > + .request =3D request, > > + .flags =3D VHOST_USER_VERSION, > > + .u64 =3D u64, > > + .size =3D 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, uint= 64_t > > *u64) > > +{ > > + VhostUserMsg msg =3D { > > + .request =3D request, > > + .flags =3D VHOST_USER_VERSION, > > + }; > > + >=20 > 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: >=20 > + if (vhost_user_one_time_request(request) && dev->vq_index !=3D 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! >=20 > > + vhost_user_write(dev, &msg, NULL, 0); > > + > > + if (vhost_user_read(dev, &msg) < 0) { > > + return 0; > > + } > > + > > + if (msg.request !=3D request) { > > + error_report("Received unexpected msg type. Expected %d receiv= ed > > %d", > > + request, msg.request); > > + return -1; > > + } > > + > > + if (msg.size !=3D sizeof(m.u64)) { > > + error_report("Received bad msg size."); > > + return -1; > > } > > > [snip] >=20 > The attached file is the fixup to apply to this patch. >=20 > Regards. >=20 > Thibaut. >=20