[Qemu-devel] [PATCH 0/8] ivshmem: test msi=off, remove CharDriver

[Qemu-devel] [PATCH 0/8] ivshmem: test msi=off, remove CharDriver

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This is a ivshmem series with various bits:
- add a test for a recently introduced regression
- the fix is included in the series but was sent separatly to cc -stable
- fix some test leaks
- get rid of CharDriver usage for eventfd
- simplify event callback

Marc-André Lureau (8):
  ivshmem: no need for opaque argument
  ivshmem: remove redundant assignment, fix crash with msi=off
  ivshmem-test: leak fixes
  libqos: remove some leaks
  ivshmem-test: test both msi & irq cases
  ivshmem: generalize ivshmem_setup_interrupts
  ivshmem: use a single eventfd callback, get rid of CharDriver
  char: remove qemu_chr_open_eventfd

 hw/misc/ivshmem.c     | 85 +++++++++++++++++++++------------------------------
 include/sysemu/char.h |  3 --
 qemu-char.c           | 13 --------
 tests/ivshmem-test.c  | 81 ++++++++++++++++++++++++++++++++----------------
 tests/libqos/pci.c    |  2 ++
 5 files changed, 91 insertions(+), 93 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/8] ivshmem: no need for opaque argument

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/misc/ivshmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index f73f0c2..7d14222 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -350,11 +350,11 @@ static void ivshmem_vector_poll(PCIDevice *dev,
     }
 }
 
-static CharDriverState* create_eventfd_chr_device(void * opaque, EventNotifier *n,
+static CharDriverState* create_eventfd_chr_device(IVShmemState *s,
+                                                  EventNotifier *n,
                                                   int vector)
 {
     /* create a event character device based on the passed eventfd */
-    IVShmemState *s = opaque;
     PCIDevice *pdev = PCI_DEVICE(s);
     int eventfd = event_notifier_get_fd(n);
     CharDriverState *chr;
-- 
2.5.0

[Qemu-devel] [PATCH 2/8] ivshmem: remove redundant assignment, fix crash with msi=off

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Fix crash when msi=false introduced in 660c97ee (msi_vectors is NULL in
this case)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/misc/ivshmem.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 7d14222..dcfc8cc 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -355,12 +355,9 @@ static CharDriverState* create_eventfd_chr_device(IVShmemState *s,
                                                   int vector)
 {
     /* create a event character device based on the passed eventfd */
-    PCIDevice *pdev = PCI_DEVICE(s);
     int eventfd = event_notifier_get_fd(n);
     CharDriverState *chr;
 
-    s->msi_vectors[vector].pdev = pdev;
-
     chr = qemu_chr_open_eventfd(eventfd);
 
     if (chr == NULL) {
-- 
2.5.0

[Qemu-devel] [PATCH 3/8] ivshmem-test: leak fixes

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/ivshmem-test.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 03c7b96..d544d5e 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -34,12 +34,10 @@ static void save_fn(QPCIDevice *dev, int devfn, void *data)
     *pdev = dev;
 }
 
-static QPCIDevice *get_device(void)
+static QPCIDevice *get_device(QPCIBus *pcibus)
 {
     QPCIDevice *dev;
-    QPCIBus *pcibus;
 
-    pcibus = qpci_init_pc();
     dev = NULL;
     qpci_device_foreach(pcibus, 0x1af4, 0x1110, save_fn, &dev);
     g_assert(dev != NULL);
@@ -50,6 +48,7 @@ static QPCIDevice *get_device(void)
 typedef struct _IVState {
     QTestState *qtest;
     void *reg_base, *mem_base;
+    QPCIBus *pcibus;
     QPCIDevice *dev;
 } IVState;
 
@@ -100,13 +99,20 @@ static inline void out_reg(IVState *s, enum Reg reg, unsigned v)
     global_qtest = qtest;
 }
 
+static void cleanup_vm(IVState *s)
+{
+    g_free(s->dev);
+    qpci_free_pc(s->pcibus);
+    qtest_quit(s->qtest);
+}
+
 static void setup_vm_cmd(IVState *s, const char *cmd, bool msix)
 {
     uint64_t barsize;
 
     s->qtest = qtest_start(cmd);
-
-    s->dev = get_device();
+    s->pcibus = qpci_init_pc();
+    s->dev = get_device(s->pcibus);
 
     /* FIXME: other bar order fails, mappings changes */
     s->mem_base = qpci_iomap(s->dev, 2, &barsize);
@@ -173,7 +179,7 @@ static void test_ivshmem_single(void)
         g_assert_cmpuint(data[i], ==, i);
     }
 
-    qtest_quit(s->qtest);
+    cleanup_vm(s);
 }
 
 static void test_ivshmem_pair(void)
@@ -218,8 +224,8 @@ static void test_ivshmem_pair(void)
         g_assert_cmpuint(data[i], ==, 0x44);
     }
 
-    qtest_quit(s1->qtest);
-    qtest_quit(s2->qtest);
+    cleanup_vm(s1);
+    cleanup_vm(s2);
     g_free(data);
 }
 
