| Message ID | 20221026021116.1988449-26-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/47] Revert "accel/tcg: Init TCG cflags in vCPU thread handler" | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > Add a tcg_ops hook to replace the restore_state_to_opc > function call. Because these generic hooks cannot depend > on target-specific types, temporarily, copy the current > target_ulong data[] into uint64_t d64[]. > > Reviewed-by: Claudio Fontana <cfontana@suse.de> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> This has triggered a regression in x86_64 stuff: β make -j30 GIT ui/keycodemapdb tests/fp/berkeley-testfloat-3 tests/fp/berkeley-softfloat-3 dtc [1/9] Generating qemu-version.h with a custom command (wrapped by meson to capture output) π11:41:11 alex.bennee@hackbox2:qemu.git/builds/bisect on ξ HEAD (8269c01) (BISECTING) [$?] β ./tests/venv/bin/avocado run tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_i440fx_tcg tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_q35_tcg tests/avocado/linux_ initrd.py:LinuxInitrd.test_with_2gib_file_should_work_with_linux_v4_16 tests/avocado/replay_kernel.py:ReplayKernelNormal.test_x86_64_pc Fetching asset from tests/avocado/linux_initrd.py:LinuxInitrd.test_with_2gib_file_should_work_with_linux_v4_16 Fetching asset from tests/avocado/replay_kernel.py:ReplayKernelNormal.test_x86_64_pc JOB ID : 1d6ae71471e46c091ed06acc59a077c10b7b1ff9 JOB LOG : /home/alex.bennee/avocado/job-results/job-2022-10-29T11.41-1d6ae71/job.log (1/4) tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_i440fx_tcg: PASS (80.15 s) (2/4) tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_q35_tcg: PASS (69.03 s) (3/4) tests/avocado/linux_initrd.py:LinuxInitrd.test_with_2gib_file_should_work_with_linux_v4_16: PASS (14.37 s) (4/4) tests/avocado/replay_kernel.py:ReplayKernelNormal.test_x86_64_pc: PASS (71.81 s) RESULTS : PASS 4 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0 JOB TIME : 235.96 s π11:45:10 alex.bennee@hackbox2:qemu.git/builds/bisect on ξ HEAD (d292568) (BISECTING) [$?] took 3m56s β ninja build ninja: error: unknown target 'build' π11:45:21 alex.bennee@hackbox2:qemu.git/builds/bisect on ξ HEAD (d292568) (BISECTING) [$?] [π΄ ERROR] β ninja [56/56] Linking target qemu-system-x86_64 π11:45:29 alex.bennee@hackbox2:qemu.git/builds/bisect on ξ HEAD (d292568) (BISECTING) [$?] took 4s β ./tests/venv/bin/avocado run tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_i440fx_tcg tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_q35_tcg tests/avocado/linux_ initrd.py:LinuxInitrd.test_with_2gib_file_should_work_with_linux_v4_16 tests/avocado/replay_kernel.py:ReplayKernelNormal.test_x86_64_pc Fetching asset from tests/avocado/linux_initrd.py:LinuxInitrd.test_with_2gib_file_should_work_with_linux_v4_16 Fetching asset from tests/avocado/replay_kernel.py:ReplayKernelNormal.test_x86_64_pc JOB ID : a1c449facd31a2907520e6971a66a3a5529c3bd2 JOB LOG : /home/alex.bennee/avocado/job-results/job-2022-10-29T11.45-a1c449f/job.log (1/4) tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_i440fx_tcg: INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout reached\nOriginal status: ERRO R\n{'name': '1-tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_i440fx_tcg', 'logdir': '/home/alex.bennee/avocado/job-results/job-2022-10-29T11.45-a1c449f/test-result... (120.58 s) (2/4) tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_q35_tcg: INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout reached\nOriginal status: ERROR\n {'name': '2-tests/avocado/boot_linux.py:BootLinuxX8664.test_pc_q35_tcg', 'logdir': '/home/alex.bennee/avocado/job-results/job-2022-10-29T11.45-a1c449f/test-results/2... (120.59 s) (3/4) tests/avocado/linux_initrd.py:LinuxInitrd.test_with_2gib_file_should_work_with_linux_v4_16: INTERRUPTED: Test died without reporting the status.\nRunner error occurre d: Timeout reached\nOriginal status: ERROR\n{'name': '3-tests/avocado/linux_initrd.py:LinuxInitrd.test_with_2gib_file_should_work_with_linux_v4_16', 'logdir': '/home/alex.bennee/avocado/job-results... (311.46 s) (4/4) tests/avocado/replay_kernel.py:ReplayKernelNormal.test_x86_64_pc: INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout reached\nOriginal status: E RROR\n{'name': '4-tests/avocado/replay_kernel.py:ReplayKernelNormal.test_x86_64_pc', 'logdir': '/home/alex.bennee/avocado/job-results/job-2022-10-29T11.45-a1c449f/test-res... (120.58 s) RESULTS : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 4 | CANCEL 0 JOB TIME : 676.41 s π11:56:59 alex.bennee@hackbox2:qemu.git/builds/bisect on ξ HEAD (d292568) (BISECTING) [$?] took 11m28s [π΄ 8] β > --- > include/exec/exec-all.h | 2 +- > include/hw/core/tcg-cpu-ops.h | 11 +++++++++++ > accel/tcg/translate-all.c | 24 ++++++++++++++++++++++-- > 3 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 5ae484e34d..3b5e84240b 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -40,7 +40,7 @@ typedef ram_addr_t tb_page_addr_t; > #endif > > void restore_state_to_opc(CPUArchState *env, TranslationBlock *tb, > - target_ulong *data); > + target_ulong *data) __attribute__((weak)); > > /** > * cpu_restore_state: > diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h > index 78c6c6635d..20e3c0ffbb 100644 > --- a/include/hw/core/tcg-cpu-ops.h > +++ b/include/hw/core/tcg-cpu-ops.h > @@ -31,6 +31,17 @@ struct TCGCPUOps { > * function to restore all the state, and register it here. > */ > void (*synchronize_from_tb)(CPUState *cpu, const TranslationBlock *tb); > + /** > + * @restore_state_to_opc: Synchronize state from INDEX_op_start_insn > + * > + * This is called when we unwind state in the middle of a TB, > + * usually before raising an exception. Set all part of the CPU > + * state which are tracked insn-by-insn in the target-specific > + * arguments to start_insn, passed as @data. > + */ > + void (*restore_state_to_opc)(CPUState *cpu, const TranslationBlock *tb, > + const uint64_t *data); > + > /** @cpu_exec_enter: Callback for cpu_exec preparation */ > void (*cpu_exec_enter)(CPUState *cpu); > /** @cpu_exec_exit: Callback for cpu_exec cleanup */ > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > index 433fa247f4..4d8783efc7 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -256,7 +256,6 @@ int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, > { > target_ulong data[TARGET_INSN_START_WORDS]; > uintptr_t host_pc = (uintptr_t)tb->tc.ptr; > - CPUArchState *env = cpu->env_ptr; > const uint8_t *p = tb->tc.ptr + tb->tc.size; > int i, j, num_insns = tb->icount; > #ifdef CONFIG_PROFILER > @@ -295,7 +294,20 @@ int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, > and shift if to the number of actually executed instructions */ > cpu_neg(cpu)->icount_decr.u16.low += num_insns - i; > } > - restore_state_to_opc(env, tb, data); > + > + { > + const struct TCGCPUOps *ops = cpu->cc->tcg_ops; > + __typeof(ops->restore_state_to_opc) restore = ops->restore_state_to_opc; > + if (restore) { > + uint64_t d64[TARGET_INSN_START_WORDS]; > + for (i = 0; i < TARGET_INSN_START_WORDS; ++i) { > + d64[i] = data[i]; > + } > + restore(cpu, tb, d64); > + } else { > + restore_state_to_opc(cpu->env_ptr, tb, data); > + } > + } > > #ifdef CONFIG_PROFILER > qatomic_set(&prof->restore_time, > @@ -307,6 +319,14 @@ int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, > > bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit) > { > + /* > + * The pc update associated with restore without exit will > + * break the relative pc adjustments performed by TARGET_TB_PCREL. > + */ > + if (TARGET_TB_PCREL) { > + assert(will_exit); > + } > + > /* > * The host_pc has to be in the rx region of the code buffer. > * If it is not we will not be able to resolve it here.
On 10/29/22 21:42, Alex BennΓ©e wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> Add a tcg_ops hook to replace the restore_state_to_opc >> function call. Because these generic hooks cannot depend >> on target-specific types, temporarily, copy the current >> target_ulong data[] into uint64_t d64[]. >> >> Reviewed-by: Claudio Fontana <cfontana@suse.de> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > This has triggered a regression in x86_64 stuff: Patches posted: 20221027100254.215253-1-richard.henderson@linaro.org ("[PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269)") r~
On Wednesday, October 26, 2022 4:10:54 AM CET Richard Henderson wrote: > Add a tcg_ops hook to replace the restore_state_to_opc > function call. Because these generic hooks cannot depend > on target-specific types, temporarily, copy the current > target_ulong data[] into uint64_t d64[]. > > Reviewed-by: Claudio Fontana <cfontana@suse.de> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- [...] > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > index 433fa247f4..4d8783efc7 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c [...] > bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit) > { > + /* > + * The pc update associated with restore without exit will > + * break the relative pc adjustments performed by TARGET_TB_PCREL. > + */ > + if (TARGET_TB_PCREL) { > + assert(will_exit); > + } > + > /* > * The host_pc has to be in the rx region of the code buffer. > * If it is not we will not be able to resolve it here. This patch appears to break macOS host. This assertion always triggers on Apple Silicon. Previous patch 8269c01417 runs fine. Any ideas? BTW Richard, could you add a message-id tag to your queued TCG patches? If you are using patchwork client then it suffices to add "msgid=on" to .pwclientrc Best regards, Christian Schoenebeck
On 11/1/22 04:56, Christian Schoenebeck wrote: > On Wednesday, October 26, 2022 4:10:54 AM CET Richard Henderson wrote: >> Add a tcg_ops hook to replace the restore_state_to_opc >> function call. Because these generic hooks cannot depend >> on target-specific types, temporarily, copy the current >> target_ulong data[] into uint64_t d64[]. >> >> Reviewed-by: Claudio Fontana <cfontana@suse.de> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- > [...] >> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c >> index 433fa247f4..4d8783efc7 100644 >> --- a/accel/tcg/translate-all.c >> +++ b/accel/tcg/translate-all.c > [...] >> bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit) >> { >> + /* >> + * The pc update associated with restore without exit will >> + * break the relative pc adjustments performed by TARGET_TB_PCREL. >> + */ >> + if (TARGET_TB_PCREL) { >> + assert(will_exit); >> + } >> + >> /* >> * The host_pc has to be in the rx region of the code buffer. >> * If it is not we will not be able to resolve it here. > > This patch appears to break macOS host. This assertion always triggers on > Apple Silicon. Previous patch 8269c01417 runs fine. Any ideas? It does not previously work fine. See https://gitlab.com/qemu-project/qemu/-/issues/1269 which also contains a link to the in-flight patches. > BTW Richard, could you add a message-id tag to your queued TCG patches? Sometimes I remember, but I don't use the same tooling for my own work as I do for queuing other people's. I haven't found much value in it. > If you > are using patchwork client then it suffices to add "msgid=on" to .pwclientrc I am not. r~
On Mon, 31 Oct 2022 at 16:42, Richard Henderson <richard.henderson@linaro.org> wrote: > On 11/1/22 04:56, Christian Schoenebeck wrote: > > On Wednesday, October 26, 2022 4:10:54 AM CET Richard Henderson wrote: > > BTW Richard, could you add a message-id tag to your queued TCG patches? > > Sometimes I remember, but I don't use the same tooling for my own work as I do for queuing > other people's. I haven't found much value in it. > > > > If you > > are using patchwork client then it suffices to add "msgid=on" to .pwclientrc > > I am not. Tools that boil down to git-am(1) need to add the -m (--message-id) flag. Stefan
On 31/10/2022 20:53, Stefan Hajnoczi wrote: > On Mon, 31 Oct 2022 at 16:42, Richard Henderson > <richard.henderson@linaro.org> wrote: >> On 11/1/22 04:56, Christian Schoenebeck wrote: >>> On Wednesday, October 26, 2022 4:10:54 AM CET Richard Henderson wrote: >>> BTW Richard, could you add a message-id tag to your queued TCG patches? >> >> Sometimes I remember, but I don't use the same tooling for my own work as I do for queuing >> other people's. I haven't found much value in it. >> >> >>> If you >>> are using patchwork client then it suffices to add "msgid=on" to .pwclientrc >> >> I am not. > > Tools that boil down to git-am(1) need to add the -m (--message-id) flag. FWIW in my local QEMU git checkout I've run "git config am.messageid true" which alters .git/config so that the -m flag is enabled by default for "git am" commands. ATB, Mark.
On 11/1/22 08:27, Mark Cave-Ayland wrote: > On 31/10/2022 20:53, Stefan Hajnoczi wrote: > >> On Mon, 31 Oct 2022 at 16:42, Richard Henderson >> <richard.henderson@linaro.org> wrote: >>> On 11/1/22 04:56, Christian Schoenebeck wrote: >>>> On Wednesday, October 26, 2022 4:10:54 AM CET Richard Henderson wrote: >>>> BTW Richard, could you add a message-id tag to your queued TCG patches? >>> >>> Sometimes I remember, but I don't use the same tooling for my own work as I do for queuing >>> other people's.Β I haven't found much value in it. >>> >>> >>>> If you >>>> are using patchwork client then it suffices to add "msgid=on" to .pwclientrc >>> >>> I am not. >> >> Tools that boil down to git-am(1) need to add the -m (--message-id) flag. > > FWIW in my local QEMU git checkout I've run "git config am.messageid true" which alters > .git/config so that the -m flag is enabled by default for "git am" commands. This all supposes that one uses git am for ones own patches, which I do not. r~
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 5ae484e34d..3b5e84240b 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -40,7 +40,7 @@ typedef ram_addr_t tb_page_addr_t; #endif void restore_state_to_opc(CPUArchState *env, TranslationBlock *tb, - target_ulong *data); + target_ulong *data) __attribute__((weak)); /** * cpu_restore_state: diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h index 78c6c6635d..20e3c0ffbb 100644 --- a/include/hw/core/tcg-cpu-ops.h +++ b/include/hw/core/tcg-cpu-ops.h @@ -31,6 +31,17 @@ struct TCGCPUOps { * function to restore all the state, and register it here. */ void (*synchronize_from_tb)(CPUState *cpu, const TranslationBlock *tb); + /** + * @restore_state_to_opc: Synchronize state from INDEX_op_start_insn + * + * This is called when we unwind state in the middle of a TB, + * usually before raising an exception. Set all part of the CPU + * state which are tracked insn-by-insn in the target-specific + * arguments to start_insn, passed as @data. + */ + void (*restore_state_to_opc)(CPUState *cpu, const TranslationBlock *tb, + const uint64_t *data); + /** @cpu_exec_enter: Callback for cpu_exec preparation */ void (*cpu_exec_enter)(CPUState *cpu); /** @cpu_exec_exit: Callback for cpu_exec cleanup */ diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 433fa247f4..4d8783efc7 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -256,7 +256,6 @@ int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, { target_ulong data[TARGET_INSN_START_WORDS]; uintptr_t host_pc = (uintptr_t)tb->tc.ptr; - CPUArchState *env = cpu->env_ptr; const uint8_t *p = tb->tc.ptr + tb->tc.size; int i, j, num_insns = tb->icount; #ifdef CONFIG_PROFILER @@ -295,7 +294,20 @@ int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, and shift if to the number of actually executed instructions */ cpu_neg(cpu)->icount_decr.u16.low += num_insns - i; } - restore_state_to_opc(env, tb, data); + + { + const struct TCGCPUOps *ops = cpu->cc->tcg_ops; + __typeof(ops->restore_state_to_opc) restore = ops->restore_state_to_opc; + if (restore) { + uint64_t d64[TARGET_INSN_START_WORDS]; + for (i = 0; i < TARGET_INSN_START_WORDS; ++i) { + d64[i] = data[i]; + } + restore(cpu, tb, d64); + } else { + restore_state_to_opc(cpu->env_ptr, tb, data); + } + } #ifdef CONFIG_PROFILER qatomic_set(&prof->restore_time, @@ -307,6 +319,14 @@ int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit) { + /* + * The pc update associated with restore without exit will + * break the relative pc adjustments performed by TARGET_TB_PCREL. + */ + if (TARGET_TB_PCREL) { + assert(will_exit); + } + /* * The host_pc has to be in the rx region of the code buffer. * If it is not we will not be able to resolve it here.