From 48e14066ac10581db4e69f75eda107cfdafa6022 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Mon, 29 Nov 2021 14:09:25 +0000 Subject: [PATCH 1/8] accel/tcg: introduce CF_NOIRQ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Here we introduce a new compiler flag to disable the checking of exit request (icount_decr.u32). This is useful when we want to ensure the next block cannot be preempted by an asynchronous event. Suggested-by: Richard Henderson Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson Message-Id: <20211129140932.4115115-2-alex.bennee@linaro.org> --- include/exec/exec-all.h | 1 + include/exec/gen-icount.h | 21 +++++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 6bb2a0f7ec..35d8e93976 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -503,6 +503,7 @@ struct TranslationBlock { #define CF_USE_ICOUNT 0x00020000 #define CF_INVALID 0x00040000 /* TB is stale. Set with @jmp_lock held */ #define CF_PARALLEL 0x00080000 /* Generate code for a parallel context */ +#define CF_NOIRQ 0x00100000 /* Generate an uninterruptible TB */ #define CF_CLUSTER_MASK 0xff000000 /* Top 8 bits are cluster ID */ #define CF_CLUSTER_SHIFT 24 diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h index 610cba58fe..c57204ddad 100644 --- a/include/exec/gen-icount.h +++ b/include/exec/gen-icount.h @@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb) { TCGv_i32 count; - tcg_ctx->exitreq_label = gen_new_label(); if (tb_cflags(tb) & CF_USE_ICOUNT) { count = tcg_temp_local_new_i32(); } else { @@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb) icount_start_insn = tcg_last_op(); } - tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label); + /* + * Emit the check against icount_decr.u32 to see if we should exit + * unless we suppress the check with CF_NOIRQ. If we are using + * icount and have suppressed interruption the higher level code + * should have ensured we don't run more instructions than the + * budget. + */ + if (tb_cflags(tb) & CF_NOIRQ) { + tcg_ctx->exitreq_label = NULL; + } else { + tcg_ctx->exitreq_label = gen_new_label(); + tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label); + } if (tb_cflags(tb) & CF_USE_ICOUNT) { tcg_gen_st16_i32(count, cpu_env, @@ -74,8 +85,10 @@ static inline void gen_tb_end(const TranslationBlock *tb, int num_insns) tcgv_i32_arg(tcg_constant_i32(num_insns))); } - gen_set_label(tcg_ctx->exitreq_label); - tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED); + if (tcg_ctx->exitreq_label) { + gen_set_label(tcg_ctx->exitreq_label); + tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED); + } } #endif From aff0e204cb1f1c036a496c94c15f5dfafcd9b4b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Mon, 29 Nov 2021 14:09:26 +0000 Subject: [PATCH 2/8] accel/tcg: suppress IRQ check for special TBs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we set cpu->cflags_next_tb it is because we want to carefully control the execution of the next TB. Currently there is a race that causes the second stage of watchpoint handling to get ignored if an IRQ is processed before we finish executing the instruction that triggers the watchpoint. Use the new CF_NOIRQ facility to avoid the race. We also suppress IRQs when handling precise self modifying code to avoid unnecessary bouncing. Signed-off-by: Alex Bennée Cc: Pavel Dovgalyuk Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245 Reviewed-by: Richard Henderson Message-Id: <20211129140932.4115115-3-alex.bennee@linaro.org> --- accel/tcg/cpu-exec.c | 9 +++++++++ accel/tcg/translate-all.c | 4 ++-- softmmu/physmem.c | 4 ++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 2d14d02f6c..409ec8c38c 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -721,6 +721,15 @@ static inline bool need_replay_interrupt(int interrupt_request) static inline bool cpu_handle_interrupt(CPUState *cpu, TranslationBlock **last_tb) { + /* + * If we have requested custom cflags with CF_NOIRQ we should + * skip checking here. Any pending interrupts will get picked up + * by the next TB we execute under normal cflags. + */ + if (cpu->cflags_next_tb != -1 && cpu->cflags_next_tb & CF_NOIRQ) { + return false; + } + /* Clear the interrupt flag now since we're processing * cpu->interrupt_request and cpu->exit_request. * Ensure zeroing happens before reading cpu->exit_request or diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index bd0bb81d08..bd71db59a9 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1738,7 +1738,7 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages, if (current_tb_modified) { page_collection_unlock(pages); /* Force execution of one insn next time. */ - cpu->cflags_next_tb = 1 | curr_cflags(cpu); + cpu->cflags_next_tb = 1 | CF_NOIRQ | curr_cflags(cpu); mmap_unlock(); cpu_loop_exit_noexc(cpu); } @@ -1906,7 +1906,7 @@ static bool tb_invalidate_phys_page(tb_page_addr_t addr, uintptr_t pc) #ifdef TARGET_HAS_PRECISE_SMC if (current_tb_modified) { /* Force execution of one insn next time. */ - cpu->cflags_next_tb = 1 | curr_cflags(cpu); + cpu->cflags_next_tb = 1 | CF_NOIRQ | curr_cflags(cpu); return true; } #endif diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 314f8b439c..3524c04c2a 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -912,7 +912,7 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len, */ if (!cpu->can_do_io) { /* Force execution of one insn next time. */ - cpu->cflags_next_tb = 1 | CF_LAST_IO | curr_cflags(cpu); + cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ | curr_cflags(cpu); cpu_loop_exit_restore(cpu, ra); } /* @@ -946,7 +946,7 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len, cpu_loop_exit(cpu); } else { /* Force execution of one insn next time. */ - cpu->cflags_next_tb = 1 | CF_LAST_IO | curr_cflags(cpu); + cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ | curr_cflags(cpu); mmap_unlock(); cpu_loop_exit_noexc(cpu); } From a7c6e562e6f3f4e009c5dfa4e44cbbe908c4bf05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Mon, 29 Nov 2021 14:09:27 +0000 Subject: [PATCH 3/8] tests/avocado: fix tcg_plugin mem access count test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we cleaned up argument handling the test was missed. Fixes: 5ae589faad ("tests/plugins/mem: introduce "track" arg and make args not positional") Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20211129140932.4115115-4-alex.bennee@linaro.org> --- tests/avocado/tcg_plugins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/avocado/tcg_plugins.py b/tests/avocado/tcg_plugins.py index 9ca1515c3b..642d2e49e3 100644 --- a/tests/avocado/tcg_plugins.py +++ b/tests/avocado/tcg_plugins.py @@ -131,7 +131,7 @@ class PluginKernelNormal(PluginKernelBase): suffix=".log") self.run_vm(kernel_path, kernel_command_line, - "tests/plugin/libmem.so,arg=both", plugin_log.name, + "tests/plugin/libmem.so,inline=true,callback=true", plugin_log.name, console_pattern, args=('-icount', 'shift=1')) From 86a41ac7fd488ce084175a30d1e5fd92dc37596b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Mon, 29 Nov 2021 14:09:28 +0000 Subject: [PATCH 4/8] plugins/meson.build: fix linker issue with weird paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Alex Bennée Tested-by: Stefan Weil Fixes: https://gitlab.com/qemu-project/qemu/-/issues/712 Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20211129140932.4115115-5-alex.bennee@linaro.org> --- plugins/meson.build | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/meson.build b/plugins/meson.build index aeb386ebae..b3de57853b 100644 --- a/plugins/meson.build +++ b/plugins/meson.build @@ -2,9 +2,9 @@ plugin_ldflags = [] # Modules need more symbols than just those in plugins/qemu-plugins.symbols if not enable_modules if 'CONFIG_HAS_LD_DYNAMIC_LIST' in config_host - plugin_ldflags = ['-Wl,--dynamic-list=' + (meson.project_build_root() / 'qemu-plugins-ld.symbols')] + plugin_ldflags = ['-Wl,--dynamic-list=qemu-plugins-ld.symbols'] elif 'CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST' in config_host - plugin_ldflags = ['-Wl,-exported_symbols_list,' + (meson.project_build_root() / 'qemu-plugins-ld64.symbols')] + plugin_ldflags = ['-Wl,-exported_symbols_list,qemu-plugins-ld64.symbols'] endif endif From a8e537fa4d61b29c5c0ab1395918ad63f7752b25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Mon, 29 Nov 2021 14:09:29 +0000 Subject: [PATCH 5/8] gdbstub: handle a potentially racing TaskState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When dealing with multi-threaded userspace programs there is a race condition with the addition of cpu->opaque (aka TaskState). This is due to cpu_copy calling cpu_create which updates the global vCPU list. However the task state isn't set until later. This shouldn't be a problem because the new thread can't have executed anything yet but the gdbstub code does liberally iterate through the CPU list in various places. This sticking plaster ensure the not yet fully realized vCPU is given an pid of -1 which should be enough to ensure it doesn't show up anywhere else. In the longer term I think the code that manages the association between vCPUs and attached GDB processes could do with a clean-up and re-factor. Signed-off-by: Alex Bennée Tested-by: Richard Henderson Reviewed-by: Richard Henderson Cc: Richard Henderson Resolves: https://gitlab.com/qemu-project/qemu/-/issues/730 Message-Id: <20211129140932.4115115-6-alex.bennee@linaro.org> --- gdbstub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdbstub.c b/gdbstub.c index 23baaef40e..141d7bc4ec 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -94,7 +94,7 @@ static inline int cpu_gdb_index(CPUState *cpu) { #if defined(CONFIG_USER_ONLY) TaskState *ts = (TaskState *) cpu->opaque; - return ts->ts_tid; + return ts ? ts->ts_tid : -1; #else return cpu->cpu_index + 1; #endif From 40525be5cbe768b23b427c6535c0b11bda80c851 Mon Sep 17 00:00:00 2001 From: Willian Rampazzo Date: Mon, 29 Nov 2021 14:09:30 +0000 Subject: [PATCH 6/8] MAINTAINERS: Remove me as a reviewer for the build and test/avocado MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove me as a reviewer for the Build and test automation and the Integration Testing with the Avocado Framework and add Beraldo Leal. Signed-off-by: Willian Rampazzo Reviewed-by: Beraldo Leal Message-Id: <20211122191124.31620-1-willianr@redhat.com> Signed-off-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20211129140932.4115115-7-alex.bennee@linaro.org> --- MAINTAINERS | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index d3879aa3c1..8f5156bfa7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3469,7 +3469,7 @@ M: Alex Bennée M: Philippe Mathieu-Daudé M: Thomas Huth R: Wainer dos Santos Moschetta -R: Willian Rampazzo +R: Beraldo Leal S: Maintained F: .github/lockdown.yml F: .gitlab-ci.yml @@ -3507,7 +3507,7 @@ W: https://trello.com/b/6Qi1pxVn/avocado-qemu R: Cleber Rosa R: Philippe Mathieu-Daudé R: Wainer dos Santos Moschetta -R: Willian Rampazzo +R: Beraldo Leal S: Odd Fixes F: tests/avocado/ From 1e970158be18ed1142a8ba996448113f90848aa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Mon, 29 Nov 2021 14:09:31 +0000 Subject: [PATCH 7/8] MAINTAINERS: Add section for Aarch64 GitLab custom runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a MAINTAINERS section to cover the GitLab YAML config file containing the jobs run on the custom runner sponsored by the Works On Arm project [*]. [*] https://developer.arm.com/solutions/infrastructure/works-on-arm Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Alex Bennée Message-Id: <20211116163226.2719320-1-f4bug@amsat.org> Message-Id: <20211129140932.4115115-8-alex.bennee@linaro.org> --- MAINTAINERS | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 8f5156bfa7..006a2293ba 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3511,6 +3511,12 @@ R: Beraldo Leal S: Odd Fixes F: tests/avocado/ +GitLab custom runner (Works On Arm Sponsored) +M: Alex Bennée +M: Philippe Mathieu-Daudé +S: Maintained +F: .gitlab-ci.d/custom-runners/ubuntu-20.04-aarch64.yml + Documentation ------------- Build system architecture From d5615bbf9103f01911df683cc3e4e85c49a92593 Mon Sep 17 00:00:00 2001 From: Juro Bystricky Date: Mon, 29 Nov 2021 14:09:32 +0000 Subject: [PATCH 8/8] tests/plugin/syscall.c: fix compiler warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix compiler warnings. The warnings can result in a broken build. This patch fixes warnings such as: In file included from /usr/include/glib-2.0/glib.h:111, from ../tests/plugin/syscall.c:13: ../tests/plugin/syscall.c: In function ‘print_entry’: /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: ‘out’ may be used uninitialized in this function [-Werror=maybe-uninitialized] g_free (*pp); ^~~~~~~~~~~~ ../tests/plugin/syscall.c:82:23: note: ‘out’ was declared here g_autofree gchar *out; ^~~ In file included from /usr/include/glib-2.0/glib.h:111, from ../tests/plugin/syscall.c:13: ../tests/plugin/syscall.c: In function ‘vcpu_syscall_ret’: /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: ‘out’ may be used uninitialized in this function [-Werror=maybe-uninitialized] g_free (*pp); ^~~~~~~~~~~~ ../tests/plugin/syscall.c:73:27: note: ‘out’ was declared here g_autofree gchar *out; ^~~ cc1: all warnings being treated as errors Signed-off-by: Juro Bystricky Signed-off-by: Alex Bennée Message-Id: <20211128011551.2115468-1-juro.bystricky@intel.com> Reviewed-by: Richard Henderson Message-Id: <20211129140932.4115115-9-alex.bennee@linaro.org> --- tests/plugin/syscall.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/plugin/syscall.c b/tests/plugin/syscall.c index 484b48de49..96040c578f 100644 --- a/tests/plugin/syscall.c +++ b/tests/plugin/syscall.c @@ -70,19 +70,17 @@ static void vcpu_syscall_ret(qemu_plugin_id_t id, unsigned int vcpu_idx, } g_mutex_unlock(&lock); } else { - g_autofree gchar *out; - out = g_strdup_printf("syscall #%" PRIi64 " returned -> %" PRIi64 "\n", - num, ret); + g_autofree gchar *out = g_strdup_printf( + "syscall #%" PRIi64 " returned -> %" PRIi64 "\n", num, ret); qemu_plugin_outs(out); } } static void print_entry(gpointer val, gpointer user_data) { - g_autofree gchar *out; SyscallStats *entry = (SyscallStats *) val; int64_t syscall_num = entry->num; - out = g_strdup_printf( + g_autofree gchar *out = g_strdup_printf( "%-13" PRIi64 "%-6" PRIi64 " %" PRIi64 "\n", syscall_num, entry->calls, entry->errors); qemu_plugin_outs(out);