From df3a2de51a07089a4a729fe1f792f658df9dade4 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Sun, 25 Jul 2021 18:44:04 +0100 Subject: [PATCH 01/12] accel/tcg: Don't use CF_COUNT_MASK as the max value of icount_decr.u16.low MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In cpu_loop_exec_tb() we were bounding the number of insns we might try to execute in a TB using CF_COUNT_MASK. This is incorrect, because we can validly put up to 0xffff into icount_decr.u16.low. In particular, since commit 78ff82bb1b67c0d7 reduced CF_COUNT_MASK to 511 this meant that we would incorrectly only try to execute 511 instructions in a 512-instruction TB, which could result in QEMU hanging when in icount mode. Use the actual maximum value, which is 0xffff. (This brings this code in to line with the similar logic in icount_prepare_for_run() in tcg-accel-ops-icount.c.) Fixes: 78ff82bb1b67c0d7 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé Resolves: https://gitlab.com/qemu-project/qemu/-/issues/499 Message-Id: <20210725174405.24568-2-peter.maydell@linaro.org> Signed-off-by: Richard Henderson --- accel/tcg/cpu-exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index fc895cf51e..6e8dc29119 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -834,7 +834,7 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, /* Ensure global icount has gone forward */ icount_update(cpu); /* Refill decrementer and continue execution. */ - insns_left = MIN(CF_COUNT_MASK, cpu->icount_budget); + insns_left = MIN(0xffff, cpu->icount_budget); cpu_neg(cpu)->icount_decr.u16.low = insns_left; cpu->icount_extra = cpu->icount_budget - insns_left; From c8cf47a946200239826e58abc203dcaa4fdbb30c Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Sun, 25 Jul 2021 18:44:05 +0100 Subject: [PATCH 02/12] accel/tcg: Remove unnecessary check on icount_extra in cpu_loop_exec_tb() In cpu_loop_exec_tb(), we decide whether to look for a TB with exactly insns_left instructions in it using the condition (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount) The check for icount_extra == 0 is unnecessary, because we just set insns_left = MIN(0xffff, cpu->icount_budget); icount_extra = icount_budget - insns_left; and so icount_extra can only be non-zero if icount_budget > 0xffff and insns_left == 0xffff. But in that case insns_left >= tb->icount because 0xffff is much larger than TCG_MAX_INSNS, so the condition will be false anyway. Remove the unnecessary check, and instead assert: * that we are only going to execute a partial TB here if the icount budget has run out (ie icount_extra == 0) * that the number of insns we're going to execute does fit into the CF_COUNT_MASK Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-Id: <20210725174405.24568-3-peter.maydell@linaro.org> Signed-off-by: Richard Henderson --- accel/tcg/cpu-exec.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 6e8dc29119..5aa42fbff3 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -843,7 +843,9 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, * execute we need to ensure we find/generate a TB with exactly * insns_left instructions in it. */ - if (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount) { + if (insns_left > 0 && insns_left < tb->icount) { + assert(insns_left <= CF_COUNT_MASK); + assert(cpu->icount_extra == 0); cpu->cflags_next_tb = (tb->cflags & ~CF_COUNT_MASK) | insns_left; } #endif From 2f0e10a4866c16e0b6f98fcc87add047200e4896 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 25 Jul 2021 12:05:57 +0100 Subject: [PATCH 03/12] bitops.h: revert db1ffc32dd ("qemu/bitops.h: add bitrev8 implementation") MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit db1ffc32dd ("qemu/bitops.h: add bitrev8 implementation") introduced a bitrev8() function to reverse the bit ordering required for storing the MAC address in the q800 PROM. This function is not required since QEMU implements its own revbit8() function which does exactly the same thing. Remove the extraneous bitrev8() function and switch its only caller in hw/m68k/q800.c to use revbit8() instead. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Message-Id: <20210725110557.3007-1-mark.cave-ayland@ilande.co.uk> Signed-off-by: Richard Henderson --- hw/m68k/q800.c | 2 +- include/qemu/bitops.h | 22 ---------------------- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c index 6817c8b5d1..ac0a13060b 100644 --- a/hw/m68k/q800.c +++ b/hw/m68k/q800.c @@ -334,7 +334,7 @@ static void q800_init(MachineState *machine) prom = memory_region_get_ram_ptr(dp8393x_prom); checksum = 0; for (i = 0; i < 6; i++) { - prom[i] = bitrev8(nd_table[0].macaddr.a[i]); + prom[i] = revbit8(nd_table[0].macaddr.a[i]); checksum ^= prom[i]; } prom[7] = 0xff - checksum; diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h index 110c56e099..03213ce952 100644 --- a/include/qemu/bitops.h +++ b/include/qemu/bitops.h @@ -618,26 +618,4 @@ static inline uint64_t half_unshuffle64(uint64_t x) return x; } -/** - * bitrev8: - * @x: 8-bit value to be reversed - * - * Given an input value with bits:: - * - * ABCDEFGH - * - * return the value with its bits reversed from left to right:: - * - * HGFEDCBA - * - * Returns: the bit-reversed value. - */ -static inline uint8_t bitrev8(uint8_t x) -{ - x = ((x >> 1) & 0x55) | ((x << 1) & 0xaa); - x = ((x >> 2) & 0x33) | ((x << 2) & 0xcc); - x = (x >> 4) | (x << 4) ; - return x; -} - #endif From cd1675f8d7fa33b5071a4d3be5dcb69c38300f17 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Tue, 13 Jul 2021 13:58:41 +0000 Subject: [PATCH 04/12] nbd/server: Mark variable unused in nbd_negotiate_meta_queries From clang-13: nbd/server.c:976:22: error: variable 'bitmaps' set but not used \ [-Werror,-Wunused-but-set-variable] which is incorrect; see //bugs.llvm.org/show_bug.cgi?id=3888. Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Richard Henderson --- nbd/server.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nbd/server.c b/nbd/server.c index b60ebc3ab6..3927f7789d 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -973,7 +973,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client, { int ret; g_autofree char *export_name = NULL; - g_autofree bool *bitmaps = NULL; + /* Mark unused to work around https://bugs.llvm.org/show_bug.cgi?id=3888 */ + g_autofree G_GNUC_UNUSED bool *bitmaps = NULL; NBDExportMetaContexts local_meta = {0}; uint32_t nb_queries; size_t i; From 2521c775bdfc8150396c4cb173b54d51f44c207e Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Mon, 12 Jul 2021 18:29:34 +0000 Subject: [PATCH 05/12] accel/tcg: Remove unused variable in cpu_exec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From clang-13: accel/tcg/cpu-exec.c:783:15: error: variable 'cc' set but not used \ [-Werror,-Wunused-but-set-variable] Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- accel/tcg/cpu-exec.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 5aa42fbff3..e5c0ccd1a2 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -855,7 +855,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, int cpu_exec(CPUState *cpu) { - CPUClass *cc = CPU_GET_CLASS(cpu); int ret; SyncClocks sc = { 0 }; @@ -889,19 +888,14 @@ int cpu_exec(CPUState *cpu) * that we support, but is still unfixed in clang: * https://bugs.llvm.org/show_bug.cgi?id=21183 * - * Reload essential local variables here for those compilers. + * Reload an essential local variable here for those compilers. * Newer versions of gcc would complain about this code (-Wclobbered), * so we only perform the workaround for clang. */ cpu = current_cpu; - cc = CPU_GET_CLASS(cpu); #else - /* - * Non-buggy compilers preserve these locals; assert that - * they have the correct value. - */ + /* Non-buggy compilers preserve this; assert the correct value. */ g_assert(cpu == current_cpu); - g_assert(cc == CPU_GET_CLASS(cpu)); #endif #ifndef CONFIG_SOFTMMU From 53645dc4cb7b74db3507558c01612c3e5ea430ab Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Mon, 12 Jul 2021 18:39:06 +0000 Subject: [PATCH 06/12] util/selfmap: Discard mapping on error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From clang-13: util/selfmap.c:26:21: error: variable 'errors' set but not used \ [-Werror,-Wunused-but-set-variable] Quite right of course, but there's no reason not to check errors. First, incrementing errors is incorrect, because qemu_strtoul returns an errno not a count -- just or them together so that we have a non-zero value at the end. Second, if we have an error, do not add the struct to the list, but free it instead. Cc: Alex Bennée Reviewed-by: Eric Blake Signed-off-by: Richard Henderson --- util/selfmap.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/util/selfmap.c b/util/selfmap.c index 2ec99dfdda..2c14f019ce 100644 --- a/util/selfmap.c +++ b/util/selfmap.c @@ -23,29 +23,34 @@ GSList *read_self_maps(void) gchar **fields = g_strsplit(lines[i], " ", 6); if (g_strv_length(fields) > 4) { MapInfo *e = g_new0(MapInfo, 1); - int errors; + int errors = 0; const char *end; - errors = qemu_strtoul(fields[0], &end, 16, &e->start); - errors += qemu_strtoul(end + 1, NULL, 16, &e->end); + errors |= qemu_strtoul(fields[0], &end, 16, &e->start); + errors |= qemu_strtoul(end + 1, NULL, 16, &e->end); e->is_read = fields[1][0] == 'r'; e->is_write = fields[1][1] == 'w'; e->is_exec = fields[1][2] == 'x'; e->is_priv = fields[1][3] == 'p'; - errors += qemu_strtoul(fields[2], NULL, 16, &e->offset); + errors |= qemu_strtoul(fields[2], NULL, 16, &e->offset); e->dev = g_strdup(fields[3]); - errors += qemu_strtou64(fields[4], NULL, 10, &e->inode); + errors |= qemu_strtou64(fields[4], NULL, 10, &e->inode); - /* - * The last field may have leading spaces which we - * need to strip. - */ - if (g_strv_length(fields) == 6) { - e->path = g_strdup(g_strchug(fields[5])); + if (!errors) { + /* + * The last field may have leading spaces which we + * need to strip. + */ + if (g_strv_length(fields) == 6) { + e->path = g_strdup(g_strchug(fields[5])); + } + map_info = g_slist_prepend(map_info, e); + } else { + g_free(e->dev); + g_free(e); } - map_info = g_slist_prepend(map_info, e); } g_strfreev(fields); From fe4bd9170c92e5557f9c49ac08acc54c0d752aba Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Mon, 12 Jul 2021 18:43:58 +0000 Subject: [PATCH 07/12] net/checksum: Remove unused variable in net_checksum_add_iov From clang-13: ../qemu/net/checksum.c:189:23: error: variable 'buf_off' set but not used \ [-Werror,-Wunused-but-set-variable] Reviewed-by: Eric Blake Signed-off-by: Richard Henderson --- net/checksum.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/net/checksum.c b/net/checksum.c index 70f4eaeb3a..68245fd748 100644 --- a/net/checksum.c +++ b/net/checksum.c @@ -186,12 +186,11 @@ uint32_t net_checksum_add_iov(const struct iovec *iov, const unsigned int iov_cnt, uint32_t iov_off, uint32_t size, uint32_t csum_offset) { - size_t iovec_off, buf_off; + size_t iovec_off; unsigned int i; uint32_t res = 0; iovec_off = 0; - buf_off = 0; for (i = 0; i < iov_cnt && size; i++) { if (iov_off < (iovec_off + iov[i].iov_len)) { size_t len = MIN((iovec_off + iov[i].iov_len) - iov_off , size); @@ -200,7 +199,6 @@ net_checksum_add_iov(const struct iovec *iov, const unsigned int iov_cnt, res += net_checksum_add_cont(len, chunk_buf, csum_offset); csum_offset += len; - buf_off += len; iov_off += len; size -= len; } From 79fe9e4313f9dbad8ee4619031900ccabd4f8824 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Mon, 12 Jul 2021 19:57:41 +0000 Subject: [PATCH 08/12] hw/audio/adlib: Remove unused variable in adlib_callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From clang-13: hw/audio/adlib.c:189:18: error: variable 'net' set but not used \ [-Werror,-Wunused-but-set-variable] Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- hw/audio/adlib.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c index 42d50d2fdc..5f979b1487 100644 --- a/hw/audio/adlib.c +++ b/hw/audio/adlib.c @@ -186,7 +186,7 @@ static int write_audio (AdlibState *s, int samples) static void adlib_callback (void *opaque, int free) { AdlibState *s = opaque; - int samples, net = 0, to_play, written; + int samples, to_play, written; samples = free >> SHIFT; if (!(s->active && s->enabled) || !samples) { @@ -219,7 +219,6 @@ static void adlib_callback (void *opaque, int free) written = write_audio (s, samples); if (written) { - net += written; samples -= written; s->pos = (s->pos + written) % s->samples; } From 2d758274ee477a96ba35ba05fe2e393c208e724b Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Mon, 12 Jul 2021 20:00:18 +0000 Subject: [PATCH 09/12] hw/ppc/spapr_events: Remove unused variable from check_exception From clang-13: hw/ppc/spapr_events.c:937:14: error: variable 'xinfo' set but not used \ [-Werror,-Wunused-but-set-variable] Acked-by: David Gibson Signed-off-by: Richard Henderson --- hw/ppc/spapr_events.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index 0cfc19be19..23e2e2fff1 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -934,7 +934,6 @@ static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr, uint32_t nret, target_ulong rets) { uint32_t mask, buf, len, event_len; - uint64_t xinfo; SpaprEventLogEntry *event; struct rtas_error_log header; int i; @@ -944,13 +943,9 @@ static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr, return; } - xinfo = rtas_ld(args, 1); mask = rtas_ld(args, 2); buf = rtas_ld(args, 4); len = rtas_ld(args, 5); - if (nargs == 7) { - xinfo |= (uint64_t)rtas_ld(args, 6) << 32; - } event = rtas_event_log_dequeue(spapr, mask); if (!event) { From 984178d86b92be23b0efb6da769f22adb08cb83a Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Mon, 12 Jul 2021 20:04:37 +0000 Subject: [PATCH 10/12] hw/pci-hist/pnv_phb4: Fix typo in pnv_phb4_ioda_write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From clang-13: hw/pci-host/pnv_phb4.c:375:18: error: variable 'v' set but not used \ [-Werror,-Wunused-but-set-variable] It's pretty clear that we meant to write back 'v' after all that computation and not 'val'. Acked-by: David Gibson Acked-by: Benjamin Herrenschmidt Reviewed-by: Cédric Le Goater Signed-off-by: Richard Henderson --- hw/pci-host/pnv_phb4.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index 54f57c660a..5c375a9f28 100644 --- a/hw/pci-host/pnv_phb4.c +++ b/hw/pci-host/pnv_phb4.c @@ -392,7 +392,7 @@ static void pnv_phb4_ioda_write(PnvPHB4 *phb, uint64_t val) v &= 0xffffffffffff0000ull; v |= 0x000000000000cfffull & val; } - *tptr = val; + *tptr = v; break; } case IODA3_TBL_MBT: From 211364c21e7f757ae1acf4e72b5df39c498fb88b Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Mon, 12 Jul 2021 20:18:46 +0000 Subject: [PATCH 11/12] linux-user/syscall: Remove unused variable from execve From clang-13: linux-user/syscall.c:8503:17: error: variable 'total_size' set but not used \ [-Werror,-Wunused-but-set-variable] Acked-by: Laurent Vivier Signed-off-by: Richard Henderson --- linux-user/syscall.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 376629c689..ccd3892b2d 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -8364,7 +8364,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, abi_ulong guest_envp; abi_ulong addr; char **q; - int total_size = 0; argc = 0; guest_argp = arg2; @@ -8396,7 +8395,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, break; if (!(*q = lock_user_string(addr))) goto execve_efault; - total_size += strlen(*q) + 1; } *q = NULL; @@ -8408,7 +8406,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, break; if (!(*q = lock_user_string(addr))) goto execve_efault; - total_size += strlen(*q) + 1; } *q = NULL; From 2bf07e788eb69bee843be274386fb20f4ab6b0f6 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Mon, 12 Jul 2021 20:26:21 +0000 Subject: [PATCH 12/12] tests/unit: Remove unused variable from test_io MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From clang-13: tests/unit/test-iov.c:161:26: error: variable 't' set but not used \ [-Werror,-Wunused-but-set-variable] Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- tests/unit/test-iov.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/unit/test-iov.c b/tests/unit/test-iov.c index 9c415e2f1f..5371066fb6 100644 --- a/tests/unit/test-iov.c +++ b/tests/unit/test-iov.c @@ -158,7 +158,7 @@ static void test_io(void) int sv[2]; int r; - unsigned i, j, k, s, t; + unsigned i, j, k, s; fd_set fds; unsigned niov; struct iovec *iov, *siov; @@ -182,7 +182,6 @@ static void test_io(void) FD_ZERO(&fds); - t = 0; if (fork() == 0) { /* writer */ @@ -201,7 +200,6 @@ static void test_io(void) g_assert(memcmp(iov, siov, sizeof(*iov)*niov) == 0); if (r >= 0) { k += r; - t += r; usleep(g_test_rand_int_range(0, 30)); } else if (errno == EAGAIN) { select(sv[1]+1, NULL, &fds, NULL, NULL); @@ -238,7 +236,6 @@ static void test_io(void) g_assert(memcmp(iov, siov, sizeof(*iov)*niov) == 0); if (r > 0) { k += r; - t += r; } else if (!r) { if (s) { break;