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

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

[marcandre.lureau]

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

This is a ivshmem series with various bits:
- add a test for msi=off regression (the fix is included in the series
  but was sent separatly to cc -stable)
- get rid of CharDriver usage for eventfd & fix regression introduced
  by d0d7708ba by Daniel Berrange
- fix some test leaks
- simplify event callback

v1->v2: after Markus Armbruster review,
- add a few comments to commit messages
- fix !msi with nvectors==0 regression
- comment in commit message that the status register is set to 1 when
  an interrupt occurs, following the ivshmem specification
- found out that getting rid of CharDriver also fixed regression from
  d0d7708ba
- rebase

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     | 84 +++++++++++++++++++++------------------------------
 include/sysemu/char.h |  3 --
 qemu-char.c           | 13 --------
 tests/ivshmem-test.c  | 81 ++++++++++++++++++++++++++++++++-----------------
 tests/libqos/pci.c    |  2 ++
 5 files changed, 90 insertions(+), 93 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v2 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 d5c89ae..358df24 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 v2 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>
Reviewed-by: Markus Armbruster <armbru@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 358df24..5029789 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 v2 3/8] ivshmem-test: leak fixes

[marcandre.lureau]

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

Add a cleanup_vm() function to free QPCIDevice & QPCIBus when cleaning
up the IVState.

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 v2 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 v2 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 v2 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 | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 5029789..c601a8a 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -769,18 +769,25 @@ static void ivshmem_reset(DeviceState *d)
     ivshmem_use_msix(s);
 }
 
-static int ivshmem_setup_msi(IVShmemState * s)
+static int ivshmem_setup_interrupts(IVShmemState *s)
 {
-    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
-        return -1;
+    /* allocate QEMU callback data for receiving interrupts */
+    if (s->vectors > 0) {
+        s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
+        if (!s->msi_vectors) {
+            return -1;
+        }
     }
 
-    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)) {
+            return -1;
+        }
 