@@ -356,8 +362,8 @@ static void test_ivshmem_server(void)
     } while (ret == 0 && g_get_monotonic_time() < end_time);
     g_assert_cmpuint(ret, !=, 0);
 
-    qtest_quit(s2->qtest);
-    qtest_quit(s1->qtest);
+    cleanup_vm(s2);
+    cleanup_vm(s1);
 
     if (qemu_write_full(thread.pipe[1], "q", 1) != 1) {
         g_error("qemu_write_full: %s", g_strerror(errno));
@@ -395,7 +401,7 @@ static void test_ivshmem_memdev(void)
     setup_vm_cmd(&state, "-object memory-backend-ram,size=1M,id=mb1"
                  " -device ivshmem,x-memdev=mb1", false);
 
-    qtest_quit(state.qtest);
+    cleanup_vm(&state);
 }
 
 static void cleanup(void)
-- 
2.5.0

[Qemu-devel] [PATCH 4/8] libqos: remove some leaks

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

qpci_device_find() returns allocated data, don't leak it.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/libqos/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 4e630c2..80b1a21 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -34,11 +34,13 @@ void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
 
             if (vendor_id != -1 &&
                 qpci_config_readw(dev, PCI_VENDOR_ID) != vendor_id) {
+                g_free(dev);
                 continue;
             }
 
             if (device_id != -1 &&
                 qpci_config_readw(dev, PCI_DEVICE_ID) != device_id) {
+                g_free(dev);
                 continue;
             }
 
-- 
2.5.0

[Qemu-devel] [PATCH 5/8] ivshmem-test: test both msi & irq cases

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Recent commit 660c97ee introduced a regression in irq case, make
sure this code path is also tested.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/ivshmem-test.c | 53 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index d544d5e..705fece 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -277,18 +277,18 @@ static void *server_thread(void *data)
     return NULL;
 }
 
-static void setup_vm_with_server(IVState *s, int nvectors)
+static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
 {
     char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
-                                "-device ivshmem,size=1M,chardev=chr0,vectors=%d",
-                                tmpserver, nvectors);
+                                "-device ivshmem,size=1M,chardev=chr0,vectors=%d,msi=%s",
+                                tmpserver, nvectors, msi ? "true" : "false");
 
-    setup_vm_cmd(s, cmd, true);
+    setup_vm_cmd(s, cmd, msi);
 
     g_free(cmd);
 }
 
