From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41801) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQHfT-0001w0-42 for qemu-devel@nongnu.org; Mon, 01 Feb 2016 11:49:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQHfP-0002mM-PZ for qemu-devel@nongnu.org; Mon, 01 Feb 2016 11:49:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36797) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQHfP-0002mI-Hk for qemu-devel@nongnu.org; Mon, 01 Feb 2016 11:49:31 -0500 From: Markus Armbruster 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, 01 Feb 2016 17:49:28 +0100 In-Reply-To: (=?utf-8?Q?=22Marc-Andr=C3=A9?= Lureau"'s message of "Mon, 1 Feb 2016 16:22:41 +0100") Message-ID: <87si1cz887.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 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: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: Paolo Bonzini , QEMU Marc-Andr=C3=A9 Lureau writes: > Hi > > On Fri, Jan 29, 2016 at 5:23 PM, Markus Armbruster wr= ote: >> 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. For me, working code trumps spec unless the code is clearly flawed. Other software doesn't interface with the spec, it interfaces with the code. However, I guess we can follow the spec in this case. Two reasons: * We can't permit arbitrary values, because value 0 could break things (I think). * If I read the code correctly, the value we read here should come from a peer's ivshmem device model. The device model writes it with event_notifier_set(), which writes 1. To get any other value, you need to get creative. So the code agrees with the spec, unless you get creative. >>> -} >>> - >>> 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", even= tfd); >>> - 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_receiv= e, >>> - 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 ve= ctor) >>> >>> if (!with_irqfd) { >>> IVSHMEM_DPRINTF("with eventfd"); >>> - s->eventfd_chr[vector] =3D create_eventfd_chr_device(s, n, vec= tor); >>> + 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. Yes, but I'm not familiar enough with this stuff to do a real review with reasonable effot. The hamfisted way to "encourage" real review is to withhold my R-by for this patch. Isn't 100% right, because I *did* look over it (and liked what I saw), but it's less wrong than merging this without real review.