Re: [PATCH] target/arm: Remove unnecessary gen_io_end() calls
On 19.06.2020 20:03, Peter Maydell wrote:
Since commit ba3e7926691ed3 it has been unnecessary for target code
to call gen_io_end() after an IO instruction in icount mode; it is
sufficient to call gen_io_start() before it and to force the end of
the TB.
Many now-unnecessary calls to gen_io_end() were removed in commit
9e9b10c6491153b, but some were missed or accidentally added later.
Remove unneeded calls from the arm target:
* the call in the handling of exception-return-via-LDM is
unnecessary, and the code is already forcing end-of-TB
* the call in the VFP access check code is more complicated:
we weren't ending the TB, so we need to add the code to
force that by setting DISAS_UPDATE
* the doc comment for ARM_CP_IO doesn't need to mention
gen_io_end() any more
Signed-off-by: Peter Maydell
Reviewed-by: Pavel Dovgalyuk
---
target/arm/cpu.h | 2 +-
target/arm/translate-vfp.inc.c | 7 +++
target/arm/translate.c | 3 ---
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 677584e5da0..cf66b8c7fb0 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2334,7 +2334,7 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
* migration or KVM state synchronization. (Typically this is for "registers"
* which are actually used as instructions for cache maintenance and so on.)
* IO indicates that this register does I/O and therefore its accesses
- * need to be surrounded by gen_io_start()/gen_io_end(). In particular,
+ * need to be marked with gen_io_start() and also end the TB. In particular,
* registers which implement clocks or timers require this.
* RAISES_EXC is for when the read or write hook might raise an exception;
* the generated code will synchronize the CPU state before calling the hook
diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
index e1a90175983..bf31b186578 100644
--- a/target/arm/translate-vfp.inc.c
+++ b/target/arm/translate-vfp.inc.c
@@ -119,15 +119,14 @@ static bool full_vfp_access_check(DisasContext *s, bool
ignore_vfp_enabled)
if (s->v7m_lspact) {
/*
* Lazy state saving affects external memory and also the NVIC,
- * so we must mark it as an IO operation for icount.
+ * so we must mark it as an IO operation for icount (and cause
+ * this to be the last insn in the TB).
*/
if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
+s->base.is_jmp = DISAS_UPDATE;
gen_io_start();
}
gen_helper_v7m_preserve_fp_state(cpu_env);
-if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
-gen_io_end();
-}
/*
* If the preserve_fp_state helper doesn't throw an exception
* then it will clear LSPACT; we don't need to repeat this for
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6d18892adee..2677eaeb1e1 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8824,9 +8824,6 @@ static bool do_ldm(DisasContext *s, arg_ldst_block *a,
int min_n)
gen_io_start();
}
gen_helper_cpsr_write_eret(cpu_env, tmp);
-if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
-gen_io_end();
-}
tcg_temp_free_i32(tmp);
/* Must exit loop to check un-masked IRQs */
s->base.is_jmp = DISAS_EXIT;
Re: [PATCH] target/arm: Remove unnecessary gen_io_end() calls
On 6/19/20 10:03 AM, Peter Maydell wrote: > Since commit ba3e7926691ed3 it has been unnecessary for target code > to call gen_io_end() after an IO instruction in icount mode; it is > sufficient to call gen_io_start() before it and to force the end of > the TB. > > Many now-unnecessary calls to gen_io_end() were removed in commit > 9e9b10c6491153b, but some were missed or accidentally added later. > Remove unneeded calls from the arm target: > > * the call in the handling of exception-return-via-LDM is >unnecessary, and the code is already forcing end-of-TB > * the call in the VFP access check code is more complicated: >we weren't ending the TB, so we need to add the code to >force that by setting DISAS_UPDATE > * the doc comment for ARM_CP_IO doesn't need to mention >gen_io_end() any more > > Signed-off-by: Peter Maydell > --- > target/arm/cpu.h | 2 +- > target/arm/translate-vfp.inc.c | 7 +++ > target/arm/translate.c | 3 --- > 3 files changed, 4 insertions(+), 8 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH] target/arm: Remove unnecessary gen_io_end() calls
Patchew URL: https://patchew.org/QEMU/20200619170324.12093-1-peter.mayd...@linaro.org/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === CC qga/qapi-generated/qga-qapi-types.o CC qga/qapi-generated/qga-qapi-visit.o CC qga/qapi-generated/qga-qapi-commands.o /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) CC qga/qapi-generated/qga-qapi-init-commands.o AR libqemuutil.a LINKelf2dmp --- GEN docs/interop/qemu-ga-ref.html GEN docs/interop/qemu-ga-ref.txt GEN docs/interop/qemu-ga-ref.7 /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) LINKqemu-keymap /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) LINKivshmem-client LINKivshmem-server /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) LINKqemu-nbd LINKqemu-storage-daemon /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) LINKqemu-img AS pc-bios/optionrom/multiboot.o AS pc-bios/optionrom/linuxboot.o /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) CC pc-bios/optionrom/linuxboot_dma.o LINKqemu-io AS pc-bios/optionrom/kvmvapic.o /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) AS pc-bios/optionrom/pvh.o LINKqemu-edid CC pc-bios/optionrom/pvh_main.o --- BUILD pc-bios/optionrom/linuxboot.raw LINKfsdev/virtfs-proxy-helper BUILD pc-bios/optionrom/linuxboot_dma.raw /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) BUILD pc-bios/optionrom/kvmvapic.raw BUILD pc-bios/optionrom/pvh.img SIGNpc-bios/optionrom/multiboot.bin SIGNpc-bios/optionrom/linuxboot.bin SIGNpc-bios/optionrom/linuxboot_dma.bin /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) SIGNpc-bios/optionrom/kvmvapic.bin BUILD pc-bios/optionrom/pvh.raw LINKscsi/qemu-pr-helper SIGNpc-bios/optionrom/pvh.bin LINKqemu-bridge-helper /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) /usr/bin/ld:
Re: [PATCH] target/arm: Remove unnecessary gen_io_end() calls
Peter Maydell writes: > Since commit ba3e7926691ed3 it has been unnecessary for target code > to call gen_io_end() after an IO instruction in icount mode; it is > sufficient to call gen_io_start() before it and to force the end of > the TB. > > Many now-unnecessary calls to gen_io_end() were removed in commit > 9e9b10c6491153b, but some were missed or accidentally added later. > Remove unneeded calls from the arm target: > > * the call in the handling of exception-return-via-LDM is >unnecessary, and the code is already forcing end-of-TB > * the call in the VFP access check code is more complicated: >we weren't ending the TB, so we need to add the code to >force that by setting DISAS_UPDATE That was the thing I was staring at trying to puzzle out. I guess going forward we want to base the base DisasContext to gen_io_start so we can flip a bit and then assert we don't start a new instruction having already handled an IO instruction which should have been the last one. > * the doc comment for ARM_CP_IO doesn't need to mention >gen_io_end() any more > > Signed-off-by: Peter Maydell Reviewed-by: Alex Bennée -- Alex Bennée