-static void test_ivshmem_server(void)
+static void test_ivshmem_server(bool msi)
 {
     IVState state1, state2, *s1, *s2;
     ServerThread thread;
@@ -306,9 +306,9 @@ static void test_ivshmem_server(void)
     ret = ivshmem_server_start(&server);
     g_assert_cmpint(ret, ==, 0);
 
-    setup_vm_with_server(&state1, nvectors);
+    setup_vm_with_server(&state1, nvectors, msi);
     s1 = &state1;
-    setup_vm_with_server(&state2, nvectors);
+    setup_vm_with_server(&state2, nvectors, msi);
     s2 = &state2;
 
     g_assert_cmpuint(in_reg(s1, IVPOSITION), ==, 0xffffffff);
@@ -338,27 +338,37 @@ static void test_ivshmem_server(void)
     g_assert_cmpuint(vm1, !=, vm2);
 
     global_qtest = s1->qtest;
-    ret = qpci_msix_table_size(s1->dev);
-    g_assert_cmpuint(ret, ==, nvectors);
+    if (msi) {
+        ret = qpci_msix_table_size(s1->dev);
+        g_assert_cmpuint(ret, ==, nvectors);
+    }
 
     /* ping vm2 -> vm1 */
-    ret = qpci_msix_pending(s1->dev, 0);
-    g_assert_cmpuint(ret, ==, 0);
+    if (msi) {
+        ret = qpci_msix_pending(s1->dev, 0);
+        g_assert_cmpuint(ret, ==, 0);
+    } else {
+        out_reg(s1, INTRSTATUS, 0);
+    }
     out_reg(s2, DOORBELL, vm1 << 16);
     do {
         g_usleep(10000);
-        ret = qpci_msix_pending(s1->dev, 0);
+        ret = msi ? qpci_msix_pending(s1->dev, 0) : in_reg(s1, INTRSTATUS);
     } while (ret == 0 && g_get_monotonic_time() < end_time);
     g_assert_cmpuint(ret, !=, 0);
 
     /* ping vm1 -> vm2 */
     global_qtest = s2->qtest;
-    ret = qpci_msix_pending(s2->dev, 0);
-    g_assert_cmpuint(ret, ==, 0);
+    if (msi) {
+        ret = qpci_msix_pending(s2->dev, 0);
+        g_assert_cmpuint(ret, ==, 0);
+    } else {
+        out_reg(s2, INTRSTATUS, 0);
+    }
     out_reg(s1, DOORBELL, vm2 << 16);
     do {
         g_usleep(10000);
-        ret = qpci_msix_pending(s2->dev, 0);
+        ret = msi ? qpci_msix_pending(s2->dev, 0) : in_reg(s2, INTRSTATUS);
     } while (ret == 0 && g_get_monotonic_time() < end_time);
     g_assert_cmpuint(ret, !=, 0);
 
@@ -376,6 +386,16 @@ static void test_ivshmem_server(void)
     close(thread.pipe[0]);
 }
 
+static void test_ivshmem_server_msi(void)
+{
+    test_ivshmem_server(true);
+}
+
+static void test_ivshmem_server_irq(void)
+{
+    test_ivshmem_server(false);
+}
+
 #define PCI_SLOT_HP             0x06
 
 static void test_ivshmem_hotplug(void)
@@ -489,7 +509,8 @@ int main(int argc, char **argv)
     qtest_add_func("/ivshmem/memdev", test_ivshmem_memdev);
     if (g_test_slow()) {
         qtest_add_func("/ivshmem/pair", test_ivshmem_pair);
-        qtest_add_func("/ivshmem/server", test_ivshmem_server);
+        qtest_add_func("/ivshmem/server-msi", test_ivshmem_server_msi);
+        qtest_add_func("/ivshmem/server-irq", test_ivshmem_server_irq);
     }
 
     ret = g_test_run();
-- 
2.5.0

[Qemu-devel] [PATCH 6/8] ivshmem: generalize ivshmem_setup_interrupts

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Call ivshmem_setup_interrupts() with or without MSI, always allocate
msi_vectors that is going to be used in all case in the following patch.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/misc/ivshmem.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index dcfc8cc..11780b1 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -768,19 +768,28 @@ static void ivshmem_reset(DeviceState *d)
     ivshmem_use_msix(s);
 }
 
-static int ivshmem_setup_msi(IVShmemState * s)
+static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
 {
-    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
-        return -1;
+    /* allocate QEMU callback data for receiving interrupts */
+    s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
+    if (!s->msi_vectors) {
+        goto fail;
     }
 
-    IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
+    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
+        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
+            goto fail;
+        }
 
-    /* allocate QEMU char devices for receiving interrupts */
-    s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
+        IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
+        ivshmem_use_msix(s);
+    }
 
