From 2c64846972897fc3aec4072f849fae2b00322f8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 18 Dec 2015 12:20:51 +0100 Subject: [PATCH 1/8] ivshmem: no need for opaque argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster --- 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 d5c89aeca1..358df2407a 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; From 47213eb1104709bf238c8d16db20aa47d37b1c59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 18 Dec 2015 15:13:08 +0100 Subject: [PATCH 2/8] ivshmem: remove redundant assignment, fix crash with msi=off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix crash when msi=false introduced in 660c97ee (msi_vectors is NULL in this case) Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster --- hw/misc/ivshmem.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index 358df2407a..50297892fd 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) { From 1760048a5d21bacf0e4838da2f61b2d8db7d2866 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 18 Dec 2015 15:13:59 +0100 Subject: [PATCH 3/8] ivshmem-test: leak fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a cleanup_vm() function to free QPCIDevice & QPCIBus when cleaning up the IVState. Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster --- 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 03c7b962a3..d544d5eb88 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) From ea53854a54bc54dddeec0c56572adf53384e960c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 18 Dec 2015 15:13:32 +0100 Subject: [PATCH 4/8] libqos: remove some leaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit qpci_device_find() returns allocated data, don't leak it. Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster --- tests/libqos/pci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c index 4e630c250a..80b1a2117d 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; } From 00ffc3c166d2100e1fb6b5bd192868d338ee825e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 18 Dec 2015 18:14:29 +0100 Subject: [PATCH 5/8] ivshmem-test: test both msi & irq cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recent commit 660c97ee introduced a regression in irq case, make sure this code path is also tested. Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster --- 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 d544d5eb88..705fece717 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(); From fd47bfe5ad423b4b09dc0244bda3b1346fa189ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Mon, 21 Dec 2015 12:08:54 +0100 Subject: [PATCH 6/8] ivshmem: generalize ivshmem_setup_interrupts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Markus Armbruster --- hw/misc/ivshmem.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index 50297892fd..5f33149c91 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -769,18 +769,20 @@ 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; - } - - IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors); - - /* allocate QEMU char devices for receiving interrupts */ + /* allocate QEMU callback data for receiving interrupts */ s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector)); - ivshmem_use_msix(s); + if (ivshmem_has_feature(s, IVSHMEM_MSI)) { + if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) { + return -1; + } + + IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors); + ivshmem_use_msix(s); + } + return 0; } @@ -947,9 +949,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; } From 9940c3236f318949c92099163281d5d23a9fcf4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Mon, 21 Dec 2015 12:10:13 +0100 Subject: [PATCH 7/8] ivshmem: use a single eventfd callback, get rid of CharDriver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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." 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 Acked-by: Markus Armbruster --- 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 5f33149c91..48b7a34a8f 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) { From 6db262557260129883c5aa47d47556f4075a3e5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Mon, 21 Dec 2015 12:26:51 +0100 Subject: [PATCH 8/8] char: remove qemu_chr_open_eventfd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Broken since d0d7708ba29cbc, since the backend is NULL. And now no longer needed by ivshmem. Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster --- 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 598dd2bf49..e035d1cbda 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 ca53e8c376..1605b30c33 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;