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>
---
 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.
-- 
2.34.1

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~

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.


-- 
Alex Bennée

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~