-    ivshmem_use_msix(s);
     return 0;
+
+fail:
+    error_setg(errp, "failed to initialize interrupts");
+    return -1;
 }
 
 static void ivshmem_enable_irqfd(IVShmemState *s)
@@ -946,9 +955,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
         IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n",
                         s->server_chr->filename);
 
-        if (ivshmem_has_feature(s, IVSHMEM_MSI) &&
-            ivshmem_setup_msi(s)) {
-            error_setg(errp, "msix initialization failed");
+        if (ivshmem_setup_interrupts(s, errp) < 0) {
             return;
         }
 
-- 
2.5.0

[Qemu-devel] [PATCH 7/8] ivshmem: use a single eventfd callback, get rid of CharDriver

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

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é Lureau <marcandre.lureau@redhat.com>
---
 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 = {
     },
 };
 
-static void ivshmem_receive(void *opaque, const uint8_t *buf, int size)
-{
-    IVShmemState *s = opaque;
-
-    IVSHMEM_DPRINTF("ivshmem_receive 0x%02x size: %d\n", *buf, size);
-
-    ivshmem_IntrStatus_write(s, *buf);
-}
-
 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 = opaque;
     PCIDevice *pdev = entry->pdev;
     IVShmemState *s = IVSHMEM(pdev);
     int vector = entry - s->msi_vectors;
+    EventNotifier *n = &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);
+    }
 }
 
 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 = event_notifier_get_fd(n);
-    CharDriverState *chr;
-
-    chr = qemu_chr_open_eventfd(eventfd);
-
-    if (chr == NULL) {
-        error_report("creating chardriver for eventfd %d failed", eventfd);
-        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 = 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 = 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 vector)
 
     if (!with_irqfd) {
         IVSHMEM_DPRINTF("with eventfd");
-        s->eventfd_chr[vector] = create_eventfd_chr_device(s, n, vector);
+        watch_vector_notifier(s, n, vector);
     } else if (msix_enabled(pdev)) {
         IVSHMEM_DPRINTF("with irqfd");
         if (ivshmem_add_kvm_msi_virq(s, vector) < 0) {
-- 
2.5.0

[Qemu-devel] [PATCH 8/8] char: remove qemu_chr_open_eventfd

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

No longer needed by ivshmem.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/char.h |  3 ---
 qemu-char.c           | 13 -------------
 2 files changed, 16 deletions(-)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index aff193f..d355116 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -357,9 +357,6 @@ void register_char_driver(const char *name, ChardevBackendKind kind,
         CharDriverState *(*create)(const char *id, ChardevBackend *backend,
                                    ChardevReturn *ret, Error **errp));
 
-/* add an eventfd to the qemu devices that are polled */
-CharDriverState *qemu_chr_open_eventfd(int eventfd);
-
 extern int term_escape_char;
 
 CharDriverState *qemu_char_get_next_serial(void);
diff --git a/qemu-char.c b/qemu-char.c
index 00a7526..8be1136 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2924,19 +2924,6 @@ static int tcp_chr_sync_read(CharDriverState *chr, const uint8_t *buf, int len)
     return size;
 }
 
-#ifndef _WIN32
-CharDriverState *qemu_chr_open_eventfd(int eventfd)
-{
-    CharDriverState *chr = qemu_chr_open_fd(eventfd, eventfd);
-
-    if (chr) {
-        chr->avail_connections = 1;
-    }
-
-    return chr;
-}
-#endif
-
 static void tcp_chr_connect(void *opaque)
 {
     CharDriverState *chr = opaque;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 0/8] ivshmem: test msi=off, remove CharDriver

[Marc-André Lureau]

Hi

On Mon, Dec 21, 2015 at 12:30 PM,  <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This is a ivshmem series with various bits:
> - add a test for a recently introduced regression
> - the fix is included in the series but was sent separatly to cc -stable
> - fix some test leaks
> - get rid of CharDriver usage for eventfd
> - simplify event callback
>

Adding a few people in CC who might help with reviewing.

thanks

> Marc-André Lureau (8):
>   ivshmem: no need for opaque argument
>   ivshmem: remove redundant assignment, fix crash with msi=off
>   ivshmem-test: leak fixes
>   libqos: remove some leaks
>   ivshmem-test: test both msi & irq cases
>   ivshmem: generalize ivshmem_setup_interrupts
>   ivshmem: use a single eventfd callback, get rid of CharDriver
>   char: remove qemu_chr_open_eventfd
>
>  hw/misc/ivshmem.c     | 85 +++++++++++++++++++++------------------------------
>  include/sysemu/char.h |  3 --
>  qemu-char.c           | 13 --------
>  tests/ivshmem-test.c  | 81 ++++++++++++++++++++++++++++++++----------------
>  tests/libqos/pci.c    |  2 ++
>  5 files changed, 91 insertions(+), 93 deletions(-)
>
> --
> 2.5.0
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 0/8] ivshmem: test msi=off, remove CharDriver

