From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42071) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agxmT-0006rH-Tr for qemu-devel@nongnu.org; Fri, 18 Mar 2016 13:01:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1agxmL-00088o-2H for qemu-devel@nongnu.org; Fri, 18 Mar 2016 13:01:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34751) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agxmK-00088B-OK for qemu-devel@nongnu.org; Fri, 18 Mar 2016 13:01:36 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 531643D48F for ; Fri, 18 Mar 2016 17:01:36 +0000 (UTC) Received: from blackfin.pond.sub.org (ovpn-116-34.ams2.redhat.com [10.36.116.34]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2IH1WFJ009469 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO) for ; Fri, 18 Mar 2016 13:01:34 -0400 From: Markus Armbruster Date: Fri, 18 Mar 2016 18:01:13 +0100 Message-Id: <1458320487-19603-27-git-send-email-armbru@redhat.com> In-Reply-To: <1458320487-19603-1-git-send-email-armbru@redhat.com> References: <1458320487-19603-1-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] [PULL 26/40] ivshmem: Propagate errors through ivshmem_recv_setup() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org This kills off the funny state described in the previous commit. Simplify ivshmem_io_read() accordingly, and update documentation. Signed-off-by: Markus Armbruster Message-Id: <1458066895-20632-27-git-send-email-armbru@redhat.com> Reviewed-by: Marc-Andr=C3=A9 Lureau --- docs/specs/ivshmem-spec.txt | 20 +++---- hw/misc/ivshmem.c | 129 ++++++++++++++++++++++++++++----------= ------ qemu-doc.texi | 9 +--- 3 files changed, 95 insertions(+), 63 deletions(-) diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt index 0cd63ad..4c33973 100644 --- a/docs/specs/ivshmem-spec.txt +++ b/docs/specs/ivshmem-spec.txt @@ -62,11 +62,11 @@ There are two ways to use this device: likely want to write a kernel driver to handle interrupts. Requires the device to be configured for interrupts, obviously. =20 -If the device is configured for interrupts, BAR2 is initially invalid. -It becomes safely accessible only after the ivshmem server provided -the shared memory. Guest software should wait for the IVPosition -register (described below) to become non-negative before accessing -BAR2. +Before QEMU 2.6.0, BAR2 can initially be invalid if the device is +configured for interrupts. It becomes safely accessible only after +the ivshmem server provided the shared memory. Guest software should +wait for the IVPosition register (described below) to become +non-negative before accessing BAR2. =20 The device is not capable to tell guest software whether it is configured for interrupts. @@ -82,7 +82,7 @@ BAR 0 contains the following registers: 4 4 read/write 0 Interrupt Status bit 0: peer interrupt bit 1..31: reserved - 8 4 read-only 0 or -1 IVPosition + 8 4 read-only 0 or ID IVPosition 12 4 write-only N/A Doorbell bit 0..15: vector bit 16..31: peer ID @@ -100,12 +100,14 @@ when an interrupt request from a peer is received. = Reading the register clears it. =20 IVPosition Register: if the device is not configured for interrupts, -this is zero. Else, it's -1 for a short while after reset, then -changes to the device's ID (between 0 and 65535). +this is zero. Else, it is the device's ID (between 0 and 65535). + +Before QEMU 2.6.0, the register may read -1 for a short while after +reset. =20 There is no good way for software to find out whether the device is configured for interrupts. A positive IVPosition means interrupts, -but zero could be either. The initial -1 cannot be reliably observed. +but zero could be either. =20 Doorbell Register: writing this register requests to interrupt a peer. The written value's high 16 bits are the ID of the peer to interrupt, diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index ad16828..7f439c3 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -234,12 +234,7 @@ static uint64_t ivshmem_io_read(void *opaque, hwaddr= addr, break; =20 case IVPOSITION: - /* return my VM ID if the memory is mapped */ - if (memory_region_is_mapped(&s->ivshmem)) { - ret =3D s->vm_id; - } else { - ret =3D -1; - } + ret =3D s->vm_id; break; =20 default: @@ -511,7 +506,8 @@ static bool fifo_update_and_get_i64(IVShmemState *s, return false; } =20 -static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector) +static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector, + Error **errp) { PCIDevice *pdev =3D PCI_DEVICE(s); MSIMessage msg =3D msix_get_message(pdev, vector); @@ -522,22 +518,21 @@ static int ivshmem_add_kvm_msi_virq(IVShmemState *s= , int vector) =20 ret =3D kvm_irqchip_add_msi_route(kvm_state, msg, pdev); if (ret < 0) { - error_report("ivshmem: kvm_irqchip_add_msi_route failed"); - return -1; + error_setg(errp, "kvm_irqchip_add_msi_route failed"); + return; } =20 s->msi_vectors[vector].virq =3D ret; s->msi_vectors[vector].pdev =3D pdev; - - return 0; } =20 -static void setup_interrupt(IVShmemState *s, int vector) +static void setup_interrupt(IVShmemState *s, int vector, Error **errp) { EventNotifier *n =3D &s->peers[s->vm_id].eventfds[vector]; bool with_irqfd =3D kvm_msi_via_irqfd_enabled() && ivshmem_has_feature(s, IVSHMEM_MSI); PCIDevice *pdev =3D PCI_DEVICE(s); + Error *err =3D NULL; =20 IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector); =20 @@ -546,13 +541,16 @@ static void setup_interrupt(IVShmemState *s, int ve= ctor) watch_vector_notifier(s, n, vector); } else if (msix_enabled(pdev)) { IVSHMEM_DPRINTF("with irqfd\n"); - if (ivshmem_add_kvm_msi_virq(s, vector) < 0) { + ivshmem_add_kvm_msi_virq(s, vector, &err); + if (err) { + error_propagate(errp, err); return; } =20 if (!msix_is_masked(pdev, vector)) { kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, s->msi_vectors[vector].vi= rq); + /* TODO handle error */ } } else { /* it will be delayed until msix is enabled, in write_config */ @@ -560,19 +558,19 @@ static void setup_interrupt(IVShmemState *s, int ve= ctor) } } =20 -static void process_msg_shmem(IVShmemState *s, int fd) +static void process_msg_shmem(IVShmemState *s, int fd, Error **errp) { Error *err =3D NULL; void *ptr; =20 if (memory_region_is_mapped(&s->ivshmem)) { - error_report("shm already initialized"); + error_setg(errp, "server sent unexpected shared memory message")= ; close(fd); return; } =20 if (check_shm_size(s, fd, &err) =3D=3D -1) { - error_report_err(err); + error_propagate(errp, err); close(fd); return; } @@ -580,7 +578,7 @@ static void process_msg_shmem(IVShmemState *s, int fd= ) /* mmap the region and map into the BAR2 */ ptr =3D mmap(0, s->ivshmem_size, PROT_READ | PROT_WRITE, MAP_SHARED,= fd, 0); if (ptr =3D=3D MAP_FAILED) { - error_report("Failed to mmap shared memory %s", strerror(errno))= ; + error_setg_errno(errp, errno, "Failed to mmap shared memory"); close(fd); return; } @@ -591,17 +589,19 @@ static void process_msg_shmem(IVShmemState *s, int = fd) memory_region_add_subregion(&s->bar, 0, &s->ivshmem); } =20 -static void process_msg_disconnect(IVShmemState *s, uint16_t posn) +static void process_msg_disconnect(IVShmemState *s, uint16_t posn, + Error **errp) { IVSHMEM_DPRINTF("posn %d has gone away\n", posn); if (posn >=3D s->nb_peers || posn =3D=3D s->vm_id) { - error_report("invalid peer %d", posn); + error_setg(errp, "invalid peer %d", posn); return; } close_peer_eventfds(s, posn); } =20 -static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd) +static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd, + Error **errp) { Peer *peer =3D &s->peers[posn]; int vector; @@ -611,8 +611,8 @@ static void process_msg_connect(IVShmemState *s, uint= 16_t posn, int fd) * descriptor for vector N-1. Count messages to find the vector. */ if (peer->nb_eventfds >=3D s->vectors) { - error_report("Too many eventfd received, device has %d vectors", - s->vectors); + error_setg(errp, "Too many eventfd received, device has %d vecto= rs", + s->vectors); close(fd); return; } @@ -623,7 +623,8 @@ static void process_msg_connect(IVShmemState *s, uint= 16_t posn, int fd) fcntl_setfl(fd, O_NONBLOCK); /* msix/irqfd poll non block */ =20 if (posn =3D=3D s->vm_id) { - setup_interrupt(s, vector); + setup_interrupt(s, vector, errp); + /* TODO do we need to handle the error? */ } =20 if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) { @@ -631,18 +632,18 @@ static void process_msg_connect(IVShmemState *s, ui= nt16_t posn, int fd) } } =20 -static void process_msg(IVShmemState *s, int64_t msg, int fd) +static void process_msg(IVShmemState *s, int64_t msg, int fd, Error **er= rp) { IVSHMEM_DPRINTF("posn is %" PRId64 ", fd is %d\n", msg, fd); =20 if (msg < -1 || msg > IVSHMEM_MAX_PEERS) { - error_report("server sent invalid message %" PRId64, msg); + error_setg(errp, "server sent invalid message %" PRId64, msg); close(fd); return; } =20 if (msg =3D=3D -1) { - process_msg_shmem(s, fd); + process_msg_shmem(s, fd, errp); return; } =20 @@ -651,17 +652,18 @@ static void process_msg(IVShmemState *s, int64_t ms= g, int fd) } =20 if (fd >=3D 0) { - process_msg_connect(s, msg, fd); + process_msg_connect(s, msg, fd, errp); } else if (s->vm_id =3D=3D -1) { s->vm_id =3D msg; } else { - process_msg_disconnect(s, msg); + process_msg_disconnect(s, msg, errp); } } =20 static void ivshmem_read(void *opaque, const uint8_t *buf, int size) { IVShmemState *s =3D opaque; + Error *err =3D NULL; int fd; int64_t msg; =20 @@ -672,10 +674,13 @@ static void ivshmem_read(void *opaque, const uint8_= t *buf, int size) fd =3D qemu_chr_fe_get_msgfd(s->server_chr); IVSHMEM_DPRINTF("posn is %" PRId64 ", fd is %d\n", msg, fd); =20 - process_msg(s, msg, fd); + process_msg(s, msg, fd, &err); + if (err) { + error_report_err(err); + } } =20 -static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd) +static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd, Error **errp) { int64_t msg; int n, ret; @@ -685,7 +690,7 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int = *pfd) ret =3D qemu_chr_fe_read_all(s->server_chr, (uint8_t *)&msg + n, sizeof(msg) - n); if (ret < 0 && ret !=3D -EINTR) { - /* TODO error handling */ + error_setg_errno(errp, -ret, "read from server failed"); return INT64_MIN; } n +=3D ret; @@ -695,15 +700,24 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, in= t *pfd) return msg; } =20 -static void ivshmem_recv_setup(IVShmemState *s) +static void ivshmem_recv_setup(IVShmemState *s, Error **errp) { + Error *err =3D NULL; int64_t msg; int fd; =20 - msg =3D ivshmem_recv_msg(s, &fd); - if (fd !=3D -1 || msg !=3D IVSHMEM_PROTOCOL_VERSION) { - fprintf(stderr, "incompatible version, you are connecting to a i= vshmem-" - "server using a different protocol please check your set= up\n"); + msg =3D ivshmem_recv_msg(s, &fd, &err); + if (err) { + error_propagate(errp, err); + return; + } + if (msg !=3D IVSHMEM_PROTOCOL_VERSION) { + error_setg(errp, "server sent version %" PRId64 ", expecting %d"= , + msg, IVSHMEM_PROTOCOL_VERSION); + return; + } + if (fd !=3D -1) { + error_setg(errp, "server sent invalid version message"); return; } =20 @@ -711,9 +725,25 @@ static void ivshmem_recv_setup(IVShmemState *s) * Receive more messages until we got shared memory. */ do { - msg =3D ivshmem_recv_msg(s, &fd); - process_msg(s, msg, fd); + msg =3D ivshmem_recv_msg(s, &fd, &err); + if (err) { + error_propagate(errp, err); + return; + } + process_msg(s, msg, fd, &err); + if (err) { + error_propagate(errp, err); + return; + } } while (msg !=3D -1); + + /* + * This function must either map the shared memory or fail. The + * loop above ensures that: it terminates normally only after it + * successfully processed the server's shared memory message. + * Assert that actually mapped the shared memory: + */ + assert(memory_region_is_mapped(&s->ivshmem)); } =20 /* Select the MSI-X vectors used by device. @@ -763,7 +793,13 @@ static void ivshmem_enable_irqfd(IVShmemState *s) int i; =20 for (i =3D 0; i < s->peers[s->vm_id].nb_eventfds; i++) { - ivshmem_add_kvm_msi_virq(s, i); + Error *err =3D NULL; + + ivshmem_add_kvm_msi_virq(s, i, &err); + if (err) { + error_report_err(err); + /* TODO do we need to handle the error? */ + } } =20 if (msix_set_vector_notifiers(pdev, @@ -809,7 +845,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uin= t32_t address, pci_default_write_config(pdev, address, val, len); is_enabled =3D msix_enabled(pdev); =20 - if (kvm_msi_via_irqfd_enabled() && s->vm_id !=3D -1) { + if (kvm_msi_via_irqfd_enabled()) { if (!was_enabled && is_enabled) { ivshmem_enable_irqfd(s); } else if (was_enabled && !is_enabled) { @@ -928,15 +964,16 @@ static void pci_ivshmem_realize(PCIDevice *dev, Err= or **errp) * Receive setup messages from server synchronously. * Older versions did it asynchronously, but that creates a * number of entertaining race conditions. - * TODO Propagate errors! Without that, we still have races - * on errors. */ - ivshmem_recv_setup(s); - if (memory_region_is_mapped(&s->ivshmem)) { - qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, - ivshmem_read, NULL, s); + ivshmem_recv_setup(s, &err); + if (err) { + error_propagate(errp, err); + return; } =20 + qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, + ivshmem_read, NULL, s); + if (ivshmem_setup_interrupts(s) < 0) { error_setg(errp, "failed to initialize interrupts"); return; diff --git a/qemu-doc.texi b/qemu-doc.texi index 65f3b29..8afbbcd 100644 --- a/qemu-doc.texi +++ b/qemu-doc.texi @@ -1289,14 +1289,7 @@ qemu-system-i386 -device ivshmem,size=3D@var{shm-s= ize},vectors=3D@var{vectors},chard =20 When using the server, the guest will be assigned a VM ID (>=3D0) that a= llows guests using the same server to communicate via interrupts. Guests can read th= eir -VM ID from a device register (see example code). Since receiving the sh= ared -memory region from the server is asynchronous, there is a (small) chance= the -guest may boot before the shared memory is attached. To allow an applic= ation -to ensure shared memory is attached, the VM ID register will return -1 (= an -invalid VM ID) until the memory is attached. Once the shared memory is -attached, the VM ID will return the guest's valid VM ID. With these sem= antics, -the guest application can check to ensure the shared memory is attached = to the -guest before proceeding. +VM ID from a device register (see ivshmem-spec.txt). =20 The @option{role} argument can be set to either master or peer and will = affect how the shared memory is migrated. With @option{role=3Dmaster}, the gue= st will --=20 2.4.3