Re: [PATCH for-8.0 00/19] Convert most CPU classes to 3-phase reset
On 11/24/22 03:50, Peter Maydell wrote: Peter Maydell (19): hw/core/cpu-common: Convert TYPE_CPU class to 3-phase reset target/arm: Convert to 3-phase reset target/avr: Convert to 3-phase reset target/cris: Convert to 3-phase reset target/hexagon: Convert to 3-phase reset target/i386: Convert to 3-phase reset target/loongarch: Convert to 3-phase reset target/m68k: Convert to 3-phase reset target/microblaze: Convert to 3-phase reset target/mips: Convert to 3-phase reset target/nios2: Convert to 3-phase reset target/openrisc: Convert to 3-phase reset target/ppc: Convert to 3-phase reset target/riscv: Convert to 3-phase reset target/rx: Convert to 3-phase reset target/sh4: Convert to 3-phase reset target/sparc: Convert to 3-phase reset target/tricore: Convert to 3-phase reset target/xtensa: Convert to 3-phase reset Reviewed-by: Richard Henderson r~
[PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local function vu_panic
Signed-off-by: Stefan Weil
Reviewed-by: Marc-André Lureau
Message-Id: <20220422070144.1043697-4...@weilnetz.de>
Signed-off-by: Laurent Vivier
---
subprojects/libvhost-user/libvhost-user.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c
b/subprojects/libvhost-user/libvhost-user.c
index 80f9952e71..d6ee6e7d91 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -45,6 +45,17 @@
#include "libvhost-user.h"
/* usually provided by GLib */
+#if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 4)
+#if !defined(__clang__) && (__GNUC__ == 4 && __GNUC_MINOR__ == 4)
+#define G_GNUC_PRINTF(format_idx, arg_idx) \
+ __attribute__((__format__(gnu_printf, format_idx, arg_idx)))
+#else
+#define G_GNUC_PRINTF(format_idx, arg_idx) \
+ __attribute__((__format__(__printf__, format_idx, arg_idx)))
+#endif
+#else /* !__GNUC__ */
+#define G_GNUC_PRINTF(format_idx, arg_idx)
+#endif /* !__GNUC__ */
#ifndef MIN
#define MIN(x, y) ({\
typeof(x) _min1 = (x); \
@@ -151,7 +162,7 @@ vu_request_to_string(unsigned int req)
}
}
-static void
+static void G_GNUC_PRINTF(2, 3)
vu_panic(VuDev *dev, const char *msg, ...)
{
char *buf = NULL;
--
2.35.1
[PATCH v3 for-7.2 5/6] MAINTAINERS: Add subprojects/libvhost-user to section "vhost"
Signed-off-by: Stefan Weil --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index cf24910249..6966490c94 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2005,6 +2005,7 @@ F: docs/interop/vhost-user.rst F: contrib/vhost-user-*/ F: backends/vhost-user.c F: include/sysemu/vhost-user-backend.h +F: subprojects/libvhost-user/ virtio M: Michael S. Tsirkin -- 2.35.1
[PATCH v3 for-7.2 6/6] Add G_GNUC_PRINTF to function qemu_set_info_str and fix related issues
With the G_GNUC_PRINTF function attribute the compiler detects two potential insecure format strings: ../../../net/stream.c:248:31: warning: format string is not a string literal (potentially insecure) [-Wformat-security] qemu_set_info_str(>nc, uri); ^~~ ../../../net/stream.c:322:31: warning: format string is not a string literal (potentially insecure) [-Wformat-security] qemu_set_info_str(>nc, uri); ^~~ There are also two other warnings: ../../../net/socket.c:182:35: warning: zero-length gnu_printf format string [-Wformat-zero-length] 182 | qemu_set_info_str(>nc, ""); | ^~ ../../../net/stream.c:170:35: warning: zero-length gnu_printf format string [-Wformat-zero-length] 170 | qemu_set_info_str(>nc, ""); Reviewed-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Stefan Weil --- include/net/net.h | 3 ++- net/socket.c | 2 +- net/stream.c | 6 +++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/include/net/net.h b/include/net/net.h index 3db75ff841..dc20b31e9f 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -177,7 +177,8 @@ ssize_t qemu_send_packet_async(NetClientState *nc, const uint8_t *buf, void qemu_purge_queued_packets(NetClientState *nc); void qemu_flush_queued_packets(NetClientState *nc); void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge); -void qemu_set_info_str(NetClientState *nc, const char *fmt, ...); +void qemu_set_info_str(NetClientState *nc, + const char *fmt, ...) G_GNUC_PRINTF(2, 3); void qemu_format_nic_info_str(NetClientState *nc, uint8_t macaddr[6]); bool qemu_has_ufo(NetClientState *nc); bool qemu_has_vnet_hdr(NetClientState *nc); diff --git a/net/socket.c b/net/socket.c index 4944bb70d5..e62137c839 100644 --- a/net/socket.c +++ b/net/socket.c @@ -179,7 +179,7 @@ static void net_socket_send(void *opaque) s->fd = -1; net_socket_rs_init(>rs, net_socket_rs_finalize, false); s->nc.link_down = true; -qemu_set_info_str(>nc, ""); +qemu_set_info_str(>nc, "%s", ""); return; } diff --git a/net/stream.c b/net/stream.c index 53b7040cc4..37ff727e0c 100644 --- a/net/stream.c +++ b/net/stream.c @@ -167,7 +167,7 @@ static gboolean net_stream_send(QIOChannel *ioc, net_socket_rs_init(>rs, net_stream_rs_finalize, false); s->nc.link_down = true; -qemu_set_info_str(>nc, ""); +qemu_set_info_str(>nc, "%s", ""); qapi_event_send_netdev_stream_disconnected(s->nc.name); @@ -245,7 +245,7 @@ static void net_stream_listen(QIONetListener *listener, } g_assert(addr != NULL); uri = socket_uri(addr); -qemu_set_info_str(>nc, uri); +qemu_set_info_str(>nc, "%s", uri); g_free(uri); qapi_event_send_netdev_stream_connected(s->nc.name, addr); qapi_free_SocketAddress(addr); @@ -319,7 +319,7 @@ static void net_stream_client_connected(QIOTask *task, gpointer opaque) addr = qio_channel_socket_get_remote_address(sioc, NULL); g_assert(addr != NULL); uri = socket_uri(addr); -qemu_set_info_str(>nc, uri); +qemu_set_info_str(>nc, "%s", uri); g_free(uri); ret = qemu_socket_try_set_nonblock(sioc->fd); -- 2.35.1
[PATCH v3 for-7.2 3/6] libvhost-user: Fix two more format strings
This fix is required for 32 bit hosts. The bug was detected by CI
for arm-linux, but is also relevant for i386-linux.
Reported-by: Stefan Hajnoczi
Signed-off-by: Stefan Weil
---
subprojects/libvhost-user/libvhost-user.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c
b/subprojects/libvhost-user/libvhost-user.c
index d67953a1c3..80f9952e71 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,8 @@ generate_faults(VuDev *dev) {
if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, _struct)) {
vu_panic(dev, "%s: Failed to userfault region %d "
- "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
+ "@%" PRIx64 " + size:%" PRIx64 " offset: %" PRIx64
+ ": (ufd=%d)%s\n",
__func__, i,
dev_region->mmap_addr,
dev_region->size, dev_region->mmap_offset,
--
2.35.1
[PATCH v3 for-7.2 0/6] Add format attributes and fix format strings
v3: - Fix description for patch 3 - Add patches 5 and 6 The patches 3 and 5 still need reviews! [PATCH v3 for-7.2 1/6] libvhost-user: Fix wrong type of argument to [PATCH v3 for-7.2 2/6] libvhost-user: Fix format strings [PATCH v3 for-7.2 3/6] libvhost-user: Fix two more format strings [PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local [PATCH v3 for-7.2 5/6] MAINTAINERS: Add subprojects/libvhost-user to [PATCH v3 for-7.2 6/6] Add G_GNUC_PRINTF to function
[PATCH v3 for-7.2 2/6] libvhost-user: Fix format strings
Signed-off-by: Stefan Weil
Reviewed-by: Marc-André Lureau
Message-Id: <20220422070144.1043697-3...@weilnetz.de>
Signed-off-by: Laurent Vivier
---
subprojects/libvhost-user/libvhost-user.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c
b/subprojects/libvhost-user/libvhost-user.c
index d9a6e3e556..d67953a1c3 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -700,7 +700,7 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
close(vmsg->fds[0]);
vu_panic(dev, "VHOST_USER_ADD_MEM_REG requires a message size of at "
- "least %d bytes and only %d bytes were received",
+ "least %zu bytes and only %d bytes were received",
VHOST_USER_MEM_REG_SIZE, vmsg->size);
return false;
}
@@ -826,7 +826,7 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
vmsg_close_fds(vmsg);
vu_panic(dev, "VHOST_USER_REM_MEM_REG requires a message size of at "
- "least %d bytes and only %d bytes were received",
+ "least %zu bytes and only %d bytes were received",
VHOST_USER_MEM_REG_SIZE, vmsg->size);
return false;
}
--
2.35.1
[PATCH v3 for-7.2 1/6] libvhost-user: Fix wrong type of argument to formatting function (reported by LGTM)
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Stefan Weil
Message-Id: <20220422070144.1043697-2...@weilnetz.de>
Signed-off-by: Laurent Vivier
---
subprojects/libvhost-user/libvhost-user.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c
b/subprojects/libvhost-user/libvhost-user.c
index ffed4729a3..d9a6e3e556 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,7 @@ generate_faults(VuDev *dev) {
if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, _struct)) {
vu_panic(dev, "%s: Failed to userfault region %d "
- "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
+ "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
__func__, i,
dev_region->mmap_addr,
dev_region->size, dev_region->mmap_offset,
--
2.35.1
Re: [PATCH for 7.2-rc? v2 0/5] continuing efforts to fix vhost-user issues
Stefan Hajnoczi writes: > On Sat, 26 Nov 2022 at 04:45, Alex Bennée wrote: >> >> >> Alex Bennée writes: >> >> > Alex Bennée writes: >> > >> >> Hi, >> >> >> > >> >> I can replicate some of the other failures I've been seeing in CI by >> >> running: >> >> >> >> ../../meson/meson.py test --repeat 10 --print-errorlogs >> >> qtest-arm/qos-test >> >> >> >> however this seems to run everything in parallel and maybe is better >> >> at exposing race conditions. Perhaps the CI system makes those races >> >> easier to hit? Unfortunately I've not been able to figure out exactly >> >> how things go wrong in the failure case. >> >> >> > >> > >> > There is a circular call - we are in vu_gpio_stop which triggers a write >> > to vhost-user which allows us to catch a disconnect event: >> > >> > #0 vhost_dev_is_started (hdev=0x557adf80d878) at >> > /home/alex/lsrc/qemu.git/include/hw/virtio/vhost.h:199 >> > #1 0x557adbe0518a in vu_gpio_stop (vdev=0x557adf80d640) at >> > ../../hw/virtio/vhost-user-gpio.c:138 >> > #2 0x557adbe04d56 in vu_gpio_disconnect (dev=0x557adf80d640) >> > at ../../hw/virtio/vhost-user-gpio.c:255 >> > #3 0x557adbe049bb in vu_gpio_event (opaque=0x557adf80d640, >> > event=CHR_EVENT_CLOSED) at ../../hw/virtio/vhost-user-gpio.c:274 >> >> I suspect the best choice here is to schedule the cleanup as a later >> date. Should I use the aio_bh one shots for this or maybe an rcu cleanup >> event? >> >> Paolo, any suggestions? >> >> > #4 0x557adc0539ef in chr_be_event (s=0x557adea51f10, >> > event=CHR_EVENT_CLOSED) at ../../chardev/char.c:61 >> > #5 0x557adc0506aa in qemu_chr_be_event (s=0x557adea51f10, >> > event=CHR_EVENT_CLOSED) at ../../chardev/char.c:81 >> > #6 0x557adc04f666 in tcp_chr_disconnect_locked >> > (chr=0x557adea51f10) at ../../chardev/char-socket.c:470 >> > #7 0x557adc04c81a in tcp_chr_write (chr=0x557adea51f10, >> > buf=0x7ffe8588cce0 "\v", len=20) at >> > ../../chardev/char-socket.c:129 > > Does this mean the backend closed the connection before receiving all > the vhost-user protocol messages sent by QEMU? > > This looks like a backend bug. It prevents QEMU's vhost-user client > from cleanly stopping the virtqueue (vhost_virtqueue_stop()). Well the backend in this case is the qtest framework so not the worlds most complete implementation. > QEMU is still broken if it cannot handle disconnect at any time. Maybe > a simple solution for that is to check for reentrancy (either by > checking an existing variable or adding a new one to prevent > vu_gpio_stop() from calling itself). vhost-user-blk introduced an additional flag: /* * There are at least two steps of initialization of the * vhost-user device. The first is a "connect" step and * second is a "start" step. Make a separation between * those initialization phases by using two fields. */ /* vhost_user_blk_connect/vhost_user_blk_disconnect */ bool connected; /* vhost_user_blk_start/vhost_user_blk_stop */ bool started_vu; but that in itself is not enough. If you look at the various cases of handling CHR_EVENT_CLOSED you'll see some schedule the shutdown with aio and some don't even bother (so will probably break the same way). Rather than have a mish-mash of solutions maybe we should introduce a new vhost function - vhost_user_async_close() which can take care of the scheduling and wrap it with a check for a valid vhost structure in case it gets shutdown in the meantime? > >> > #8 0x557adc050999 in qemu_chr_write_buffer (s=0x557adea51f10, >> > buf=0x7ffe8588cce0 "\v", len=20, offset=0x7ffe8588cbe4, write_all=true) at >> > ../../chardev/char.c:121 >> > #9 0x557adc0507c7 in qemu_chr_write (s=0x557adea51f10, >> > buf=0x7ffe8588cce0 "\v", len=20, write_all=true) at >> > ../../chardev/char.c:173 >> > #10 0x557adc046f3a in qemu_chr_fe_write_all (be=0x557adf80d830, >> > buf=0x7ffe8588cce0 "\v", len=20) at ../../chardev/char-fe.c:53 >> > #11 0x557adbddc02f in vhost_user_write (dev=0x557adf80d878, >> > msg=0x7ffe8588cce0, fds=0x0, fd_num=0) at ../../hw/virtio/vhost-user.c:490 >> > #12 0x557adbddd48f in vhost_user_get_vring_base (dev=0x557adf80d878, >> > ring=0x7ffe8588d000) at ../../hw/virtio/vhost-user.c:1260 >> > #13 0x557adbdd4bd6 in vhost_virtqueue_stop (dev=0x557adf80d878, >> > vdev=0x557adf80d640, vq=0x557adf843570, idx=0) at >> > ../../hw/virtio/vhost.c:1220 >> > #14 0x557adbdd7eda in vhost_dev_stop (hdev=0x557adf80d878, >> > vdev=0x557adf80d640, vrings=false) at ../../hw/virtio/vhost.c:1916 >> > #15 0x557adbe051a6 in vu_gpio_stop (vdev=0x557adf80d640) at >> > ../../hw/virtio/vhost-user-gpio.c:142 >> > #16 0x557adbe04849 in vu_gpio_set_status (vdev=0x557adf80d640, >> > status=15 '\017') at ../../hw/virtio/vhost-user-gpio.c:173 >> > #17 0x557adbdc87ff in virtio_set_status (vdev=0x557adf80d640, val=15 >> > '\017') at ../../hw/virtio/virtio.c:2442 >> > #18
Re: [PATCH] 9pfs: Fix some return statements in the synth backend
On Thursday, November 24, 2022 4:58:38 PM CET Greg Kurz wrote: > The qemu_v9fs_synth_mkdir() and qemu_v9fs_synth_add_file() functions > currently return a positive errno value on failure. This causes > checkpatch.pl to spit several errors like the one below: > > ERROR: return of an errno should typically be -ve (return -EAGAIN) > #79: FILE: hw/9pfs/9p-synth.c:79: > +return EAGAIN; > > Simply change the sign. This has no consequence since callers > assert() the returned value to be equal to 0. > > While here also get rid of the uneeded ret variables as suggested > by return_directly.cocci. > > Reported-by: Markus Armbruster > Signed-off-by: Greg Kurz > --- > hw/9pfs/9p-synth.c | 22 -- > 1 file changed, 8 insertions(+), 14 deletions(-) Queued on 9p.next: https://github.com/cschoenebeck/qemu/commits/9p.next I would have expected more locations like that. Thanks! Best regards, Christian Schoenebeck
Re: [PATCH for 7.2-rc? v2 0/5] continuing efforts to fix vhost-user issues
On Sat, 26 Nov 2022 at 04:45, Alex Bennée wrote: > > > Alex Bennée writes: > > > Alex Bennée writes: > > > >> Hi, > >> > > > >> I can replicate some of the other failures I've been seeing in CI by > >> running: > >> > >> ../../meson/meson.py test --repeat 10 --print-errorlogs > >> qtest-arm/qos-test > >> > >> however this seems to run everything in parallel and maybe is better > >> at exposing race conditions. Perhaps the CI system makes those races > >> easier to hit? Unfortunately I've not been able to figure out exactly > >> how things go wrong in the failure case. > >> > > > > > > There is a circular call - we are in vu_gpio_stop which triggers a write > > to vhost-user which allows us to catch a disconnect event: > > > > #0 vhost_dev_is_started (hdev=0x557adf80d878) at > > /home/alex/lsrc/qemu.git/include/hw/virtio/vhost.h:199 > > #1 0x557adbe0518a in vu_gpio_stop (vdev=0x557adf80d640) at > > ../../hw/virtio/vhost-user-gpio.c:138 > > #2 0x557adbe04d56 in vu_gpio_disconnect (dev=0x557adf80d640) at > > ../../hw/virtio/vhost-user-gpio.c:255 > > #3 0x557adbe049bb in vu_gpio_event (opaque=0x557adf80d640, > > event=CHR_EVENT_CLOSED) at ../../hw/virtio/vhost-user-gpio.c:274 > > I suspect the best choice here is to schedule the cleanup as a later > date. Should I use the aio_bh one shots for this or maybe an rcu cleanup > event? > > Paolo, any suggestions? > > > #4 0x557adc0539ef in chr_be_event (s=0x557adea51f10, > > event=CHR_EVENT_CLOSED) at ../../chardev/char.c:61 > > #5 0x557adc0506aa in qemu_chr_be_event (s=0x557adea51f10, > > event=CHR_EVENT_CLOSED) at ../../chardev/char.c:81 > > #6 0x557adc04f666 in tcp_chr_disconnect_locked (chr=0x557adea51f10) > > at ../../chardev/char-socket.c:470 > > #7 0x557adc04c81a in tcp_chr_write (chr=0x557adea51f10, > > buf=0x7ffe8588cce0 "\v", len=20) at ../../chardev/char-socket.c:129 Does this mean the backend closed the connection before receiving all the vhost-user protocol messages sent by QEMU? This looks like a backend bug. It prevents QEMU's vhost-user client from cleanly stopping the virtqueue (vhost_virtqueue_stop()). QEMU is still broken if it cannot handle disconnect at any time. Maybe a simple solution for that is to check for reentrancy (either by checking an existing variable or adding a new one to prevent vu_gpio_stop() from calling itself). > > #8 0x557adc050999 in qemu_chr_write_buffer (s=0x557adea51f10, > > buf=0x7ffe8588cce0 "\v", len=20, offset=0x7ffe8588cbe4, write_all=true) at > > ../../chardev/char.c:121 > > #9 0x557adc0507c7 in qemu_chr_write (s=0x557adea51f10, > > buf=0x7ffe8588cce0 "\v", len=20, write_all=true) at ../../chardev/char.c:173 > > #10 0x557adc046f3a in qemu_chr_fe_write_all (be=0x557adf80d830, > > buf=0x7ffe8588cce0 "\v", len=20) at ../../chardev/char-fe.c:53 > > #11 0x557adbddc02f in vhost_user_write (dev=0x557adf80d878, > > msg=0x7ffe8588cce0, fds=0x0, fd_num=0) at ../../hw/virtio/vhost-user.c:490 > > #12 0x557adbddd48f in vhost_user_get_vring_base (dev=0x557adf80d878, > > ring=0x7ffe8588d000) at ../../hw/virtio/vhost-user.c:1260 > > #13 0x557adbdd4bd6 in vhost_virtqueue_stop (dev=0x557adf80d878, > > vdev=0x557adf80d640, vq=0x557adf843570, idx=0) at > > ../../hw/virtio/vhost.c:1220 > > #14 0x557adbdd7eda in vhost_dev_stop (hdev=0x557adf80d878, > > vdev=0x557adf80d640, vrings=false) at ../../hw/virtio/vhost.c:1916 > > #15 0x557adbe051a6 in vu_gpio_stop (vdev=0x557adf80d640) at > > ../../hw/virtio/vhost-user-gpio.c:142 > > #16 0x557adbe04849 in vu_gpio_set_status (vdev=0x557adf80d640, > > status=15 '\017') at ../../hw/virtio/vhost-user-gpio.c:173 > > #17 0x557adbdc87ff in virtio_set_status (vdev=0x557adf80d640, val=15 > > '\017') at ../../hw/virtio/virtio.c:2442 > > #18 0x557adbdcbfa0 in virtio_vmstate_change (opaque=0x557adf80d640, > > running=false, state=RUN_STATE_SHUTDOWN) at ../../hw/virtio/virtio.c:3736 > > #19 0x557adb91ad27 in vm_state_notify (running=false, > > state=RUN_STATE_SHUTDOWN) at ../../softmmu/runstate.c:334 > > #20 0x557adb910e88 in do_vm_stop (state=RUN_STATE_SHUTDOWN, > > send_stop=false) at ../../softmmu/cpus.c:262 > > #21 0x557adb910e30 in vm_shutdown () at ../../softmmu/cpus.c:280 > > #22 0x557adb91b9c3 in qemu_cleanup () at ../../softmmu/runstate.c:827 > > #23 0x557adb522975 in qemu_default_main () at ../../softmmu/main.c:38 > > #24 0x557adb5229a8 in main (argc=27, argv=0x7ffe8588d2f8) at > > ../../softmmu/main.c:48 > > (rr) p hdev->started > > $9 = true > > (rr) info thread > > Id Target IdFrame > > * 1Thread 2140414.2140414 (qemu-system-aar) vhost_dev_is_started > > (hdev=0x557adf80d878) at > > /home/alex/lsrc/qemu.git/include/hw/virtio/vhost.h:199 > > 2Thread 2140414.2140439 (qemu-system-aar) 0x7002 in > > syscall_traced () > > 3Thread
Re: [PATCH v2] vfio/pci: Verify each MSI vector to avoid invalid MSI vectors
On Sat, 26 Nov 2022 06:33:15 +,
"chenxiang (M)" wrote:
>
>
> 在 2022/11/23 20:08, Marc Zyngier 写道:
> > On Wed, 23 Nov 2022 01:42:36 +,
> > chenxiang wrote:
> >> From: Xiang Chen
> >>
> >> Currently the number of MSI vectors comes from register PCI_MSI_FLAGS
> >> which should be power-of-2 in qemu, in some scenaries it is not the same as
> >> the number that driver requires in guest, for example, a PCI driver wants
> >> to allocate 6 MSI vecotrs in guest, but as the limitation, it will allocate
> >> 8 MSI vectors. So it requires 8 MSI vectors in qemu while the driver in
> >> guest only wants to allocate 6 MSI vectors.
> >>
> >> When GICv4.1 is enabled, it iterates over all possible MSIs and enable the
> >> forwarding while the guest has only created some of mappings in the virtual
> >> ITS, so some calls fail. The exception print is as following:
> >> vfio-pci :3a:00.1: irq bypass producer (token 8f08224d)
> >> registration
> >> fails:66311
> >>
> >> To avoid the issue, verify each MSI vector, skip some operations such as
> >> request_irq() and irq_bypass_register_producer() for those invalid MSI
> >> vectors.
> >>
> >> Signed-off-by: Xiang Chen
> >> ---
> >> I reported the issue at the link:
> >> https://lkml.kernel.org/lkml/87cze9lcut.wl-...@kernel.org/T/
> >>
> >> Change Log:
> >> v1 -> v2:
> >> Verify each MSI vector in kernel instead of adding systemcall according to
> >> Mar's suggestion
> >> ---
> >> arch/arm64/kvm/vgic/vgic-irqfd.c | 13 +
> >> arch/arm64/kvm/vgic/vgic-its.c| 36
> >>
> >> arch/arm64/kvm/vgic/vgic.h| 1 +
> >> drivers/vfio/pci/vfio_pci_intrs.c | 33 +
> >> include/linux/kvm_host.h | 2 ++
> >> 5 files changed, 85 insertions(+)
> >>
> >> diff --git a/arch/arm64/kvm/vgic/vgic-irqfd.c
> >> b/arch/arm64/kvm/vgic/vgic-irqfd.c
> >> index 475059b..71f6af57 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-irqfd.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-irqfd.c
> >> @@ -98,6 +98,19 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> >>return vgic_its_inject_msi(kvm, );
> >> }
> >> +int kvm_verify_msi(struct kvm *kvm,
> >> + struct kvm_kernel_irq_routing_entry *irq_entry)
> >> +{
> >> + struct kvm_msi msi;
> >> +
> >> + if (!vgic_has_its(kvm))
> >> + return -ENODEV;
> >> +
> >> + kvm_populate_msi(irq_entry, );
> >> +
> >> + return vgic_its_verify_msi(kvm, );
> >> +}
> >> +
> >> /**
> >>* kvm_arch_set_irq_inatomic: fast-path for irqfd injection
> >>*/
> >> diff --git a/arch/arm64/kvm/vgic/vgic-its.c
> >> b/arch/arm64/kvm/vgic/vgic-its.c
> >> index 94a666d..8312a4a 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-its.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> >> @@ -767,6 +767,42 @@ int vgic_its_inject_cached_translation(struct kvm
> >> *kvm, struct kvm_msi *msi)
> >>return 0;
> >> }
> >> +int vgic_its_verify_msi(struct kvm *kvm, struct kvm_msi *msi)
> >> +{
> >> + struct vgic_its *its;
> >> + struct its_ite *ite;
> >> + struct kvm_vcpu *vcpu;
> >> + int ret = 0;
> >> +
> >> + if (!irqchip_in_kernel(kvm) || (msi->flags & ~KVM_MSI_VALID_DEVID))
> >> + return -EINVAL;
> >> +
> >> + if (!vgic_has_its(kvm))
> >> + return -ENODEV;
> >> +
> >> + its = vgic_msi_to_its(kvm, msi);
> >> + if (IS_ERR(its))
> >> + return PTR_ERR(its);
> >> +
> >> + mutex_lock(>its_lock);
> >> + if (!its->enabled) {
> >> + ret = -EBUSY;
> >> + goto unlock;
> >> + }
> >> + ite = find_ite(its, msi->devid, msi->data);
> >> + if (!ite || !its_is_collection_mapped(ite->collection)) {
> >> + ret = E_ITS_INT_UNMAPPED_INTERRUPT;
> >> + goto unlock;
> >> + }
> >> +
> >> + vcpu = kvm_get_vcpu(kvm, ite->collection->target_addr);
> >> + if (!vcpu)
> >> + ret = E_ITS_INT_UNMAPPED_INTERRUPT;
> > I'm sorry, but what does this mean to the caller? This should never
> > leak outside of the ITS code.
>
> Actually it is already leak outside of ITS code, and please see the
> exception printk (E_ITS_INT_UNMAPPED_INTERRUPT is 0x10307 which is
> equal to 66311):
>
> vfio-pci :3a:00.1: irq bypass producer (token 8f08224d)
> registration fails:66311
>
But that's hardly interpreted, which is the whole point. Only zero is
considered a success value.
> > Honestly, the whole things seems really complicated to avoid something
> > that is only a harmless warning .
>
> It seems also waste some interrupts. Allocating and requesting some
> interrupts but not used.
What makes you think they are not used? A guest can install a mapping
for those at any point. They won't be directly injected, but they will
be delivered to the guest via the normal SW injection mechanism.
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH for 7.2-rc? v2 0/5] continuing efforts to fix vhost-user issues
Alex Bennée writes: > Alex Bennée writes: > >> Hi, >> > >> I can replicate some of the other failures I've been seeing in CI by >> running: >> >> ../../meson/meson.py test --repeat 10 --print-errorlogs qtest-arm/qos-test >> >> however this seems to run everything in parallel and maybe is better >> at exposing race conditions. Perhaps the CI system makes those races >> easier to hit? Unfortunately I've not been able to figure out exactly >> how things go wrong in the failure case. >> > > > There is a circular call - we are in vu_gpio_stop which triggers a write > to vhost-user which allows us to catch a disconnect event: > > #0 vhost_dev_is_started (hdev=0x557adf80d878) at > /home/alex/lsrc/qemu.git/include/hw/virtio/vhost.h:199 > #1 0x557adbe0518a in vu_gpio_stop (vdev=0x557adf80d640) at > ../../hw/virtio/vhost-user-gpio.c:138 > #2 0x557adbe04d56 in vu_gpio_disconnect (dev=0x557adf80d640) at > ../../hw/virtio/vhost-user-gpio.c:255 > #3 0x557adbe049bb in vu_gpio_event (opaque=0x557adf80d640, > event=CHR_EVENT_CLOSED) at ../../hw/virtio/vhost-user-gpio.c:274 I suspect the best choice here is to schedule the cleanup as a later date. Should I use the aio_bh one shots for this or maybe an rcu cleanup event? Paolo, any suggestions? > #4 0x557adc0539ef in chr_be_event (s=0x557adea51f10, > event=CHR_EVENT_CLOSED) at ../../chardev/char.c:61 > #5 0x557adc0506aa in qemu_chr_be_event (s=0x557adea51f10, > event=CHR_EVENT_CLOSED) at ../../chardev/char.c:81 > #6 0x557adc04f666 in tcp_chr_disconnect_locked (chr=0x557adea51f10) at > ../../chardev/char-socket.c:470 > #7 0x557adc04c81a in tcp_chr_write (chr=0x557adea51f10, > buf=0x7ffe8588cce0 "\v", len=20) at ../../chardev/char-socket.c:129 > #8 0x557adc050999 in qemu_chr_write_buffer (s=0x557adea51f10, > buf=0x7ffe8588cce0 "\v", len=20, offset=0x7ffe8588cbe4, write_all=true) at > ../../chardev/char.c:121 > #9 0x557adc0507c7 in qemu_chr_write (s=0x557adea51f10, > buf=0x7ffe8588cce0 "\v", len=20, write_all=true) at ../../chardev/char.c:173 > #10 0x557adc046f3a in qemu_chr_fe_write_all (be=0x557adf80d830, > buf=0x7ffe8588cce0 "\v", len=20) at ../../chardev/char-fe.c:53 > #11 0x557adbddc02f in vhost_user_write (dev=0x557adf80d878, > msg=0x7ffe8588cce0, fds=0x0, fd_num=0) at ../../hw/virtio/vhost-user.c:490 > #12 0x557adbddd48f in vhost_user_get_vring_base (dev=0x557adf80d878, > ring=0x7ffe8588d000) at ../../hw/virtio/vhost-user.c:1260 > #13 0x557adbdd4bd6 in vhost_virtqueue_stop (dev=0x557adf80d878, > vdev=0x557adf80d640, vq=0x557adf843570, idx=0) at ../../hw/virtio/vhost.c:1220 > #14 0x557adbdd7eda in vhost_dev_stop (hdev=0x557adf80d878, > vdev=0x557adf80d640, vrings=false) at ../../hw/virtio/vhost.c:1916 > #15 0x557adbe051a6 in vu_gpio_stop (vdev=0x557adf80d640) at > ../../hw/virtio/vhost-user-gpio.c:142 > #16 0x557adbe04849 in vu_gpio_set_status (vdev=0x557adf80d640, > status=15 '\017') at ../../hw/virtio/vhost-user-gpio.c:173 > #17 0x557adbdc87ff in virtio_set_status (vdev=0x557adf80d640, val=15 > '\017') at ../../hw/virtio/virtio.c:2442 > #18 0x557adbdcbfa0 in virtio_vmstate_change (opaque=0x557adf80d640, > running=false, state=RUN_STATE_SHUTDOWN) at ../../hw/virtio/virtio.c:3736 > #19 0x557adb91ad27 in vm_state_notify (running=false, > state=RUN_STATE_SHUTDOWN) at ../../softmmu/runstate.c:334 > #20 0x557adb910e88 in do_vm_stop (state=RUN_STATE_SHUTDOWN, > send_stop=false) at ../../softmmu/cpus.c:262 > #21 0x557adb910e30 in vm_shutdown () at ../../softmmu/cpus.c:280 > #22 0x557adb91b9c3 in qemu_cleanup () at ../../softmmu/runstate.c:827 > #23 0x557adb522975 in qemu_default_main () at ../../softmmu/main.c:38 > #24 0x557adb5229a8 in main (argc=27, argv=0x7ffe8588d2f8) at > ../../softmmu/main.c:48 > (rr) p hdev->started > $9 = true > (rr) info thread > Id Target IdFrame > * 1Thread 2140414.2140414 (qemu-system-aar) vhost_dev_is_started > (hdev=0x557adf80d878) at > /home/alex/lsrc/qemu.git/include/hw/virtio/vhost.h:199 > 2Thread 2140414.2140439 (qemu-system-aar) 0x7002 in > syscall_traced () > 3Thread 2140414.2140442 (qemu-system-aar) 0x7002 in > syscall_traced () > 4Thread 2140414.2140443 (qemu-system-aar) 0x7002 in > syscall_traced () > > During which we eliminate the vhost_dev with a memset: > > Thread 1 hit Hardware watchpoint 2: *(unsigned int *) 0x557adf80da30 > > Old value = 2 > New value = 0 > __memset_avx2_unaligned_erms () at > ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:220 > Download failed: Invalid argument. Continuing without source file > ./string/../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S. > 220 ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S: No such > file or directory. > (rr) bt > #0