[Marc-André Lureau]

ping

On Thu, Jan 7, 2016 at 4:52 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Mon, Dec 21, 2015 at 12:30 PM,  <marcandre.lureau@redhat.com> wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> This is a ivshmem series with various bits:
>> - add a test for a recently introduced regression
>> - the fix is included in the series but was sent separatly to cc -stable
>> - fix some test leaks
>> - get rid of CharDriver usage for eventfd
>> - simplify event callback
>>
>
> Adding a few people in CC who might help with reviewing.
>
> thanks
>
>> Marc-André Lureau (8):
>>   ivshmem: no need for opaque argument
>>   ivshmem: remove redundant assignment, fix crash with msi=off
>>   ivshmem-test: leak fixes
>>   libqos: remove some leaks
>>   ivshmem-test: test both msi & irq cases
>>   ivshmem: generalize ivshmem_setup_interrupts
>>   ivshmem: use a single eventfd callback, get rid of CharDriver
>>   char: remove qemu_chr_open_eventfd
>>
>>  hw/misc/ivshmem.c     | 85 +++++++++++++++++++++------------------------------
>>  include/sysemu/char.h |  3 --
>>  qemu-char.c           | 13 --------
>>  tests/ivshmem-test.c  | 81 ++++++++++++++++++++++++++++++++----------------
>>  tests/libqos/pci.c    |  2 ++
>>  5 files changed, 91 insertions(+), 93 deletions(-)
>>
>> --
>> 2.5.0
>>
>>
>
>
>
> --
> Marc-André Lureau



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 0/8] ivshmem: test msi=off, remove CharDriver

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Dec 21, 2015 at 12:30 PM,  <marcandre.lureau@redhat.com> wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> This is a ivshmem series with various bits:
>> - add a test for a recently introduced regression
>> - the fix is included in the series but was sent separatly to cc -stable
>> - fix some test leaks
>> - get rid of CharDriver usage for eventfd
>> - simplify event callback
>>
>
> Adding a few people in CC who might help with reviewing.

The last patch doesn't apply anymore.  I'll look over the series anyway.

Re: [Qemu-devel] [PATCH 4/8] libqos: remove some leaks

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> qpci_device_find() returns allocated data, don't leak it.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/libqos/pci.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 4e630c2..80b1a21 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -34,11 +34,13 @@ void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
       for (slot = 0; slot < 32; slot++) {
           int fn;

           for (fn = 0; fn < 8; fn++) {
               QPCIDevice *dev;

               dev = qpci_device_find(bus, QPCI_DEVFN(slot, fn));
               if (!dev) {
                   continue;
               }
>  
>              if (vendor_id != -1 &&
>                  qpci_config_readw(dev, PCI_VENDOR_ID) != vendor_id) {
> +                g_free(dev);
>                  continue;
>              }
>  
>              if (device_id != -1 &&
>                  qpci_config_readw(dev, PCI_DEVICE_ID) != device_id) {
> +                g_free(dev);
>                  continue;
>              }

               func(dev, QPCI_DEVFN(slot, fn), data);
           }
       }
   }

The existing users pass a func that saves dev, and free the saved dev
later.  Works as long as we call func() at most once.  If multiple
devices match, all but the last one are leaked.  Can this happen?

