Marc-André Lureau <marcandre.lur...@gmail.com> writes: > On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <arm...@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 <arm...@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! [...]