| Message ID | 1456771254-17511-25-git-send-email-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> wrote: > This kills off the funny state described in the previous commit. > > Simplify ivshmem_io_read() accordingly, and update documentation. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > docs/specs/ivshmem-spec.txt | 20 ++++---- > hw/misc/ivshmem.c | 121 +++++++++++++++++++++++++++----------------- > qemu-doc.texi | 9 +--- > 3 files changed, 87 insertions(+), 63 deletions(-) > > diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt > index 4fc6f37..3eb8c97 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. > > -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. > > 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. > > 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. > > 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. > > 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 352937f..831da53 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; > > case IVPOSITION: > - /* return my VM ID if the memory is mapped */ > - if (memory_region_is_mapped(&s->ivshmem)) { > - ret = s->vm_id; > - } else { > - ret = -1; > - } > + ret = s->vm_id; > break; > > default: > @@ -511,7 +506,8 @@ static bool fifo_update_and_get_i64(IVShmemState *s, > return false; > } > > -static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector) > +static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector, > + Error **errp) I prefer to return -1 in case of error, even if Error** is also returned. > { > PCIDevice *pdev = PCI_DEVICE(s); > MSIMessage msg = msix_get_message(pdev, vector); > @@ -522,22 +518,21 @@ static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector) > > ret = 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; > } > > s->msi_vectors[vector].virq = ret; > s->msi_vectors[vector].pdev = pdev; > - > - return 0; > } > > -static void setup_interrupt(IVShmemState *s, int vector) > +static void setup_interrupt(IVShmemState *s, int vector, Error **errp) > { > EventNotifier *n = &s->peers[s->vm_id].eventfds[vector]; > bool with_irqfd = kvm_msi_via_irqfd_enabled() && > ivshmem_has_feature(s, IVSHMEM_MSI); > PCIDevice *pdev = PCI_DEVICE(s); > + Error *err = NULL; > > IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector); > > @@ -546,13 +541,16 @@ static void setup_interrupt(IVShmemState *s, int vector) > 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; That would make this simpler, avoiding local err variables, and propagate. But you seem to prefer that form. I don't know if there is any conventions (I am used to glib conventions, and usually a bool success is returned, even if the function takes a GError) > > if (!msix_is_masked(pdev, vector)) { > kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, > s->msi_vectors[vector].virq); > + /* 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 vector) > } > } > > -static void process_msg_shmem(IVShmemState *s, int fd) > +static void process_msg_shmem(IVShmemState *s, int fd, Error **errp) > { > Error *err = NULL; > void *ptr; > > if (memory_region_is_mapped(&s->ivshmem)) { > - error_report("shm already initialized"); > + error_setg(errp, "server sent unexpected shared memory message"); > close(fd); > return; > } > > if (check_shm_size(s, fd, &err) == -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 = mmap(0, s->ivshmem_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > if (ptr == 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); > } > > -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 >= s->nb_peers) { > - error_report("invalid peer %d", posn); > + error_setg(errp, "invalid peer %d", posn); > return; > } > close_peer_eventfds(s, posn); > } > > -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 = &s->peers[posn]; > int vector; > @@ -611,8 +611,8 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd) > * descriptor for vector N-1. Count messages to find the vector. > */ > if (peer->nb_eventfds >= s->vectors) { > - error_report("Too many eventfd received, device has %d vectors", > - s->vectors); > + error_setg(errp, "Too many eventfd received, device has %d vectors", > + s->vectors); > close(fd); > return; > } > @@ -623,7 +623,8 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd) > fcntl_setfl(fd, O_NONBLOCK); /* msix/irqfd poll non block */ > > if (posn == s->vm_id) { > - setup_interrupt(s, vector); > + setup_interrupt(s, vector, errp); > + /* TODO do we need to handle the error? */ > } > > if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) { > @@ -631,18 +632,18 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd) > } > } > > -static void process_msg(IVShmemState *s, int64_t msg, int fd) > +static void process_msg(IVShmemState *s, int64_t msg, int fd, Error **errp) > { > IVSHMEM_DPRINTF("posn is %" PRId64 ", fd is %d\n", msg, fd); > > 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; > } > > if (msg == -1) { > - process_msg_shmem(s, fd); > + process_msg_shmem(s, fd, errp); > return; > } > > @@ -651,17 +652,18 @@ static void process_msg(IVShmemState *s, int64_t msg, int fd) > } > > if (fd >= 0) { > - process_msg_connect(s, msg, fd); > + process_msg_connect(s, msg, fd, errp); > } else if (s->vm_id == -1) { > s->vm_id = msg; > } else { > - process_msg_disconnect(s, msg); > + process_msg_disconnect(s, msg, errp); > } > } > > static void ivshmem_read(void *opaque, const uint8_t *buf, int size) > { > IVShmemState *s = opaque; > + Error *err = NULL; > int incoming_fd; > int64_t incoming_posn; > > @@ -673,10 +675,13 @@ static void ivshmem_read(void *opaque, const uint8_t *buf, int size) > IVSHMEM_DPRINTF("posn is %" PRId64 ", fd is %d\n", > incoming_posn, incoming_fd); > > - process_msg(s, incoming_posn, incoming_fd); > + process_msg(s, incoming_posn, incoming_fd, &err); > + if (err) { > + error_report_err(err); > + } > } > > -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; > @@ -686,7 +691,7 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd) > ret = qemu_chr_fe_read_all(s->server_chr, (uint8_t *)&msg + n, > sizeof(msg) - n); > if (ret < 0 && ret != -EINTR) { > - /* TODO error handling */ > + error_setg_errno(errp, -ret, "read from server failed"); > return INT64_MIN; > } > n += ret; > @@ -696,15 +701,24 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd) > return msg; > } > > -static void ivshmem_recv_setup(IVShmemState *s) > +static void ivshmem_recv_setup(IVShmemState *s, Error **errp) > { > + Error *err = NULL; > int64_t msg; > int fd; > > - msg = ivshmem_recv_msg(s, &fd); > - if (fd != -1 || msg != IVSHMEM_PROTOCOL_VERSION) { > - fprintf(stderr, "incompatible version, you are connecting to a ivshmem-" > - "server using a different protocol please check your setup\n"); > + msg = ivshmem_recv_msg(s, &fd, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + if (msg != IVSHMEM_PROTOCOL_VERSION) { > + error_setg(errp, "server sent version %" PRId64 ", expecting %d", > + msg, IVSHMEM_PROTOCOL_VERSION); > + return; > + } > + if (fd != -1) { > + error_setg(errp, "server sent invalid version message"); > return; > } > > @@ -712,8 +726,16 @@ static void ivshmem_recv_setup(IVShmemState *s) > * Receive more messages until we got shared memory. > */ > do { > - msg = ivshmem_recv_msg(s, &fd); > - process_msg(s, msg, fd); > + msg = 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 != -1); > > assert(memory_region_is_mapped(&s->ivshmem)); > @@ -768,7 +790,13 @@ static void ivshmem_enable_irqfd(IVShmemState *s) > int i; > > for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) { > - ivshmem_add_kvm_msi_virq(s, i); > + Error *err = NULL; > + > + ivshmem_add_kvm_msi_virq(s, i, &err); > + if (err) { > + error_report_err(err); > + /* TODO do we need to handle the error? */ > + } > } > > if (msix_set_vector_notifiers(pdev, > @@ -814,7 +842,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address, > pci_default_write_config(pdev, address, val, len); > is_enabled = msix_enabled(pdev); > > - if (kvm_msi_via_irqfd_enabled() && s->vm_id != -1) { > + if (kvm_msi_via_irqfd_enabled()) { > if (!was_enabled && is_enabled) { > ivshmem_enable_irqfd(s); > } else if (was_enabled && !is_enabled) { > @@ -933,15 +961,16 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **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; > } > > + 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=@var{shm-size},vectors=@var{vectors},chard > > When using the server, the guest will be assigned a VM ID (>=0) that allows guests > using the same server to communicate via interrupts. Guests can read their > -VM ID from a device register (see example code). Since receiving the shared > -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 application > -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 semantics, > -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). > > The @option{role} argument can be set to either master or peer and will affect > how the shared memory is migrated. With @option{role=master}, the guest will > -- > 2.4.3 > > looks good otherwise
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> wrote: >> This kills off the funny state described in the previous commit. >> >> Simplify ivshmem_io_read() accordingly, and update documentation. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> docs/specs/ivshmem-spec.txt | 20 ++++---- >> hw/misc/ivshmem.c | 121 +++++++++++++++++++++++++++----------------- >> qemu-doc.texi | 9 +--- >> 3 files changed, 87 insertions(+), 63 deletions(-) >> >> diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt >> index 4fc6f37..3eb8c97 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. >> >> -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. >> >> 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. >> >> 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. >> >> 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. >> >> 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 352937f..831da53 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; >> >> case IVPOSITION: >> - /* return my VM ID if the memory is mapped */ >> - if (memory_region_is_mapped(&s->ivshmem)) { >> - ret = s->vm_id; >> - } else { >> - ret = -1; >> - } >> + ret = s->vm_id; >> break; >> >> default: >> @@ -511,7 +506,8 @@ static bool fifo_update_and_get_i64(IVShmemState *s, >> return false; >> } >> >> -static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector) >> +static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector, >> + Error **errp) > > I prefer to return -1 in case of error, even if Error** is also returned. You know, I'd prefer that, too, and I've argued for it unsuccessfully. As it is, we fairly consistently return void when the function returns errors through Error ** and has no non-error value. >> { >> PCIDevice *pdev = PCI_DEVICE(s); >> MSIMessage msg = msix_get_message(pdev, vector); >> @@ -522,22 +518,21 @@ static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector) >> >> ret = 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; >> } >> >> s->msi_vectors[vector].virq = ret; >> s->msi_vectors[vector].pdev = pdev; >> - >> - return 0; >> } >> >> -static void setup_interrupt(IVShmemState *s, int vector) >> +static void setup_interrupt(IVShmemState *s, int vector, Error **errp) >> { >> EventNotifier *n = &s->peers[s->vm_id].eventfds[vector]; >> bool with_irqfd = kvm_msi_via_irqfd_enabled() && >> ivshmem_has_feature(s, IVSHMEM_MSI); >> PCIDevice *pdev = PCI_DEVICE(s); >> + Error *err = NULL; >> >> IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector); >> >> @@ -546,13 +541,16 @@ static void setup_interrupt(IVShmemState *s, int vector) >> 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; > > That would make this simpler, avoiding local err variables, and > propagate. But you seem to prefer that form. I don't know if there is > any conventions (I am used to glib conventions, and usually a bool > success is returned, even if the function takes a GError) Does GLib spell out this convention somewhere? I can perhaps try to cook up a patch to demonstrate the advantages of returning a success/failure value even with Error **, and try to get our convention changed. Until then, we better stick to the existing convention, whether we like it or not. Thanks! [...]
Hi On Wed, Mar 2, 2016 at 8:35 PM, Markus Armbruster <armbru@redhat.com> wrote: > You know, I'd prefer that, too, and I've argued for it unsuccessfully. > As it is, we fairly consistently return void when the function returns > errors through Error ** and has no non-error value. Good to know we are on same side. >>> { >>> PCIDevice *pdev = PCI_DEVICE(s); >>> MSIMessage msg = msix_get_message(pdev, vector); >>> @@ -522,22 +518,21 @@ static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector) >>> >>> ret = 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; >>> } >>> >>> s->msi_vectors[vector].virq = ret; >>> s->msi_vectors[vector].pdev = pdev; >>> - >>> - return 0; >>> } >>> >>> -static void setup_interrupt(IVShmemState *s, int vector) >>> +static void setup_interrupt(IVShmemState *s, int vector, Error **errp) >>> { >>> EventNotifier *n = &s->peers[s->vm_id].eventfds[vector]; >>> bool with_irqfd = kvm_msi_via_irqfd_enabled() && >>> ivshmem_has_feature(s, IVSHMEM_MSI); >>> PCIDevice *pdev = PCI_DEVICE(s); >>> + Error *err = NULL; >>> >>> IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector); >>> >>> @@ -546,13 +541,16 @@ static void setup_interrupt(IVShmemState *s, int vector) >>> 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; >> >> That would make this simpler, avoiding local err variables, and >> propagate. But you seem to prefer that form. I don't know if there is >> any conventions (I am used to glib conventions, and usually a bool >> success is returned, even if the function takes a GError) > > Does GLib spell out this convention somewhere? For glib, there is a paragraph about return bool/GError conventions (which is usually adapted to other return type): https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html > > I can perhaps try to cook up a patch to demonstrate the advantages of > returning a success/failure value even with Error **, and try to get our > convention changed. > > Until then, we better stick to the existing convention, whether we like > it or not. ok
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Wed, Mar 2, 2016 at 8:35 PM, Markus Armbruster <armbru@redhat.com> wrote: >> You know, I'd prefer that, too, and I've argued for it unsuccessfully. >> As it is, we fairly consistently return void when the function returns >> errors through Error ** and has no non-error value. > > Good to know we are on same side. > >>>> { >>>> PCIDevice *pdev = PCI_DEVICE(s); >>>> MSIMessage msg = msix_get_message(pdev, vector); >>>> @@ -522,22 +518,21 @@ static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector) >>>> >>>> ret = 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; >>>> } >>>> >>>> s->msi_vectors[vector].virq = ret; >>>> s->msi_vectors[vector].pdev = pdev; >>>> - >>>> - return 0; >>>> } >>>> >>>> -static void setup_interrupt(IVShmemState *s, int vector) >>>> +static void setup_interrupt(IVShmemState *s, int vector, Error **errp) >>>> { >>>> EventNotifier *n = &s->peers[s->vm_id].eventfds[vector]; >>>> bool with_irqfd = kvm_msi_via_irqfd_enabled() && >>>> ivshmem_has_feature(s, IVSHMEM_MSI); >>>> PCIDevice *pdev = PCI_DEVICE(s); >>>> + Error *err = NULL; >>>> >>>> IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector); >>>> >>>> @@ -546,13 +541,16 @@ static void setup_interrupt(IVShmemState *s, int vector) >>>> 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; >>> >>> That would make this simpler, avoiding local err variables, and >>> propagate. But you seem to prefer that form. I don't know if there is >>> any conventions (I am used to glib conventions, and usually a bool >>> success is returned, even if the function takes a GError) >> >> Does GLib spell out this convention somewhere? > > For glib, there is a paragraph about return bool/GError conventions > (which is usually adapted to other return type): > https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html While I can't see a hard-and-fast rule there, the text clearly shows a strong preference for making the function value a reliable error indicator whenever possible. Thanks! >> >> I can perhaps try to cook up a patch to demonstrate the advantages of >> returning a success/failure value even with Error **, and try to get our >> convention changed. >> >> Until then, we better stick to the existing convention, whether we like >> it or not. > > ok
diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt index 4fc6f37..3eb8c97 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. -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. 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. 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. 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. 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 352937f..831da53 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; case IVPOSITION: - /* return my VM ID if the memory is mapped */ - if (memory_region_is_mapped(&s->ivshmem)) { - ret = s->vm_id; - } else { - ret = -1; - } + ret = s->vm_id; break; default: @@ -511,7 +506,8 @@ static bool fifo_update_and_get_i64(IVShmemState *s, return false; } -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 = PCI_DEVICE(s); MSIMessage msg = msix_get_message(pdev, vector); @@ -522,22 +518,21 @@ static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector) ret = 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; } s->msi_vectors[vector].virq = ret; s->msi_vectors[vector].pdev = pdev; - - return 0; } -static void setup_interrupt(IVShmemState *s, int vector) +static void setup_interrupt(IVShmemState *s, int vector, Error **errp) { EventNotifier *n = &s->peers[s->vm_id].eventfds[vector]; bool with_irqfd = kvm_msi_via_irqfd_enabled() && ivshmem_has_feature(s, IVSHMEM_MSI); PCIDevice *pdev = PCI_DEVICE(s); + Error *err = NULL; IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector); @@ -546,13 +541,16 @@ static void setup_interrupt(IVShmemState *s, int vector) 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; } if (!msix_is_masked(pdev, vector)) { kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, s->msi_vectors[vector].virq); + /* 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 vector) } } -static void process_msg_shmem(IVShmemState *s, int fd) +static void process_msg_shmem(IVShmemState *s, int fd, Error **errp) { Error *err = NULL; void *ptr; if (memory_region_is_mapped(&s->ivshmem)) { - error_report("shm already initialized"); + error_setg(errp, "server sent unexpected shared memory message"); close(fd); return; } if (check_shm_size(s, fd, &err) == -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 = mmap(0, s->ivshmem_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); if (ptr == 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); } -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 >= s->nb_peers) { - error_report("invalid peer %d", posn); + error_setg(errp, "invalid peer %d", posn); return; } close_peer_eventfds(s, posn); } -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 = &s->peers[posn]; int vector; @@ -611,8 +611,8 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd) * descriptor for vector N-1. Count messages to find the vector. */ if (peer->nb_eventfds >= s->vectors) { - error_report("Too many eventfd received, device has %d vectors", - s->vectors); + error_setg(errp, "Too many eventfd received, device has %d vectors", + s->vectors); close(fd); return; } @@ -623,7 +623,8 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd) fcntl_setfl(fd, O_NONBLOCK); /* msix/irqfd poll non block */ if (posn == s->vm_id) { - setup_interrupt(s, vector); + setup_interrupt(s, vector, errp); + /* TODO do we need to handle the error? */ } if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) { @@ -631,18 +632,18 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd) } } -static void process_msg(IVShmemState *s, int64_t msg, int fd) +static void process_msg(IVShmemState *s, int64_t msg, int fd, Error **errp) { IVSHMEM_DPRINTF("posn is %" PRId64 ", fd is %d\n", msg, fd); 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; } if (msg == -1) { - process_msg_shmem(s, fd); + process_msg_shmem(s, fd, errp); return; } @@ -651,17 +652,18 @@ static void process_msg(IVShmemState *s, int64_t msg, int fd) } if (fd >= 0) { - process_msg_connect(s, msg, fd); + process_msg_connect(s, msg, fd, errp); } else if (s->vm_id == -1) { s->vm_id = msg; } else { - process_msg_disconnect(s, msg); + process_msg_disconnect(s, msg, errp); } } static void ivshmem_read(void *opaque, const uint8_t *buf, int size) { IVShmemState *s = opaque; + Error *err = NULL; int incoming_fd; int64_t incoming_posn; @@ -673,10 +675,13 @@ static void ivshmem_read(void *opaque, const uint8_t *buf, int size) IVSHMEM_DPRINTF("posn is %" PRId64 ", fd is %d\n", incoming_posn, incoming_fd); - process_msg(s, incoming_posn, incoming_fd); + process_msg(s, incoming_posn, incoming_fd, &err); + if (err) { + error_report_err(err); + } } -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; @@ -686,7 +691,7 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd) ret = qemu_chr_fe_read_all(s->server_chr, (uint8_t *)&msg + n, sizeof(msg) - n); if (ret < 0 && ret != -EINTR) { - /* TODO error handling */ + error_setg_errno(errp, -ret, "read from server failed"); return INT64_MIN; } n += ret; @@ -696,15 +701,24 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd) return msg; } -static void ivshmem_recv_setup(IVShmemState *s) +static void ivshmem_recv_setup(IVShmemState *s, Error **errp) { + Error *err = NULL; int64_t msg; int fd; - msg = ivshmem_recv_msg(s, &fd); - if (fd != -1 || msg != IVSHMEM_PROTOCOL_VERSION) { - fprintf(stderr, "incompatible version, you are connecting to a ivshmem-" - "server using a different protocol please check your setup\n"); + msg = ivshmem_recv_msg(s, &fd, &err); + if (err) { + error_propagate(errp, err); + return; + } + if (msg != IVSHMEM_PROTOCOL_VERSION) { + error_setg(errp, "server sent version %" PRId64 ", expecting %d", + msg, IVSHMEM_PROTOCOL_VERSION); + return; + } + if (fd != -1) { + error_setg(errp, "server sent invalid version message"); return; } @@ -712,8 +726,16 @@ static void ivshmem_recv_setup(IVShmemState *s) * Receive more messages until we got shared memory. */ do { - msg = ivshmem_recv_msg(s, &fd); - process_msg(s, msg, fd); + msg = 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 != -1); assert(memory_region_is_mapped(&s->ivshmem)); @@ -768,7 +790,13 @@ static void ivshmem_enable_irqfd(IVShmemState *s) int i; for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) { - ivshmem_add_kvm_msi_virq(s, i); + Error *err = NULL; + + ivshmem_add_kvm_msi_virq(s, i, &err); + if (err) { + error_report_err(err); + /* TODO do we need to handle the error? */ + } } if (msix_set_vector_notifiers(pdev, @@ -814,7 +842,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address, pci_default_write_config(pdev, address, val, len); is_enabled = msix_enabled(pdev); - if (kvm_msi_via_irqfd_enabled() && s->vm_id != -1) { + if (kvm_msi_via_irqfd_enabled()) { if (!was_enabled && is_enabled) { ivshmem_enable_irqfd(s); } else if (was_enabled && !is_enabled) { @@ -933,15 +961,16 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **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; } + 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=@var{shm-size},vectors=@var{vectors},chard When using the server, the guest will be assigned a VM ID (>=0) that allows guests using the same server to communicate via interrupts. Guests can read their -VM ID from a device register (see example code). Since receiving the shared -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 application -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 semantics, -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). The @option{role} argument can be set to either master or peer and will affect how the shared memory is migrated. With @option{role=master}, the guest will
This kills off the funny state described in the previous commit. Simplify ivshmem_io_read() accordingly, and update documentation. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/specs/ivshmem-spec.txt | 20 ++++---- hw/misc/ivshmem.c | 121 +++++++++++++++++++++++++++----------------- qemu-doc.texi | 9 +--- 3 files changed, 87 insertions(+), 63 deletions(-)