Re: [Qemu-devel] [PATCH 6/8] ivshmem: generalize ivshmem_setup_interrupts

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Call ivshmem_setup_interrupts() with or without MSI, always allocate
> msi_vectors that is going to be used in all case in the following patch.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/misc/ivshmem.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index dcfc8cc..11780b1 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -768,19 +768,28 @@ static void ivshmem_reset(DeviceState *d)
>      ivshmem_use_msix(s);
>  }
>  
> -static int ivshmem_setup_msi(IVShmemState * s)
> +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>  {
> -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> -        return -1;
> +    /* allocate QEMU callback data for receiving interrupts */
> +    s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
> +    if (!s->msi_vectors) {

Happens exactly when s->vectors is zero.  Is that a legitimate
configuration?

> +        goto fail;
>      }
>  
> -    IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> +        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> +            goto fail;
> +        }
>  
> -    /* allocate QEMU char devices for receiving interrupts */
> -    s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
> +        IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
> +        ivshmem_use_msix(s);
> +    }
>  
> -    ivshmem_use_msix(s);
>      return 0;
> +
> +fail:
> +    error_setg(errp, "failed to initialize interrupts");
> +    return -1;
>  }

Recommend not to move the error_setg().  Keeps this function simpler, at
no cost.

>  
>  static void ivshmem_enable_irqfd(IVShmemState *s)
> @@ -946,9 +955,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
>          IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n",
>                          s->server_chr->filename);
>  
> -        if (ivshmem_has_feature(s, IVSHMEM_MSI) &&
> -            ivshmem_setup_msi(s)) {
> -            error_setg(errp, "msix initialization failed");
> +        if (ivshmem_setup_interrupts(s, errp) < 0) {
>              return;
>          }

Yup, the only change is we now allocate s->msi_vectors whether we have
IVSHMEM_MSI or not.

Re: [Qemu-devel] [PATCH 7/8] ivshmem: use a single eventfd callback, get rid of CharDriver

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 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é Lureau <marcandre.lureau@redhat.com>
> ---
>  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 = {
>      },
>  };
>  
> -static void ivshmem_receive(void *opaque, const uint8_t *buf, int size)
> -{
> -    IVShmemState *s = 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."

> -}
> -
>  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 = opaque;
>      PCIDevice *pdev = entry->pdev;
>      IVShmemState *s = IVSHMEM(pdev);
>      int vector = entry - s->msi_vectors;
> +    EventNotifier *n = &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.

> +    }
>  }
>  
>  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 = event_notifier_get_fd(n);
> -    CharDriverState *chr;
> -
> -    chr = qemu_chr_open_eventfd(eventfd);
> -
> -    if (chr == NULL) {
> -        error_report("creating chardriver for eventfd %d failed", eventfd);
> -        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 = 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 = 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 vector)
>  
>      if (!with_irqfd) {
>          IVSHMEM_DPRINTF("with eventfd");
> -        s->eventfd_chr[vector] = create_eventfd_chr_device(s, n, vector);
> +        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?

Re: [Qemu-devel] [PATCH 4/8] libqos: remove some leaks

[Marc-André Lureau]

Hi

On Fri, Jan 29, 2016 at 4:43 PM, Markus Armbruster <armbru@redhat.com> wrote:
> The existing users pass a func that saves dev, and free the saved dev
> later.  Works as long as we call func() at most once.  If multiple
> devices match, all but the last one are leaked.  Can this happen?

It is the responsability of the func() callback to deal with multiple
matches. I don't think this needs to change.

This fix is only about the case of unmatching devices that need to be
free within qpci_device_foreach().

Do you ack that fix?


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 6/8] ivshmem: generalize ivshmem_setup_interrupts

[Marc-André Lureau]

Hi

