| Message ID | 20221024132459.3229709-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: Fix x86 TARGET_TB_PCREL (#1269) | expand |
On 10/24/22 15:24, 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[]. > > 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 e5f8b224a5..a772e8cbdc 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)); Hi Richard, doesn't matter much since this is removed later on, but I wonder why the need for attribute weak here? I don't see you overloading this function in later patches.. Thanks, Claudio > > /** > * 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 4ed75a13e1..19cd23e9a0 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -329,7 +329,6 @@ static 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 > @@ -368,7 +367,20 @@ static 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, > @@ -380,6 +392,14 @@ static 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/25/22 01:05, Claudio Fontana wrote: > On 10/24/22 15:24, 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[]. >> >> 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 e5f8b224a5..a772e8cbdc 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)); > > Hi Richard, doesn't matter much since this is removed later on, but I wonder why the need for attribute weak here? > I don't see you overloading this function in later patches.. So that it can be undefined. Otherwise I can't remove the existing symbol from each target. r~
On 10/24/22 17:15, Richard Henderson wrote: > On 10/25/22 01:05, Claudio Fontana wrote: >> On 10/24/22 15:24, 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[]. >>> >>> 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 e5f8b224a5..a772e8cbdc 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)); >> >> Hi Richard, doesn't matter much since this is removed later on, but I wonder why the need for attribute weak here? >> I don't see you overloading this function in later patches.. > > So that it can be undefined. Otherwise I can't remove the existing symbol from each target. > > > r~ > > Right - there is still the call to restore_state_to_opc in the else branch in the general code. I wonder if checking for NULL would make sense in theory, I think that with both GCC and Clang the external declaration with attribute weak would make the function address evaluate to NULL, so that could be a possible thing to exploit, but no matter. Reviewed-by: Claudio Fontana <cfontana@suse.de>
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index e5f8b224a5..a772e8cbdc 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 4ed75a13e1..19cd23e9a0 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -329,7 +329,6 @@ static 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 @@ -368,7 +367,20 @@ static 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, @@ -380,6 +392,14 @@ static 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.
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[]. 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(-)