-    /* 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;
 }
 
@@ -947,9 +954,8 @@ 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) < 0) {
+            error_setg(errp, "failed to initialize interrupts");
             return;
         }
 
-- 
2.5.0

[Qemu-devel] [PATCH v2 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.

Before this patch, ivshmem writes the first byte received to
s->intrstatus. But ivshmem_device_spec.txt says "The status register is
set to 1 when an interrupt occurs." After this commit, follows the
specification, set to 1 when an interrupt occurs.

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 c601a8a..3d8a854 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)
@@ -588,7 +569,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 v2 8/8] char: remove qemu_chr_open_eventfd

[marcandre.lureau]

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

Broken since d0d7708ba29cbc, since the backend is NULL.

And now 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 598dd2b..e035d1c 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -360,9 +360,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 ca53e8c..1605b30 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2838,19 +2838,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, NULL, NULL);
-
-    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 v2 1/8] ivshmem: no need for opaque argument

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

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

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Add a cleanup_vm() function to free QPCIDevice & QPCIBus when cleaning
> up the IVState.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 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>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> 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>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 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 | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 5029789..c601a8a 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -769,18 +769,25 @@ static void ivshmem_reset(DeviceState *d)
>      ivshmem_use_msix(s);
>  }
>  
> -static int ivshmem_setup_msi(IVShmemState * s)
> +static int ivshmem_setup_interrupts(IVShmemState *s)
>  {
> -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> -        return -1;
> +    /* allocate QEMU callback data for receiving interrupts */
> +    if (s->vectors > 0) {
> +        s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
> +        if (!s->msi_vectors) {

Can't happen; s->vectors * sizeof(MSIVector) != 0.

> +            return -1;
> +        }
>      }
>  
> -    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)) {
> +            return -1;
> +        }
>  
> -    /* 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;
>  }
>  
> @@ -947,9 +954,8 @@ 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) < 0) {
> +            error_setg(errp, "failed to initialize interrupts");
>              return;
>          }

With the useless conditional dropped:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 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.
>
> Before this patch, ivshmem writes the first byte received to
> s->intrstatus. But ivshmem_device_spec.txt says "The status register is
> set to 1 when an interrupt occurs."

Suggest to add: Fortunately, the byte usually comes from another ivshmem
device, and those always write 1.

>                                     After this commit, follows the
> specification, set to 1 when an interrupt occurs.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Acked-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Broken since d0d7708ba29cbc, since the backend is NULL.
>
> And now no longer needed by ivshmem.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This is a ivshmem series with various bits:
> - add a test for msi=off regression (the fix is included in the series
>   but was sent separatly to cc -stable)
> - get rid of CharDriver usage for eventfd & fix regression introduced
>   by d0d7708ba by Daniel Berrange
> - fix some test leaks
> - simplify event callback
>
> v1->v2: after Markus Armbruster review,
> - add a few comments to commit messages
> - fix !msi with nvectors==0 regression
> - comment in commit message that the status register is set to 1 when
>   an interrupt occurs, following the ivshmem specification
> - found out that getting rid of CharDriver also fixed regression from
>   d0d7708ba
> - rebase

The two nits I found seem harmless enough to me to sneak into the pull
request rather than post a full v3 first.  Use your judgement.

Thanks!

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

[Marc-André Lureau]

Hi

----- Original Message -----
> 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 | 26 ++++++++++++++++----------
> >  1 file changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> > index 5029789..c601a8a 100644
> > --- a/hw/misc/ivshmem.c
> > +++ b/hw/misc/ivshmem.c
> > @@ -769,18 +769,25 @@ static void ivshmem_reset(DeviceState *d)
> >      ivshmem_use_msix(s);
> >  }
> >  
> > -static int ivshmem_setup_msi(IVShmemState * s)
> > +static int ivshmem_setup_interrupts(IVShmemState *s)
> >  {
> > -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> > -        return -1;
> > +    /* allocate QEMU callback data for receiving interrupts */
> > +    if (s->vectors > 0) {
> > +        s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
> > +        if (!s->msi_vectors) {
> 
> Can't happen; s->vectors * sizeof(MSIVector) != 0.

It can't happen on allocation failure?

> > +            return -1;
> > +        }
> >      }
> >  
> > -    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)) {
> > +            return -1;
> > +        }
> >  
> > -    /* 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;
> >  }
> >  
> > @@ -947,9 +954,8 @@ 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) < 0) {
> > +            error_setg(errp, "failed to initialize interrupts");
> >              return;
> >          }
> 
> With the useless conditional dropped:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

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

[Markus Armbruster]

Marc-André Lureau <mlureau@redhat.com> writes:

> Hi
>
> ----- Original Message -----
>> 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 | 26 ++++++++++++++++----------
>> >  1 file changed, 16 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> > index 5029789..c601a8a 100644
>> > --- a/hw/misc/ivshmem.c
>> > +++ b/hw/misc/ivshmem.c
>> > @@ -769,18 +769,25 @@ static void ivshmem_reset(DeviceState *d)
>> >      ivshmem_use_msix(s);
>> >  }
>> >  
>> > -static int ivshmem_setup_msi(IVShmemState * s)
>> > +static int ivshmem_setup_interrupts(IVShmemState *s)
>> >  {
>> > -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
>> > -        return -1;
>> > +    /* allocate QEMU callback data for receiving interrupts */
>> > +    if (s->vectors > 0) {
>> > +        s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
>> > +        if (!s->msi_vectors) {
>> 
>> Can't happen; s->vectors * sizeof(MSIVector) != 0.
>
> It can't happen on allocation failure?

Nope.

Compare g_malloc0() and g_try_malloc0() at
https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html

We generally use the "try" variants only for memory allocations that can
execute during operation and can be "big".

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

[Marc-André Lureau]

Hi

----- Original Message -----
> Marc-André Lureau <mlureau@redhat.com> writes:
> 
> > Hi
> >
> > ----- Original Message -----
> >> 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 | 26 ++++++++++++++++----------
> >> >  1 file changed, 16 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >> > index 5029789..c601a8a 100644
> >> > --- a/hw/misc/ivshmem.c
> >> > +++ b/hw/misc/ivshmem.c
> >> > @@ -769,18 +769,25 @@ static void ivshmem_reset(DeviceState *d)
> >> >      ivshmem_use_msix(s);
> >> >  }
> >> >  
> >> > -static int ivshmem_setup_msi(IVShmemState * s)
> >> > +static int ivshmem_setup_interrupts(IVShmemState *s)
> >> >  {
> >> > -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> >> > -        return -1;
> >> > +    /* allocate QEMU callback data for receiving interrupts */
> >> > +    if (s->vectors > 0) {
> >> > +        s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
> >> > +        if (!s->msi_vectors) {
> >> 
> >> Can't happen; s->vectors * sizeof(MSIVector) != 0.
> >
> > It can't happen on allocation failure?
> 
> Nope.
> 
> Compare g_malloc0() and g_try_malloc0() at
> https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html
> 
> We generally use the "try" variants only for memory allocations that can
> execute during operation and can be "big".

oops, that was too obvious ;)

thanks