On Fri, Jan 29, 2016 at 4:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Call ivshmem_setup_interrupts() with or without MSI, always allocate
>> msi_vectors that is going to be used in all case in the following patch.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  hw/misc/ivshmem.c | 27 +++++++++++++++++----------
>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index dcfc8cc..11780b1 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -768,19 +768,28 @@ static void ivshmem_reset(DeviceState *d)
>>      ivshmem_use_msix(s);
>>  }
>>
>> -static int ivshmem_setup_msi(IVShmemState * s)
>> +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>  {
>> -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
>> -        return -1;
>> +    /* allocate QEMU callback data for receiving interrupts */
>> +    s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
>> +    if (!s->msi_vectors) {
>
> Happens exactly when s->vectors is zero.  Is that a legitimate
> configuration?

msix_init_exclusive_bar() already failed with nvectors == 0. However
it is acceptable for !msi to have nvectors == 0. Fixed.

>> +        goto fail;
>>      }
>>
>> -    IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
>> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>> +        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
>> +            goto fail;
>> +        }
>>
>> -    /* allocate QEMU char devices for receiving interrupts */
>> -    s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
>> +        IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
>> +        ivshmem_use_msix(s);
>> +    }
>>
>> -    ivshmem_use_msix(s);
>>      return 0;
>> +
>> +fail:
>> +    error_setg(errp, "failed to initialize interrupts");
>> +    return -1;
>>  }
>
> Recommend not to move the error_setg().  Keeps this function simpler, at
> no cost.

ok

>
>>
>>  static void ivshmem_enable_irqfd(IVShmemState *s)
>> @@ -946,9 +955,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
>>          IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n",
>>                          s->server_chr->filename);
>>
>> -        if (ivshmem_has_feature(s, IVSHMEM_MSI) &&
>> -            ivshmem_setup_msi(s)) {
>> -            error_setg(errp, "msix initialization failed");
>> +        if (ivshmem_setup_interrupts(s, errp) < 0) {
>>              return;
>>          }
>
> Yup, the only change is we now allocate s->msi_vectors whether we have
> IVSHMEM_MSI or not.
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 7/8] ivshmem: use a single eventfd callback, get rid of CharDriver

[Marc-André Lureau]

Hi

