From 638511e99207290c83d87326187e41f88fb92301 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Mon, 31 Jul 2023 11:17:27 +0100 Subject: [PATCH 1/6] target/arm: Fix MemOp for STGP When converting to decodetree, the code to rebuild mop for the pair only made it into trans_STP and not into trans_STGP. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1790 Fixes: 8c212eb6594 ("target/arm: Convert load/store-pair to decodetree") Signed-off-by: Richard Henderson Message-id: 20230726165416.309624-1-richard.henderson@linaro.org Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/tcg/translate-a64.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c index ef0c47407a..5fa1257d32 100644 --- a/target/arm/tcg/translate-a64.c +++ b/target/arm/tcg/translate-a64.c @@ -3004,6 +3004,9 @@ static bool trans_STGP(DisasContext *s, arg_ldstpair *a) MemOp mop; TCGv_i128 tmp; + /* STGP only comes in one size. */ + tcg_debug_assert(a->sz == MO_64); + if (!dc_isar_feature(aa64_mte_insn_reg, s)) { return false; } @@ -3029,13 +3032,25 @@ static bool trans_STGP(DisasContext *s, arg_ldstpair *a) gen_helper_stg(cpu_env, dirty_addr, dirty_addr); } - mop = finalize_memop(s, a->sz); - clean_addr = gen_mte_checkN(s, dirty_addr, true, false, 2 << a->sz, mop); + mop = finalize_memop(s, MO_64); + clean_addr = gen_mte_checkN(s, dirty_addr, true, false, 2 << MO_64, mop); tcg_rt = cpu_reg(s, a->rt); tcg_rt2 = cpu_reg(s, a->rt2); - assert(a->sz == 3); + /* + * STGP is defined as two 8-byte memory operations and one tag operation. + * We implement it as one single 16-byte memory operation for convenience. + * Rebuild mop as for STP. + * TODO: The atomicity with LSE2 is stronger than required. + * Need a form of MO_ATOM_WITHIN16_PAIR that never requires + * 16-byte atomicity. + */ + mop = MO_128; + if (s->align_mem) { + mop |= MO_ALIGN_8; + } + mop = finalize_memop_pair(s, mop); tmp = tcg_temp_new_i128(); if (s->be_data == MO_LE) { From 548b8edc6d9a5d6e1aab932f0ffcf43235c33a67 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sun, 11 Jun 2023 12:34:34 +0900 Subject: [PATCH 2/6] elf2dmp: Don't abandon when Prcb is set to 0 Prcb may be set to 0 for some CPUs if the dump was taken before they start. The dump may still contain valuable information for started CPUs so don't abandon conversion in such a case. Signed-off-by: Akihiko Odaki Reviewed-by: Viktor Prutyanov Message-id: 20230611033434.14659-1-akihiko.odaki@daynix.com Signed-off-by: Peter Maydell --- contrib/elf2dmp/main.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c index 89f0c69ab0..6d4d18501a 100644 --- a/contrib/elf2dmp/main.c +++ b/contrib/elf2dmp/main.c @@ -316,6 +316,11 @@ static int fill_context(KDDEBUGGER_DATA64 *kdbg, return 1; } + if (!Prcb) { + eprintf("Context for CPU #%d is missing\n", i); + continue; + } + if (va_space_rw(vs, Prcb + kdbg->OffsetPrcbContext, &Context, sizeof(Context), 0)) { eprintf("Failed to read CPU #%d ContextFrame location\n", i); From 2b0d656ab6484cae7f174e194215a6d50343ecd2 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Thu, 27 Jul 2023 11:39:06 +0100 Subject: [PATCH 3/6] target/arm: Avoid writing to constant TCGv in trans_CSEL() In commit 0b188ea05acb5 we changed the implementation of trans_CSEL() to use tcg_constant_i32(). However, this change was incorrect, because the implementation of the function sets up the TCGv_i32 rn and rm to be either zero or else a TCG temp created in load_reg(), and these TCG temps are then in both cases written to by the emitted TCG ops. The result is that we hit a TCG assertion: qemu-system-arm: ../../tcg/tcg.c:4455: tcg_reg_alloc_mov: Assertion `!temp_readonly(ots)' failed. (or on a non-debug build, just produce a garbage result) Adjust the code so that rn and rm are always writeable temporaries whether the instruction is using the special case "0" or a normal register as input. Cc: qemu-stable@nongnu.org Fixes: 0b188ea05acb5 ("target/arm: Use tcg_constant in trans_CSEL") Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20230727103906.2641264-1-peter.maydell@linaro.org --- target/arm/tcg/translate.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c index 13c88ba1b9..b71ac2d0d5 100644 --- a/target/arm/tcg/translate.c +++ b/target/arm/tcg/translate.c @@ -8799,7 +8799,7 @@ static bool trans_IT(DisasContext *s, arg_IT *a) /* v8.1M CSEL/CSINC/CSNEG/CSINV */ static bool trans_CSEL(DisasContext *s, arg_CSEL *a) { - TCGv_i32 rn, rm, zero; + TCGv_i32 rn, rm; DisasCompare c; if (!arm_dc_feature(s, ARM_FEATURE_V8_1M)) { @@ -8817,16 +8817,17 @@ static bool trans_CSEL(DisasContext *s, arg_CSEL *a) } /* In this insn input reg fields of 0b1111 mean "zero", not "PC" */ - zero = tcg_constant_i32(0); + rn = tcg_temp_new_i32(); + rm = tcg_temp_new_i32(); if (a->rn == 15) { - rn = zero; + tcg_gen_movi_i32(rn, 0); } else { - rn = load_reg(s, a->rn); + load_reg_var(s, rn, a->rn); } if (a->rm == 15) { - rm = zero; + tcg_gen_movi_i32(rm, 0); } else { - rm = load_reg(s, a->rm); + load_reg_var(s, rm, a->rm); } switch (a->op) { @@ -8846,7 +8847,7 @@ static bool trans_CSEL(DisasContext *s, arg_CSEL *a) } arm_test_cc(&c, a->fcond); - tcg_gen_movcond_i32(c.cond, rn, c.value, zero, rn, rm); + tcg_gen_movcond_i32(c.cond, rn, c.value, tcg_constant_i32(0), rn, rm); store_reg(s, a->rd, rn); return true; From 71054f72f14e7a62b6e623997404259d52ea43fb Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 18 Jul 2023 11:46:28 +0100 Subject: [PATCH 4/6] target/arm/tcg: Don't build AArch64 decodetree files for qemu-system-arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we list all the Arm decodetree files together and add them unconditionally to arm_ss. This means we build them for both qemu-system-aarch64 and qemu-system-arm. However, some of them are AArch64-specific, so there is no need to build them for qemu-system-arm. (Meson is smart enough to notice that the generated .c.inc file is not used by any objects that go into qemu-system-arm, so we only unnecessarily run decodetree, not anything more heavyweight like a recompile or relink, but it's still unnecessary work.) Split gen into gen_a32 and gen_a64, and only add gen_a64 for TARGET_AARCH64 compiles. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Message-id: 20230718104628.1137734-1-peter.maydell@linaro.org --- target/arm/tcg/meson.build | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build index bdcab56489..6fca38f2cc 100644 --- a/target/arm/tcg/meson.build +++ b/target/arm/tcg/meson.build @@ -1,7 +1,11 @@ -gen = [ +gen_a64 = [ + decodetree.process('a64.decode', extra_args: ['--static-decode=disas_a64']), decodetree.process('sve.decode', extra_args: '--decode=disas_sve'), decodetree.process('sme.decode', extra_args: '--decode=disas_sme'), decodetree.process('sme-fa64.decode', extra_args: '--static-decode=disas_sme_fa64'), +] + +gen_a32 = [ decodetree.process('neon-shared.decode', extra_args: '--decode=disas_neon_shared'), decodetree.process('neon-dp.decode', extra_args: '--decode=disas_neon_dp'), decodetree.process('neon-ls.decode', extra_args: '--decode=disas_neon_ls'), @@ -13,10 +17,10 @@ gen = [ decodetree.process('a32-uncond.decode', extra_args: '--static-decode=disas_a32_uncond'), decodetree.process('t32.decode', extra_args: '--static-decode=disas_t32'), decodetree.process('t16.decode', extra_args: ['-w', '16', '--static-decode=disas_t16']), - decodetree.process('a64.decode', extra_args: ['--static-decode=disas_a64']), ] -arm_ss.add(gen) +arm_ss.add(gen_a32) +arm_ss.add(when: 'TARGET_AARCH64', if_true: gen_a64) arm_ss.add(files( 'cpu32.c', From fe6bda58e083ec8ffa5c5166e3b1055501b6318a Mon Sep 17 00:00:00 2001 From: Gavin Shan Date: Mon, 31 Jul 2023 22:59:46 +1000 Subject: [PATCH 5/6] kvm: Fix crash due to access uninitialized kvm_state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Runs into core dump on arm64 and the backtrace extracted from the core dump is shown as below. It's caused by accessing uninitialized @kvm_state in kvm_flush_coalesced_mmio_buffer() due to commit 176d073029 ("hw/arm/virt: Use machine_memory_devices_init()"), where the machine's memory region is added earlier than before. main qemu_init configure_accelerators qemu_opts_foreach do_configure_accelerator accel_init_machine kvm_init virt_kvm_type virt_set_memmap machine_memory_devices_init memory_region_add_subregion memory_region_add_subregion_common memory_region_update_container_subregions memory_region_transaction_begin qemu_flush_coalesced_mmio_buffer kvm_flush_coalesced_mmio_buffer Fix it by bailing early in kvm_flush_coalesced_mmio_buffer() on the uninitialized @kvm_state. With this applied, no crash is observed on arm64. Fixes: 176d073029 ("hw/arm/virt: Use machine_memory_devices_init()") Signed-off-by: Gavin Shan Reviewed-by: David Hildenbrand Reviewed-by: Philippe Mathieu-Daudé Message-id: 20230731125946.2038742-1-gshan@redhat.com Signed-off-by: Peter Maydell --- accel/kvm/kvm-all.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 373d876c05..7b3da8dc3a 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2812,7 +2812,7 @@ void kvm_flush_coalesced_mmio_buffer(void) { KVMState *s = kvm_state; - if (s->coalesced_flush_in_progress) { + if (!s || s->coalesced_flush_in_progress) { return; } From 108e8180c6b0c315711aa54e914030a313505c17 Mon Sep 17 00:00:00 2001 From: Nicholas Piggin Date: Tue, 11 Jul 2023 18:59:03 +1000 Subject: [PATCH 6/6] gdbstub: Fix client Ctrl-C handling The gdb remote protocol has a special interrupt character (0x03) that is transmitted outside the regular packet processing, and represents a Ctrl-C pressed in the client. Despite not being a regular packet, it does expect a regular stop response if the stub successfully stops the running program. See: https://sourceware.org/gdb/onlinedocs/gdb/Interrupts.html Inhibiting the stop reply packet can lead to gdb client hang. So permit a stop response when receiving a character from gdb that stops the vm. Additionally, add a warning if that was not a 0x03 character, because the gdb session is likely to end up getting confused if this happens. Cc: qemu-stable@nongnu.org Fixes: 758370052fb ("gdbstub: only send stop-reply packets when allowed to") Reported-by: Frederic Barrat Signed-off-by: Nicholas Piggin Tested-by: Joel Stanley Message-id: 20230711085903.304496-1-npiggin@gmail.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- gdbstub/gdbstub.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c index 6911b73c07..ce8b42eb15 100644 --- a/gdbstub/gdbstub.c +++ b/gdbstub/gdbstub.c @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch) return; } if (runstate_is_running()) { - /* when the CPU is running, we cannot do anything except stop - it when receiving a char */ + /* + * When the CPU is running, we cannot do anything except stop + * it when receiving a char. This is expected on a Ctrl-C in the + * gdb client. Because we are in all-stop mode, gdb sends a + * 0x03 byte which is not a usual packet, so we handle it specially + * here, but it does expect a stop reply. + */ + if (ch != 0x03) { + warn_report("gdbstub: client sent packet while target running\n"); + } + gdbserver_state.allow_stop_reply = true; vm_stop(RUN_STATE_PAUSED); } else #endif