From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48307) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQGJP-0002dE-Gs for qemu-devel@nongnu.org; Mon, 01 Feb 2016 10:22:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQGJO-0000Dk-6J for qemu-devel@nongnu.org; Mon, 01 Feb 2016 10:22:43 -0500 Received: from mail-oi0-x236.google.com ([2607:f8b0:4003:c06::236]:33074) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQGJN-0000Dg-Ux for qemu-devel@nongnu.org; Mon, 01 Feb 2016 10:22:42 -0500 Received: by mail-oi0-x236.google.com with SMTP id r14so91385509oie.0 for ; Mon, 01 Feb 2016 07:22:41 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <87zivo5p8i.fsf@blackfin.pond.sub.org> References: <1450697444-30119-1-git-send-email-marcandre.lureau@redhat.com> <1450697444-30119-8-git-send-email-marcandre.lureau@redhat.com> <87zivo5p8i.fsf@blackfin.pond.sub.org> Date: Mon, 1 Feb 2016 16:22:41 +0100 Message-ID: From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 7/8] ivshmem: use a single eventfd callback, get rid of CharDriver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Paolo Bonzini , QEMU Hi On Fri, Jan 29, 2016 at 5:23 PM, Markus Armbruster wrot= e: > marcandre.lureau@redhat.com writes: > >> From: Marc-Andr=C3=A9 Lureau >> >> Simplify the interrupt handling by having a single callback on irq&msi >> cases. Remove usage of CharDriver, replace it with >> qemu_set_fd_handler(). Use event_notifier_test_and_clear() to read the >> eventfd. >> >> Signed-off-by: Marc-Andr=C3=A9 Lureau >> --- >> hw/misc/ivshmem.c | 55 ++++++++++++++++++------------------------------= ------- >> 1 file changed, 18 insertions(+), 37 deletions(-) >> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c >> index 11780b1..9eb8a81 100644 >> --- a/hw/misc/ivshmem.c >> +++ b/hw/misc/ivshmem.c >> @@ -263,15 +263,6 @@ static const MemoryRegionOps ivshmem_mmio_ops =3D { >> }, >> }; >> >> -static void ivshmem_receive(void *opaque, const uint8_t *buf, int size) >> -{ >> - IVShmemState *s =3D opaque; >> - >> - IVSHMEM_DPRINTF("ivshmem_receive 0x%02x size: %d\n", *buf, size); >> - >> - ivshmem_IntrStatus_write(s, *buf); > > Before your patch, we write the first byte received to s->intrstatus. > This is odd; ivshmem_device_spec.txt says "The status register is set to > 1 when an interrupt occurs." I didn't noticed that (it has been like this from initial commit), I think we should follow the spec. >> -} >> - >> static int ivshmem_can_receive(void * opaque) >> { >> return sizeof(int64_t); >> @@ -282,15 +273,24 @@ static void ivshmem_event(void *opaque, int event) >> IVSHMEM_DPRINTF("ivshmem_event %d\n", event); >> } >> >> -static void fake_irqfd(void *opaque, const uint8_t *buf, int size) { >> - >> +static void ivshmem_vector_notify(void *opaque) >> +{ >> MSIVector *entry =3D opaque; >> PCIDevice *pdev =3D entry->pdev; >> IVShmemState *s =3D IVSHMEM(pdev); >> int vector =3D entry - s->msi_vectors; >> + EventNotifier *n =3D &s->peers[s->vm_id].eventfds[vector]; >> + >> + if (!event_notifier_test_and_clear(n)) { >> + return; >> + } >> >> IVSHMEM_DPRINTF("interrupt on vector %p %d\n", pdev, vector); >> - msix_notify(pdev, vector); >> + if (ivshmem_has_feature(s, IVSHMEM_MSI)) { >> + msix_notify(pdev, vector); >> + } else { >> + ivshmem_IntrStatus_write(s, 1); > > After the patch, we write 1 to s->intrstatus. May well be an > improvement, or even a bug fix, but it needs to be explained in the > commit message. Ok > >> + } >> } >> >> static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector, >> @@ -350,35 +350,16 @@ static void ivshmem_vector_poll(PCIDevice *dev, >> } >> } >> >> -static CharDriverState* create_eventfd_chr_device(IVShmemState *s, >> - EventNotifier *n, >> - int vector) >> +static void watch_vector_notifier(IVShmemState *s, EventNotifier *n, >> + int vector) >> { >> - /* create a event character device based on the passed eventfd */ >> int eventfd =3D event_notifier_get_fd(n); >> - CharDriverState *chr; >> - >> - chr =3D qemu_chr_open_eventfd(eventfd); >> - >> - if (chr =3D=3D NULL) { >> - error_report("creating chardriver for eventfd %d failed", event= fd); >> - return NULL; >> - } >> - qemu_chr_fe_claim_no_fail(chr); >> >> /* if MSI is supported we need multiple interrupts */ >> - if (ivshmem_has_feature(s, IVSHMEM_MSI)) { >> - s->msi_vectors[vector].pdev =3D PCI_DEVICE(s); >> - >> - qemu_chr_add_handlers(chr, ivshmem_can_receive, fake_irqfd, >> - ivshmem_event, &s->msi_vectors[vector]); >> - } else { >> - qemu_chr_add_handlers(chr, ivshmem_can_receive, ivshmem_receive= , >> - ivshmem_event, s); >> - } >> - >> - return chr; >> + s->msi_vectors[vector].pdev =3D PCI_DEVICE(s); >> >> + qemu_set_fd_handler(eventfd, ivshmem_vector_notify, >> + NULL, &s->msi_vectors[vector]); >> } >> >> static int check_shm_size(IVShmemState *s, int fd, Error **errp) >> @@ -587,7 +568,7 @@ static void setup_interrupt(IVShmemState *s, int vec= tor) >> >> if (!with_irqfd) { >> IVSHMEM_DPRINTF("with eventfd"); >> - s->eventfd_chr[vector] =3D create_eventfd_chr_device(s, n, vect= or); >> + watch_vector_notifier(s, n, vector); >> } else if (msix_enabled(pdev)) { >> IVSHMEM_DPRINTF("with irqfd"); >> if (ivshmem_add_kvm_msi_virq(s, vector) < 0) { > > I like the looks of it, not least because it enables removal of > qemu_chr_open_eventfd() in the next patch. But I recommend to get an > R-by from someone who actually understands this chardev stuff. Paolo, > perhaps? It's really not much, qemu_chr_open_eventfd() was introduced for ivshmem to watch eventfd with qemu_chr_add_handlers(), but we can simply use EventNotifier + qemu_set_fd_handler() alone. --=20 Marc-Andr=C3=A9 Lureau