On Fri, Jan 29, 2016 at 5:23 PM, Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> 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é Lureau <marcandre.lureau@redhat.com>
>> ---
>>  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 = {
>>      },
>>  };
>>
>> -static void ivshmem_receive(void *opaque, const uint8_t *buf, int size)
>> -{
>> -    IVShmemState *s = 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 = opaque;
>>      PCIDevice *pdev = entry->pdev;
>>      IVShmemState *s = IVSHMEM(pdev);
>>      int vector = entry - s->msi_vectors;
>> +    EventNotifier *n = &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 = event_notifier_get_fd(n);
>> -    CharDriverState *chr;
>> -
>> -    chr = qemu_chr_open_eventfd(eventfd);
>> -
>> -    if (chr == NULL) {
>> -        error_report("creating chardriver for eventfd %d failed", eventfd);
>> -        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 = 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 = 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 vector)
>>
>>      if (!with_irqfd) {
>>          IVSHMEM_DPRINTF("with eventfd");
>> -        s->eventfd_chr[vector] = create_eventfd_chr_device(s, n, vector);
>> +        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.

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 4/8] libqos: remove some leaks

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Fri, Jan 29, 2016 at 4:43 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> The existing users pass a func that saves dev, and free the saved dev
>> later.  Works as long as we call func() at most once.  If multiple
>> devices match, all but the last one are leaked.  Can this happen?
>
> It is the responsability of the func() callback to deal with multiple
> matches.

If that's the case, and multiple devices can match, then the callbacks
are all broken.

>          I don't think this needs to change.

If you mean to say that you don't have to fix it in this series: yes,
but it still needs fixing.  If you don't want to fix it, consider adding
a FIXME comment.

> This fix is only about the case of unmatching devices that need to be
> free within qpci_device_foreach().
>
> Do you ack that fix?

I'd be willing to.

Re: [Qemu-devel] [PATCH 7/8] ivshmem: use a single eventfd callback, get rid of CharDriver

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Fri, Jan 29, 2016 at 5:23 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> marcandre.lureau@redhat.com writes:
>>
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> 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é Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  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 = {
>>>      },
>>>  };
>>>
>>> -static void ivshmem_receive(void *opaque, const uint8_t *buf, int size)
>>> -{
>>> -    IVShmemState *s = 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 = opaque;
>>>      PCIDevice *pdev = entry->pdev;
>>>      IVShmemState *s = IVSHMEM(pdev);
>>>      int vector = entry - s->msi_vectors;
>>> +    EventNotifier *n = &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 = event_notifier_get_fd(n);
>>> -    CharDriverState *chr;
>>> -
>>> -    chr = qemu_chr_open_eventfd(eventfd);
>>> -
>>> -    if (chr == NULL) {
>>> -        error_report("creating chardriver for eventfd %d failed", eventfd);
>>> -        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 = 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 = 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 vector)
>>>
>>>      if (!with_irqfd) {
>>>          IVSHMEM_DPRINTF("with eventfd");
>>> -        s->eventfd_chr[vector] = create_eventfd_chr_device(s, n, vector);
>>> +        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.

Re: [Qemu-devel] [PATCH 6/8] ivshmem: generalize ivshmem_setup_interrupts

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Fri, Jan 29, 2016 at 4:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> marcandre.lureau@redhat.com writes:
>>
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Call ivshmem_setup_interrupts() with or without MSI, always allocate
>>> msi_vectors that is going to be used in all case in the following patch.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  hw/misc/ivshmem.c | 27 +++++++++++++++++----------
>>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>> index dcfc8cc..11780b1 100644
>>> --- a/hw/misc/ivshmem.c
>>> +++ b/hw/misc/ivshmem.c
>>> @@ -768,19 +768,28 @@ static void ivshmem_reset(DeviceState *d)
>>>      ivshmem_use_msix(s);
>>>  }
>>>
>>> -static int ivshmem_setup_msi(IVShmemState * s)
>>> +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>>  {
>>> -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
>>> -        return -1;
>>> +    /* allocate QEMU callback data for receiving interrupts */
>>> +    s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
>>> +    if (!s->msi_vectors) {
>>
>> Happens exactly when s->vectors is zero.  Is that a legitimate
>> configuration?
>
> msix_init_exclusive_bar() already failed with nvectors == 0. However
> it is acceptable for !msi to have nvectors == 0. Fixed.

Sounds like I can expect a respin, correct me if I'm wrong.

[...]

Re: [Qemu-devel] [PATCH 4/8] libqos: remove some leaks

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
>> Hi
>>
>> On Fri, Jan 29, 2016 at 4:43 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> The existing users pass a func that saves dev, and free the saved dev
>>> later.  Works as long as we call func() at most once.  If multiple
>>> devices match, all but the last one are leaked.  Can this happen?
>>
>> It is the responsability of the func() callback to deal with multiple
>> matches.
>
> If that's the case, and multiple devices can match, then the callbacks
> are all broken.
>
>>          I don't think this needs to change.
>
> If you mean to say that you don't have to fix it in this series: yes,
> but it still needs fixing.  If you don't want to fix it, consider adding
> a FIXME comment.

I'll have some ivshmem work coming up, and will try to remember to
include a fix for this.

[...]

Re: [Qemu-devel] [PATCH 7/8] ivshmem: use a single eventfd callback, get rid of CharDriver

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
>> Hi
>>
>> On Fri, Jan 29, 2016 at 5:23 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> marcandre.lureau@redhat.com writes:
>>>
>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>> 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é Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>>  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 = {
>>>>      },
>>>>  };
>>>>
>>>> -static void ivshmem_receive(void *opaque, const uint8_t *buf, int size)
>>>> -{
>>>> -    IVShmemState *s = 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.

I'm afraid I didn't read the code correctly.  Yes, the peer writes 1
unless someone got creative.  And yes, if we're using the pipe
emulation, we actually read what the peer wrote.  But if we're using
eventfd(2), we read the sum of what the peer wrote since our last read.
If that sum is zero modulo 256, the interrupt is lost.  Fortunately, we
should be using KVM ioeventfd in practice, which should bypass the
flawed code.

So this is actually a bug fix, but it's much too late to update the
commit message.

I told you I want review from someone who actually understands this
stuff :)