* [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269)
@ 2022-10-27 10:02 Richard Henderson
2022-10-27 10:02 ` [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data Richard Henderson
` (6 more replies)
0 siblings, 7 replies; 17+ messages in thread
From: Richard Henderson @ 2022-10-27 10:02 UTC (permalink / raw)
To: qemu-devel; +Cc: cfontana
As per #1269, this affects NetBSD installer boot.
The problem is that one of the x86 acpi callbacks modifies
env->eip during an mmio store, which means that the tracking
that translate.c does is thrown out of whack.
Introduce a method to extract unwind data without the
writeback to env. This isn't a perfect abstraction, but I
couldn't think of anything better. There's a couple of lines
of code duplication, but probably less than any abstration
that we might put on top
Changes for v2:
* Rebase on master, 23 patches merged.
* Comments adjusted per review (claudio)
r~
Richard Henderson (6):
accel/tcg: Introduce cpu_unwind_state_data
target/i386: Use cpu_unwind_state_data for tpr access
target/openrisc: Always exit after mtspr npc
target/openrisc: Use cpu_unwind_state_data for mfspr
accel/tcg: Remove will_exit argument from cpu_restore_state
accel/tcg: Remove reset_icount argument from cpu_restore_state_from_tb
accel/tcg/internal.h | 4 +-
include/exec/exec-all.h | 24 +++++---
accel/tcg/cpu-exec-common.c | 2 +-
accel/tcg/tb-maint.c | 4 +-
accel/tcg/translate-all.c | 91 +++++++++++++++++------------
target/alpha/helper.c | 2 +-
target/alpha/mem_helper.c | 2 +-
target/arm/op_helper.c | 2 +-
target/arm/tlb_helper.c | 8 +--
target/cris/helper.c | 2 +-
target/i386/helper.c | 21 ++++++-
target/i386/tcg/sysemu/svm_helper.c | 2 +-
target/m68k/op_helper.c | 4 +-
target/microblaze/helper.c | 2 +-
target/nios2/op_helper.c | 2 +-
target/openrisc/sys_helper.c | 17 ++++--
target/ppc/excp_helper.c | 2 +-
target/s390x/tcg/excp_helper.c | 2 +-
target/tricore/op_helper.c | 2 +-
target/xtensa/helper.c | 6 +-
20 files changed, 125 insertions(+), 76 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data
2022-10-27 10:02 [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
@ 2022-10-27 10:02 ` Richard Henderson
2022-10-27 10:40 ` Claudio Fontana
2022-10-27 10:02 ` [PATCH v2 2/6] target/i386: Use cpu_unwind_state_data for tpr access Richard Henderson
` (5 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2022-10-27 10:02 UTC (permalink / raw)
To: qemu-devel; +Cc: cfontana
Add a way to examine the unwind data without actually
restoring the data back into env.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/internal.h | 4 +--
include/exec/exec-all.h | 21 ++++++++---
accel/tcg/translate-all.c | 74 ++++++++++++++++++++++++++-------------
3 files changed, 68 insertions(+), 31 deletions(-)
diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
index 1227bb69bd..9c06b320b7 100644
--- a/accel/tcg/internal.h
+++ b/accel/tcg/internal.h
@@ -106,8 +106,8 @@ void tb_reset_jump(TranslationBlock *tb, int n);
TranslationBlock *tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
tb_page_addr_t phys_page2);
bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc);
-int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
- uintptr_t searched_pc, bool reset_icount);
+void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
+ uintptr_t host_pc, bool reset_icount);
/* Return the current PC from CPU, which may be cached in TB. */
static inline target_ulong log_pc(CPUState *cpu, const TranslationBlock *tb)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index e948992a80..7d851f5907 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -39,20 +39,33 @@ typedef ram_addr_t tb_page_addr_t;
#define TB_PAGE_ADDR_FMT RAM_ADDR_FMT
#endif
+/**
+ * cpu_unwind_state_data:
+ * @cpu: the cpu context
+ * @host_pc: the host pc within the translation
+ * @data: output data
+ *
+ * Attempt to load the the unwind state for a host pc occurring in
+ * translated code. If @host_pc is not in translated code, the
+ * function returns false; otherwise @data is loaded.
+ * This is the same unwind info as given to restore_state_to_opc.
+ */
+bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data);
+
/**
* cpu_restore_state:
- * @cpu: the vCPU state is to be restore to
- * @searched_pc: the host PC the fault occurred at
+ * @cpu: the cpu context
+ * @host_pc: the host pc within the translation
* @will_exit: true if the TB executed will be interrupted after some
cpu adjustments. Required for maintaining the correct
icount valus
* @return: true if state was restored, false otherwise
*
* Attempt to restore the state for a fault occurring in translated
- * code. If the searched_pc is not in translated code no state is
+ * code. If @host_pc is not in translated code no state is
* restored and the function returns false.
*/
-bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
+bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit);
G_NORETURN void cpu_loop_exit_noexc(CPUState *cpu);
G_NORETURN void cpu_loop_exit(CPUState *cpu);
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index f185356a36..319becb698 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -247,52 +247,66 @@ static int encode_search(TranslationBlock *tb, uint8_t *block)
return p - block;
}
-/* The cpu state corresponding to 'searched_pc' is restored.
- * When reset_icount is true, current TB will be interrupted and
- * icount should be recalculated.
- */
-int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
- uintptr_t searched_pc, bool reset_icount)
+static int cpu_unwind_data_from_tb(TranslationBlock *tb, uintptr_t host_pc,
+ uint64_t *data)
{
- uint64_t data[TARGET_INSN_START_WORDS];
- uintptr_t host_pc = (uintptr_t)tb->tc.ptr;
+ uintptr_t iter_pc = (uintptr_t)tb->tc.ptr;
const uint8_t *p = tb->tc.ptr + tb->tc.size;
int i, j, num_insns = tb->icount;
-#ifdef CONFIG_PROFILER
- TCGProfile *prof = &tcg_ctx->prof;
- int64_t ti = profile_getclock();
-#endif
- searched_pc -= GETPC_ADJ;
+ host_pc -= GETPC_ADJ;
- if (searched_pc < host_pc) {
+ if (host_pc < iter_pc) {
return -1;
}
- memset(data, 0, sizeof(data));
+ memset(data, 0, sizeof(uint64_t) * TARGET_INSN_START_WORDS);
if (!TARGET_TB_PCREL) {
data[0] = tb_pc(tb);
}
- /* Reconstruct the stored insn data while looking for the point at
- which the end of the insn exceeds the searched_pc. */
+ /*
+ * Reconstruct the stored insn data while looking for the point
+ * at which the end of the insn exceeds host_pc.
+ */
for (i = 0; i < num_insns; ++i) {
for (j = 0; j < TARGET_INSN_START_WORDS; ++j) {
data[j] += decode_sleb128(&p);
}
- host_pc += decode_sleb128(&p);
- if (host_pc > searched_pc) {
- goto found;
+ iter_pc += decode_sleb128(&p);
+ if (iter_pc > host_pc) {
+ return num_insns - i;
}
}
return -1;
+}
+
+/*
+ * The cpu state corresponding to 'host_pc' is restored.
+ * When reset_icount is true, current TB will be interrupted and
+ * icount should be recalculated.
+ */
+void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
+ uintptr_t host_pc, bool reset_icount)
+{
+ uint64_t data[TARGET_INSN_START_WORDS];
+#ifdef CONFIG_PROFILER
+ TCGProfile *prof = &tcg_ctx->prof;
+ int64_t ti = profile_getclock();
+#endif
+ int insns_left = cpu_unwind_data_from_tb(tb, host_pc, data);
+
+ if (insns_left < 0) {
+ return;
+ }
- found:
if (reset_icount && (tb_cflags(tb) & CF_USE_ICOUNT)) {
assert(icount_enabled());
- /* Reset the cycle counter to the start of the block
- and shift if to the number of actually executed instructions */
- cpu_neg(cpu)->icount_decr.u16.low += num_insns - i;
+ /*
+ * Reset the cycle counter to the start of the block and
+ * shift if to the number of actually executed instructions.
+ */
+ cpu_neg(cpu)->icount_decr.u16.low += insns_left;
}
cpu->cc->tcg_ops->restore_state_to_opc(cpu, tb, data);
@@ -302,7 +316,6 @@ int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
prof->restore_time + profile_getclock() - ti);
qatomic_set(&prof->restore_count, prof->restore_count + 1);
#endif
- return 0;
}
bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
@@ -335,6 +348,17 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
return false;
}
+bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data)
+{
+ if (in_code_gen_buffer((const void *)(host_pc - tcg_splitwx_diff))) {
+ TranslationBlock *tb = tcg_tb_lookup(host_pc);
+ if (tb) {
+ return cpu_unwind_data_from_tb(tb, host_pc, data) >= 0;
+ }
+ }
+ return false;
+}
+
void page_init(void)
{
page_size_init();
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/6] target/i386: Use cpu_unwind_state_data for tpr access
2022-10-27 10:02 [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
2022-10-27 10:02 ` [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data Richard Henderson
@ 2022-10-27 10:02 ` Richard Henderson
2022-10-27 12:22 ` Claudio Fontana
2022-10-27 10:02 ` [PATCH v2 3/6] target/openrisc: Always exit after mtspr npc Richard Henderson
` (4 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2022-10-27 10:02 UTC (permalink / raw)
To: qemu-devel; +Cc: cfontana
Avoid cpu_restore_state, and modifying env->eip out from
underneath the translator with TARGET_TB_PCREL. There is
some slight duplication from x86_restore_state_to_opc,
but it's just a few lines.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1269
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/i386/helper.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/target/i386/helper.c b/target/i386/helper.c
index b62a1e48e2..2cd1756f1a 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -509,6 +509,23 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank,
}
}
+static target_ulong get_memio_eip(CPUX86State *env)
+{
+ uint64_t data[TARGET_INSN_START_WORDS];
+ CPUState *cs = env_cpu(env);
+
+ if (!cpu_unwind_state_data(cs, cs->mem_io_pc, data)) {
+ return env->eip;
+ }
+
+ /* Per x86_restore_state_to_opc. */
+ if (TARGET_TB_PCREL) {
+ return (env->eip & TARGET_PAGE_MASK) | data[0];
+ } else {
+ return data[0] - env->segs[R_CS].base;
+ }
+}
+
void cpu_report_tpr_access(CPUX86State *env, TPRAccess access)
{
X86CPU *cpu = env_archcpu(env);
@@ -519,9 +536,9 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access)
cpu_interrupt(cs, CPU_INTERRUPT_TPR);
} else if (tcg_enabled()) {
- cpu_restore_state(cs, cs->mem_io_pc, false);
+ target_ulong eip = get_memio_eip(env);
- apic_handle_tpr_access_report(cpu->apic_state, env->eip, access);
+ apic_handle_tpr_access_report(cpu->apic_state, eip, access);
}
}
#endif /* !CONFIG_USER_ONLY */
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/6] target/openrisc: Always exit after mtspr npc
2022-10-27 10:02 [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
2022-10-27 10:02 ` [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data Richard Henderson
2022-10-27 10:02 ` [PATCH v2 2/6] target/i386: Use cpu_unwind_state_data for tpr access Richard Henderson
@ 2022-10-27 10:02 ` Richard Henderson
2022-10-27 10:49 ` Philippe Mathieu-Daudé
2022-10-27 10:02 ` [PATCH v2 4/6] target/openrisc: Use cpu_unwind_state_data for mfspr Richard Henderson
` (3 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2022-10-27 10:02 UTC (permalink / raw)
To: qemu-devel; +Cc: cfontana
We have called cpu_restore_state asserting will_exit.
Do not go back on that promise. This affects icount.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/openrisc/sys_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
index 09b3c97d7c..a3508e421d 100644
--- a/target/openrisc/sys_helper.c
+++ b/target/openrisc/sys_helper.c
@@ -51,8 +51,8 @@ void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong spr, target_ulong rb)
if (env->pc != rb) {
env->pc = rb;
env->dflag = 0;
- cpu_loop_exit(cs);
}
+ cpu_loop_exit(cs);
break;
case TO_SPR(0, 17): /* SR */
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 4/6] target/openrisc: Use cpu_unwind_state_data for mfspr
2022-10-27 10:02 [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
` (2 preceding siblings ...)
2022-10-27 10:02 ` [PATCH v2 3/6] target/openrisc: Always exit after mtspr npc Richard Henderson
@ 2022-10-27 10:02 ` Richard Henderson
2022-10-28 9:16 ` Claudio Fontana
2022-10-27 10:02 ` [PATCH v2 5/6] accel/tcg: Remove will_exit argument from cpu_restore_state Richard Henderson
` (2 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2022-10-27 10:02 UTC (permalink / raw)
To: qemu-devel; +Cc: cfontana
Since we do not plan to exit, use cpu_unwind_state_data
and extract exactly the data requested.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/openrisc/sys_helper.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
index a3508e421d..dde2fa1623 100644
--- a/target/openrisc/sys_helper.c
+++ b/target/openrisc/sys_helper.c
@@ -199,6 +199,7 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
target_ulong spr)
{
#ifndef CONFIG_USER_ONLY
+ uint64_t data[TARGET_INSN_START_WORDS];
MachineState *ms = MACHINE(qdev_get_machine());
OpenRISCCPU *cpu = env_archcpu(env);
CPUState *cs = env_cpu(env);
@@ -232,14 +233,20 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
return env->evbar;
case TO_SPR(0, 16): /* NPC (equals PC) */
- cpu_restore_state(cs, GETPC(), false);
+ if (cpu_unwind_state_data(cs, GETPC(), data)) {
+ return data[0];
+ }
return env->pc;
case TO_SPR(0, 17): /* SR */
return cpu_get_sr(env);
case TO_SPR(0, 18): /* PPC */
- cpu_restore_state(cs, GETPC(), false);
+ if (cpu_unwind_state_data(cs, GETPC(), data)) {
+ if (data[1] & 2) {
+ return data[0] - 4;
+ }
+ }
return env->ppc;
case TO_SPR(0, 32): /* EPCR */
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 5/6] accel/tcg: Remove will_exit argument from cpu_restore_state
2022-10-27 10:02 [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
` (3 preceding siblings ...)
2022-10-27 10:02 ` [PATCH v2 4/6] target/openrisc: Use cpu_unwind_state_data for mfspr Richard Henderson
@ 2022-10-27 10:02 ` Richard Henderson
2022-10-27 10:02 ` [PATCH v2 6/6] accel/tcg: Remove reset_icount argument from cpu_restore_state_from_tb Richard Henderson
2022-10-31 0:40 ` [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
6 siblings, 0 replies; 17+ messages in thread
From: Richard Henderson @ 2022-10-27 10:02 UTC (permalink / raw)
To: qemu-devel; +Cc: cfontana
The value passed is always true, and if the target's
synchronize_from_tb hook is non-trivial, not exiting
may be erroneous.
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/exec/exec-all.h | 5 +----
accel/tcg/cpu-exec-common.c | 2 +-
accel/tcg/translate-all.c | 12 ++----------
target/alpha/helper.c | 2 +-
target/alpha/mem_helper.c | 2 +-
target/arm/op_helper.c | 2 +-
target/arm/tlb_helper.c | 8 ++++----
target/cris/helper.c | 2 +-
target/i386/tcg/sysemu/svm_helper.c | 2 +-
target/m68k/op_helper.c | 4 ++--
target/microblaze/helper.c | 2 +-
target/nios2/op_helper.c | 2 +-
target/openrisc/sys_helper.c | 4 ++--
target/ppc/excp_helper.c | 2 +-
target/s390x/tcg/excp_helper.c | 2 +-
target/tricore/op_helper.c | 2 +-
target/xtensa/helper.c | 6 +++---
17 files changed, 25 insertions(+), 36 deletions(-)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 7d851f5907..9b7bfbf09a 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -56,16 +56,13 @@ bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data);
* cpu_restore_state:
* @cpu: the cpu context
* @host_pc: the host pc within the translation
- * @will_exit: true if the TB executed will be interrupted after some
- cpu adjustments. Required for maintaining the correct
- icount valus
* @return: true if state was restored, false otherwise
*
* Attempt to restore the state for a fault occurring in translated
* code. If @host_pc is not in translated code no state is
* restored and the function returns false.
*/
-bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit);
+bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc);
G_NORETURN void cpu_loop_exit_noexc(CPUState *cpu);
G_NORETURN void cpu_loop_exit(CPUState *cpu);
diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
index be6fe45aa5..c7bc8c6efa 100644
--- a/accel/tcg/cpu-exec-common.c
+++ b/accel/tcg/cpu-exec-common.c
@@ -71,7 +71,7 @@ void cpu_loop_exit(CPUState *cpu)
void cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc)
{
if (pc) {
- cpu_restore_state(cpu, pc, true);
+ cpu_restore_state(cpu, pc);
}
cpu_loop_exit(cpu);
}
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 319becb698..90997fed47 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -318,16 +318,8 @@ void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
#endif
}
-bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
+bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc)
{
- /*
- * 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.
@@ -341,7 +333,7 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
if (in_code_gen_buffer((const void *)(host_pc - tcg_splitwx_diff))) {
TranslationBlock *tb = tcg_tb_lookup(host_pc);
if (tb) {
- cpu_restore_state_from_tb(cpu, tb, host_pc, will_exit);
+ cpu_restore_state_from_tb(cpu, tb, host_pc, true);
return true;
}
}
diff --git a/target/alpha/helper.c b/target/alpha/helper.c
index a5a389b5a3..970c869771 100644
--- a/target/alpha/helper.c
+++ b/target/alpha/helper.c
@@ -532,7 +532,7 @@ G_NORETURN void dynamic_excp(CPUAlphaState *env, uintptr_t retaddr,
cs->exception_index = excp;
env->error_code = error;
if (retaddr) {
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
/* Floating-point exceptions (our only users) point to the next PC. */
env->pc += 4;
}
diff --git a/target/alpha/mem_helper.c b/target/alpha/mem_helper.c
index 47283a0612..a39b52c5dd 100644
--- a/target/alpha/mem_helper.c
+++ b/target/alpha/mem_helper.c
@@ -28,7 +28,7 @@ static void do_unaligned_access(CPUAlphaState *env, vaddr addr, uintptr_t retadd
uint64_t pc;
uint32_t insn;
- cpu_restore_state(env_cpu(env), retaddr, true);
+ cpu_restore_state(env_cpu(env), retaddr);
pc = env->pc;
insn = cpu_ldl_code(env, pc);
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index c5bde1cfcc..70672bcd9f 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -78,7 +78,7 @@ void raise_exception_ra(CPUARMState *env, uint32_t excp, uint32_t syndrome,
* we must restore CPU state here before setting the syndrome
* the caller passed us, and cannot use cpu_loop_exit_restore().
*/
- cpu_restore_state(cs, ra, true);
+ cpu_restore_state(cs, ra);
raise_exception(env, excp, syndrome, target_el);
}
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index 69b0dc69df..0f4f4fc809 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -156,7 +156,7 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
ARMMMUFaultInfo fi = {};
/* now we have a real cpu fault */
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
fi.type = ARMFault_Alignment;
arm_deliver_fault(cpu, vaddr, access_type, mmu_idx, &fi);
@@ -196,7 +196,7 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
ARMMMUFaultInfo fi = {};
/* now we have a real cpu fault */
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
fi.ea = arm_extabort_type(response);
fi.type = ARMFault_SyncExternal;
@@ -252,7 +252,7 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
return false;
} else {
/* now we have a real cpu fault */
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
arm_deliver_fault(cpu, address, access_type, mmu_idx, fi);
}
}
@@ -271,7 +271,7 @@ void arm_cpu_record_sigsegv(CPUState *cs, vaddr addr,
* We report both ESR and FAR to signal handlers.
* For now, it's easiest to deliver the fault normally.
*/
- cpu_restore_state(cs, ra, true);
+ cpu_restore_state(cs, ra);
arm_deliver_fault(cpu, addr, access_type, MMU_USER_IDX, &fi);
}
diff --git a/target/cris/helper.c b/target/cris/helper.c
index 91e4aeb178..81a72699b5 100644
--- a/target/cris/helper.c
+++ b/target/cris/helper.c
@@ -87,7 +87,7 @@ bool cris_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
cs->exception_index = EXCP_BUSFAULT;
env->fault_vector = res.bf_vec;
if (retaddr) {
- if (cpu_restore_state(cs, retaddr, true)) {
+ if (cpu_restore_state(cs, retaddr)) {
/* Evaluate flags after retranslation. */
helper_top_evaluate_flags(env);
}
diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index 8e88567399..2d27731b60 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -704,7 +704,7 @@ void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1,
{
CPUState *cs = env_cpu(env);
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
qemu_log_mask(CPU_LOG_TB_IN_ASM, "vmexit(%08x, %016" PRIx64 ", %016"
PRIx64 ", " TARGET_FMT_lx ")!\n",
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 5da176d642..1ce850bbc5 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -460,7 +460,7 @@ void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
M68kCPU *cpu = M68K_CPU(cs);
CPUM68KState *env = &cpu->env;
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
if (m68k_feature(env, M68K_FEATURE_M68040)) {
env->mmu.mmusr = 0;
@@ -558,7 +558,7 @@ raise_exception_format2(CPUM68KState *env, int tt, int ilen, uintptr_t raddr)
cs->exception_index = tt;
/* Recover PC and CC_OP for the beginning of the insn. */
- cpu_restore_state(cs, raddr, true);
+ cpu_restore_state(cs, raddr);
/* Flags are current in env->cc_*, or are undefined. */
env->cc_op = CC_OP_FLAGS;
diff --git a/target/microblaze/helper.c b/target/microblaze/helper.c
index a607fe68e5..98bdb82de8 100644
--- a/target/microblaze/helper.c
+++ b/target/microblaze/helper.c
@@ -277,7 +277,7 @@ void mb_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
uint32_t esr, iflags;
/* Recover the pc and iflags from the corresponding insn_start. */
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
iflags = cpu->env.iflags;
qemu_log_mask(CPU_LOG_INT,
diff --git a/target/nios2/op_helper.c b/target/nios2/op_helper.c
index 2e30d0a908..0aaf33ffc2 100644
--- a/target/nios2/op_helper.c
+++ b/target/nios2/op_helper.c
@@ -40,7 +40,7 @@ void nios2_cpu_loop_exit_advance(CPUNios2State *env, uintptr_t retaddr)
* Do this here, rather than in restore_state_to_opc(),
* lest we affect QEMU internal exceptions, like EXCP_DEBUG.
*/
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
env->pc += 4;
cpu_loop_exit(cs);
}
diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
index dde2fa1623..ec145960e3 100644
--- a/target/openrisc/sys_helper.c
+++ b/target/openrisc/sys_helper.c
@@ -45,7 +45,7 @@ void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong spr, target_ulong rb)
break;
case TO_SPR(0, 16): /* NPC */
- cpu_restore_state(cs, GETPC(), true);
+ cpu_restore_state(cs, GETPC());
/* ??? Mirror or1ksim in not trashing delayed branch state
when "jumping" to the current instruction. */
if (env->pc != rb) {
@@ -131,7 +131,7 @@ void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong spr, target_ulong rb)
case TO_SPR(8, 0): /* PMR */
env->pmr = rb;
if (env->pmr & PMR_DME || env->pmr & PMR_SME) {
- cpu_restore_state(cs, GETPC(), true);
+ cpu_restore_state(cs, GETPC());
env->pc += 4;
cs->halted = 1;
raise_exception(cpu, EXCP_HALTED);
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 43f2480e94..3ded309265 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2414,7 +2414,7 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
uint32_t insn;
/* Restore state and reload the insn we executed, for filling in DSISR. */
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
insn = cpu_ldl_code(env, env->nip);
switch (env->mmu_model) {
diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
index 29ccf70df1..2cd6d062b9 100644
--- a/target/s390x/tcg/excp_helper.c
+++ b/target/s390x/tcg/excp_helper.c
@@ -39,7 +39,7 @@ G_NORETURN void tcg_s390_program_interrupt(CPUS390XState *env,
{
CPUState *cs = env_cpu(env);
- cpu_restore_state(cs, ra, true);
+ cpu_restore_state(cs, ra);
qemu_log_mask(CPU_LOG_INT, "program interrupt at %#" PRIx64 "\n",
env->psw.addr);
trigger_pgm_exception(env, code);
diff --git a/target/tricore/op_helper.c b/target/tricore/op_helper.c
index a79c838a92..532ae6b74c 100644
--- a/target/tricore/op_helper.c
+++ b/target/tricore/op_helper.c
@@ -31,7 +31,7 @@ void raise_exception_sync_internal(CPUTriCoreState *env, uint32_t class, int tin
{
CPUState *cs = env_cpu(env);
/* in case we come from a helper-call we need to restore the PC */
- cpu_restore_state(cs, pc, true);
+ cpu_restore_state(cs, pc);
/* Tin is loaded into d[15] */
env->gpr_d[15] = tin;
diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
index e0a9caab4b..2aa9777a8e 100644
--- a/target/xtensa/helper.c
+++ b/target/xtensa/helper.c
@@ -253,7 +253,7 @@ void xtensa_cpu_do_unaligned_access(CPUState *cs,
assert(xtensa_option_enabled(env->config,
XTENSA_OPTION_UNALIGNED_EXCEPTION));
- cpu_restore_state(CPU(cpu), retaddr, true);
+ cpu_restore_state(CPU(cpu), retaddr);
HELPER(exception_cause_vaddr)(env,
env->pc, LOAD_STORE_ALIGNMENT_CAUSE,
addr);
@@ -284,7 +284,7 @@ bool xtensa_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
} else if (probe) {
return false;
} else {
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
HELPER(exception_cause_vaddr)(env, env->pc, ret, address);
}
}
@@ -297,7 +297,7 @@ void xtensa_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
XtensaCPU *cpu = XTENSA_CPU(cs);
CPUXtensaState *env = &cpu->env;
- cpu_restore_state(cs, retaddr, true);
+ cpu_restore_state(cs, retaddr);
HELPER(exception_cause_vaddr)(env, env->pc,
access_type == MMU_INST_FETCH ?
INSTR_PIF_ADDR_ERROR_CAUSE :
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 6/6] accel/tcg: Remove reset_icount argument from cpu_restore_state_from_tb
2022-10-27 10:02 [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
` (4 preceding siblings ...)
2022-10-27 10:02 ` [PATCH v2 5/6] accel/tcg: Remove will_exit argument from cpu_restore_state Richard Henderson
@ 2022-10-27 10:02 ` Richard Henderson
2022-10-31 0:40 ` [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
6 siblings, 0 replies; 17+ messages in thread
From: Richard Henderson @ 2022-10-27 10:02 UTC (permalink / raw)
To: qemu-devel; +Cc: cfontana
The value passed is always true.
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/internal.h | 2 +-
accel/tcg/tb-maint.c | 4 ++--
accel/tcg/translate-all.c | 15 +++++++--------
3 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
index 9c06b320b7..cb13bade4f 100644
--- a/accel/tcg/internal.h
+++ b/accel/tcg/internal.h
@@ -107,7 +107,7 @@ TranslationBlock *tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
tb_page_addr_t phys_page2);
bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc);
void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
- uintptr_t host_pc, bool reset_icount);
+ uintptr_t host_pc);
/* Return the current PC from CPU, which may be cached in TB. */
static inline target_ulong log_pc(CPUState *cpu, const TranslationBlock *tb)
diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index c8e921089d..0cdb35548c 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -536,7 +536,7 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
* restore the CPU state.
*/
current_tb_modified = true;
- cpu_restore_state_from_tb(cpu, current_tb, retaddr, true);
+ cpu_restore_state_from_tb(cpu, current_tb, retaddr);
}
#endif /* TARGET_HAS_PRECISE_SMC */
tb_phys_invalidate__locked(tb);
@@ -685,7 +685,7 @@ bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc)
* function to partially restore the CPU state.
*/
current_tb_modified = true;
- cpu_restore_state_from_tb(cpu, current_tb, pc, true);
+ cpu_restore_state_from_tb(cpu, current_tb, pc);
}
#endif /* TARGET_HAS_PRECISE_SMC */
tb_phys_invalidate(tb, addr);
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 90997fed47..0089578f8f 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -282,12 +282,11 @@ static int cpu_unwind_data_from_tb(TranslationBlock *tb, uintptr_t host_pc,
}
/*
- * The cpu state corresponding to 'host_pc' is restored.
- * When reset_icount is true, current TB will be interrupted and
- * icount should be recalculated.
+ * The cpu state corresponding to 'host_pc' is restored in
+ * preparation for exiting the TB.
*/
void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
- uintptr_t host_pc, bool reset_icount)
+ uintptr_t host_pc)
{
uint64_t data[TARGET_INSN_START_WORDS];
#ifdef CONFIG_PROFILER
@@ -300,7 +299,7 @@ void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
return;
}
- if (reset_icount && (tb_cflags(tb) & CF_USE_ICOUNT)) {
+ if (tb_cflags(tb) & CF_USE_ICOUNT) {
assert(icount_enabled());
/*
* Reset the cycle counter to the start of the block and
@@ -333,7 +332,7 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc)
if (in_code_gen_buffer((const void *)(host_pc - tcg_splitwx_diff))) {
TranslationBlock *tb = tcg_tb_lookup(host_pc);
if (tb) {
- cpu_restore_state_from_tb(cpu, tb, host_pc, true);
+ cpu_restore_state_from_tb(cpu, tb, host_pc);
return true;
}
}
@@ -1032,7 +1031,7 @@ void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr)
tb = tcg_tb_lookup(retaddr);
if (tb) {
/* We can use retranslation to find the PC. */
- cpu_restore_state_from_tb(cpu, tb, retaddr, true);
+ cpu_restore_state_from_tb(cpu, tb, retaddr);
tb_phys_invalidate(tb, -1);
} else {
/* The exception probably happened in a helper. The CPU state should
@@ -1068,7 +1067,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
cpu_abort(cpu, "cpu_io_recompile: could not find TB for pc=%p",
(void *)retaddr);
}
- cpu_restore_state_from_tb(cpu, tb, retaddr, true);
+ cpu_restore_state_from_tb(cpu, tb, retaddr);
/*
* Some guests must re-execute the branch when re-executing a delay
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data
2022-10-27 10:02 ` [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data Richard Henderson
@ 2022-10-27 10:40 ` Claudio Fontana
2022-10-27 11:30 ` Richard Henderson
0 siblings, 1 reply; 17+ messages in thread
From: Claudio Fontana @ 2022-10-27 10:40 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
On 10/27/22 12:02, Richard Henderson wrote:
> Add a way to examine the unwind data without actually
> restoring the data back into env.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> accel/tcg/internal.h | 4 +--
> include/exec/exec-all.h | 21 ++++++++---
> accel/tcg/translate-all.c | 74 ++++++++++++++++++++++++++-------------
> 3 files changed, 68 insertions(+), 31 deletions(-)
>
> diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
> index 1227bb69bd..9c06b320b7 100644
> --- a/accel/tcg/internal.h
> +++ b/accel/tcg/internal.h
> @@ -106,8 +106,8 @@ void tb_reset_jump(TranslationBlock *tb, int n);
> TranslationBlock *tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
> tb_page_addr_t phys_page2);
> bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc);
> -int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> - uintptr_t searched_pc, bool reset_icount);
> +void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> + uintptr_t host_pc, bool reset_icount);
>
> /* Return the current PC from CPU, which may be cached in TB. */
> static inline target_ulong log_pc(CPUState *cpu, const TranslationBlock *tb)
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index e948992a80..7d851f5907 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -39,20 +39,33 @@ typedef ram_addr_t tb_page_addr_t;
> #define TB_PAGE_ADDR_FMT RAM_ADDR_FMT
> #endif
>
> +/**
> + * cpu_unwind_state_data:
> + * @cpu: the cpu context
> + * @host_pc: the host pc within the translation
> + * @data: output data
> + *
> + * Attempt to load the the unwind state for a host pc occurring in
> + * translated code. If @host_pc is not in translated code, the
> + * function returns false; otherwise @data is loaded.
> + * This is the same unwind info as given to restore_state_to_opc.
> + */
> +bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data);
> +
> /**
> * cpu_restore_state:
> - * @cpu: the vCPU state is to be restore to
> - * @searched_pc: the host PC the fault occurred at
> + * @cpu: the cpu context
> + * @host_pc: the host pc within the translation
> * @will_exit: true if the TB executed will be interrupted after some
> cpu adjustments. Required for maintaining the correct
> icount valus
> * @return: true if state was restored, false otherwise
> *
> * Attempt to restore the state for a fault occurring in translated
> - * code. If the searched_pc is not in translated code no state is
> + * code. If @host_pc is not in translated code no state is
> * restored and the function returns false.
> */
> -bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
> +bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit);
>
> G_NORETURN void cpu_loop_exit_noexc(CPUState *cpu);
> G_NORETURN void cpu_loop_exit(CPUState *cpu);
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index f185356a36..319becb698 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -247,52 +247,66 @@ static int encode_search(TranslationBlock *tb, uint8_t *block)
> return p - block;
> }
>
> -/* The cpu state corresponding to 'searched_pc' is restored.
> - * When reset_icount is true, current TB will be interrupted and
> - * icount should be recalculated.
> - */
> -int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> - uintptr_t searched_pc, bool reset_icount)
Maybe add a small comment about what the return value of this static function means?
It can be indirectly inferred from its point of use:
+ int insns_left = cpu_unwind_data_from_tb(tb, host_pc, data);
But I find having the information about the meaning of a function and return value useful to be available there.
IIUC for external functions the standard way is to document in the header files, but for the static functions I would think we can do it here.
With that Reviewed-by: Claudio Fontana <cfontana@suse.de>
> +static int cpu_unwind_data_from_tb(TranslationBlock *tb, uintptr_t host_pc,
> + uint64_t *data)
> {
> - uint64_t data[TARGET_INSN_START_WORDS];
> - uintptr_t host_pc = (uintptr_t)tb->tc.ptr;
> + uintptr_t iter_pc = (uintptr_t)tb->tc.ptr;
> const uint8_t *p = tb->tc.ptr + tb->tc.size;
> int i, j, num_insns = tb->icount;
> -#ifdef CONFIG_PROFILER
> - TCGProfile *prof = &tcg_ctx->prof;
> - int64_t ti = profile_getclock();
> -#endif
>
> - searched_pc -= GETPC_ADJ;
> + host_pc -= GETPC_ADJ;
>
> - if (searched_pc < host_pc) {
> + if (host_pc < iter_pc) {
> return -1;
> }
>
> - memset(data, 0, sizeof(data));
> + memset(data, 0, sizeof(uint64_t) * TARGET_INSN_START_WORDS);
> if (!TARGET_TB_PCREL) {
> data[0] = tb_pc(tb);
> }
>
> - /* Reconstruct the stored insn data while looking for the point at
> - which the end of the insn exceeds the searched_pc. */
> + /*
> + * Reconstruct the stored insn data while looking for the point
> + * at which the end of the insn exceeds host_pc.
> + */
> for (i = 0; i < num_insns; ++i) {
> for (j = 0; j < TARGET_INSN_START_WORDS; ++j) {
> data[j] += decode_sleb128(&p);
> }
> - host_pc += decode_sleb128(&p);
> - if (host_pc > searched_pc) {
> - goto found;
> + iter_pc += decode_sleb128(&p);
> + if (iter_pc > host_pc) {
> + return num_insns - i;
> }
> }
> return -1;
> +}
> +
> +/*
> + * The cpu state corresponding to 'host_pc' is restored.
> + * When reset_icount is true, current TB will be interrupted and
> + * icount should be recalculated.
> + */
> +void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> + uintptr_t host_pc, bool reset_icount)
> +{
> + uint64_t data[TARGET_INSN_START_WORDS];
> +#ifdef CONFIG_PROFILER
> + TCGProfile *prof = &tcg_ctx->prof;
> + int64_t ti = profile_getclock();
> +#endif
> + int insns_left = cpu_unwind_data_from_tb(tb, host_pc, data);
> +
> + if (insns_left < 0) {
> + return;
> + }
>
> - found:
> if (reset_icount && (tb_cflags(tb) & CF_USE_ICOUNT)) {
> assert(icount_enabled());
> - /* Reset the cycle counter to the start of the block
> - and shift if to the number of actually executed instructions */
> - cpu_neg(cpu)->icount_decr.u16.low += num_insns - i;
> + /*
> + * Reset the cycle counter to the start of the block and
> + * shift if to the number of actually executed instructions.
> + */
> + cpu_neg(cpu)->icount_decr.u16.low += insns_left;
> }
>
> cpu->cc->tcg_ops->restore_state_to_opc(cpu, tb, data);
> @@ -302,7 +316,6 @@ int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> prof->restore_time + profile_getclock() - ti);
> qatomic_set(&prof->restore_count, prof->restore_count + 1);
> #endif
> - return 0;
> }
>
> bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
> @@ -335,6 +348,17 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
> return false;
> }
>
> +bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data)
> +{
> + if (in_code_gen_buffer((const void *)(host_pc - tcg_splitwx_diff))) {
> + TranslationBlock *tb = tcg_tb_lookup(host_pc);
> + if (tb) {
> + return cpu_unwind_data_from_tb(tb, host_pc, data) >= 0;
> + }
> + }
> + return false;
> +}
> +
> void page_init(void)
> {
> page_size_init();
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/6] target/openrisc: Always exit after mtspr npc
2022-10-27 10:02 ` [PATCH v2 3/6] target/openrisc: Always exit after mtspr npc Richard Henderson
@ 2022-10-27 10:49 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-10-27 10:49 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: cfontana
On 27/10/22 12:02, Richard Henderson wrote:
> We have called cpu_restore_state asserting will_exit.
> Do not go back on that promise. This affects icount.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/openrisc/sys_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data
2022-10-27 10:40 ` Claudio Fontana
@ 2022-10-27 11:30 ` Richard Henderson
2022-10-27 11:39 ` Claudio Fontana
0 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2022-10-27 11:30 UTC (permalink / raw)
To: Claudio Fontana, qemu-devel
On 10/27/22 20:40, Claudio Fontana wrote:
> On 10/27/22 12:02, Richard Henderson wrote:
>> Add a way to examine the unwind data without actually
>> restoring the data back into env.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> accel/tcg/internal.h | 4 +--
>> include/exec/exec-all.h | 21 ++++++++---
>> accel/tcg/translate-all.c | 74 ++++++++++++++++++++++++++-------------
>> 3 files changed, 68 insertions(+), 31 deletions(-)
>>
>> diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
>> index 1227bb69bd..9c06b320b7 100644
>> --- a/accel/tcg/internal.h
>> +++ b/accel/tcg/internal.h
>> @@ -106,8 +106,8 @@ void tb_reset_jump(TranslationBlock *tb, int n);
>> TranslationBlock *tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>> tb_page_addr_t phys_page2);
>> bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc);
>> -int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
>> - uintptr_t searched_pc, bool reset_icount);
>> +void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
>> + uintptr_t host_pc, bool reset_icount);
>>
>> /* Return the current PC from CPU, which may be cached in TB. */
>> static inline target_ulong log_pc(CPUState *cpu, const TranslationBlock *tb)
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index e948992a80..7d851f5907 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -39,20 +39,33 @@ typedef ram_addr_t tb_page_addr_t;
>> #define TB_PAGE_ADDR_FMT RAM_ADDR_FMT
>> #endif
>>
>> +/**
>> + * cpu_unwind_state_data:
>> + * @cpu: the cpu context
>> + * @host_pc: the host pc within the translation
>> + * @data: output data
>> + *
>> + * Attempt to load the the unwind state for a host pc occurring in
>> + * translated code. If @host_pc is not in translated code, the
>> + * function returns false; otherwise @data is loaded.
>> + * This is the same unwind info as given to restore_state_to_opc.
>> + */
>> +bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data);
>> +
>> /**
>> * cpu_restore_state:
>> - * @cpu: the vCPU state is to be restore to
>> - * @searched_pc: the host PC the fault occurred at
>> + * @cpu: the cpu context
>> + * @host_pc: the host pc within the translation
>> * @will_exit: true if the TB executed will be interrupted after some
>> cpu adjustments. Required for maintaining the correct
>> icount valus
>> * @return: true if state was restored, false otherwise
>> *
>> * Attempt to restore the state for a fault occurring in translated
>> - * code. If the searched_pc is not in translated code no state is
>> + * code. If @host_pc is not in translated code no state is
>> * restored and the function returns false.
>> */
>> -bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
>> +bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit);
>>
>> G_NORETURN void cpu_loop_exit_noexc(CPUState *cpu);
>> G_NORETURN void cpu_loop_exit(CPUState *cpu);
>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>> index f185356a36..319becb698 100644
>> --- a/accel/tcg/translate-all.c
>> +++ b/accel/tcg/translate-all.c
>> @@ -247,52 +247,66 @@ static int encode_search(TranslationBlock *tb, uint8_t *block)
>> return p - block;
>> }
>>
>> -/* The cpu state corresponding to 'searched_pc' is restored.
>> - * When reset_icount is true, current TB will be interrupted and
>> - * icount should be recalculated.
>> - */
>> -int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
>> - uintptr_t searched_pc, bool reset_icount)
>
>
> Maybe add a small comment about what the return value of this static function means?
> It can be indirectly inferred from its point of use:
>
> + int insns_left = cpu_unwind_data_from_tb(tb, host_pc, data);
>
> But I find having the information about the meaning of a function and return value useful to be available there.
>
> IIUC for external functions the standard way is to document in the header files, but for the static functions I would think we can do it here.
>
> With that Reviewed-by: Claudio Fontana <cfontana@suse.de>
I added
+/**
+ * cpu_unwind_data_from_tb: Load unwind data for TB
+ * @tb: translation block
+ * @host_pc: the host pc within translation
+ * @data: output array
+ *
+ * Within @tb, locate the guest insn whose translation contains @host_pc,
+ * then load the unwind data created by INDEX_opc_start_insn for that
+ * guest insn. Return the number of guest insns which remain un-executed
+ * within @tb -- these must be credited back to the cpu's icount budget.
+ *
+ * If we could not determine which guest insn to which @host_pc belongs,
+ * return -1 and do not load unwind data.
+ * FIXME: Such a failure is likely to break the guest, as we were not
+ * expecting to unwind from such a location. This may be some sort of
+ * backend code generation problem. Consider asserting instead.
*/
Which I think captures some of your v1 comments as well.
r~
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data
2022-10-27 11:30 ` Richard Henderson
@ 2022-10-27 11:39 ` Claudio Fontana
0 siblings, 0 replies; 17+ messages in thread
From: Claudio Fontana @ 2022-10-27 11:39 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
On 10/27/22 13:30, Richard Henderson wrote:
> On 10/27/22 20:40, Claudio Fontana wrote:
>> On 10/27/22 12:02, Richard Henderson wrote:
>>> Add a way to examine the unwind data without actually
>>> restoring the data back into env.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>> accel/tcg/internal.h | 4 +--
>>> include/exec/exec-all.h | 21 ++++++++---
>>> accel/tcg/translate-all.c | 74 ++++++++++++++++++++++++++-------------
>>> 3 files changed, 68 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
>>> index 1227bb69bd..9c06b320b7 100644
>>> --- a/accel/tcg/internal.h
>>> +++ b/accel/tcg/internal.h
>>> @@ -106,8 +106,8 @@ void tb_reset_jump(TranslationBlock *tb, int n);
>>> TranslationBlock *tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>>> tb_page_addr_t phys_page2);
>>> bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc);
>>> -int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
>>> - uintptr_t searched_pc, bool reset_icount);
>>> +void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
>>> + uintptr_t host_pc, bool reset_icount);
>>>
>>> /* Return the current PC from CPU, which may be cached in TB. */
>>> static inline target_ulong log_pc(CPUState *cpu, const TranslationBlock *tb)
>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>> index e948992a80..7d851f5907 100644
>>> --- a/include/exec/exec-all.h
>>> +++ b/include/exec/exec-all.h
>>> @@ -39,20 +39,33 @@ typedef ram_addr_t tb_page_addr_t;
>>> #define TB_PAGE_ADDR_FMT RAM_ADDR_FMT
>>> #endif
>>>
>>> +/**
>>> + * cpu_unwind_state_data:
>>> + * @cpu: the cpu context
>>> + * @host_pc: the host pc within the translation
>>> + * @data: output data
>>> + *
>>> + * Attempt to load the the unwind state for a host pc occurring in
>>> + * translated code. If @host_pc is not in translated code, the
>>> + * function returns false; otherwise @data is loaded.
>>> + * This is the same unwind info as given to restore_state_to_opc.
>>> + */
>>> +bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data);
>>> +
>>> /**
>>> * cpu_restore_state:
>>> - * @cpu: the vCPU state is to be restore to
>>> - * @searched_pc: the host PC the fault occurred at
>>> + * @cpu: the cpu context
>>> + * @host_pc: the host pc within the translation
>>> * @will_exit: true if the TB executed will be interrupted after some
>>> cpu adjustments. Required for maintaining the correct
>>> icount valus
>>> * @return: true if state was restored, false otherwise
>>> *
>>> * Attempt to restore the state for a fault occurring in translated
>>> - * code. If the searched_pc is not in translated code no state is
>>> + * code. If @host_pc is not in translated code no state is
>>> * restored and the function returns false.
>>> */
>>> -bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
>>> +bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit);
>>>
>>> G_NORETURN void cpu_loop_exit_noexc(CPUState *cpu);
>>> G_NORETURN void cpu_loop_exit(CPUState *cpu);
>>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>>> index f185356a36..319becb698 100644
>>> --- a/accel/tcg/translate-all.c
>>> +++ b/accel/tcg/translate-all.c
>>> @@ -247,52 +247,66 @@ static int encode_search(TranslationBlock *tb, uint8_t *block)
>>> return p - block;
>>> }
>>>
>>> -/* The cpu state corresponding to 'searched_pc' is restored.
>>> - * When reset_icount is true, current TB will be interrupted and
>>> - * icount should be recalculated.
>>> - */
>>> -int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
>>> - uintptr_t searched_pc, bool reset_icount)
>>
>>
>> Maybe add a small comment about what the return value of this static function means?
>> It can be indirectly inferred from its point of use:
>>
>> + int insns_left = cpu_unwind_data_from_tb(tb, host_pc, data);
>>
>> But I find having the information about the meaning of a function and return value useful to be available there.
>>
>> IIUC for external functions the standard way is to document in the header files, but for the static functions I would think we can do it here.
>>
>> With that Reviewed-by: Claudio Fontana <cfontana@suse.de>
>
>
> I added
>
> +/**
> + * cpu_unwind_data_from_tb: Load unwind data for TB
> + * @tb: translation block
> + * @host_pc: the host pc within translation
> + * @data: output array
> + *
> + * Within @tb, locate the guest insn whose translation contains @host_pc,
> + * then load the unwind data created by INDEX_opc_start_insn for that
> + * guest insn. Return the number of guest insns which remain un-executed
> + * within @tb -- these must be credited back to the cpu's icount budget.
> + *
> + * If we could not determine which guest insn to which @host_pc belongs,
> + * return -1 and do not load unwind data.
> + * FIXME: Such a failure is likely to break the guest, as we were not
> + * expecting to unwind from such a location. This may be some sort of
> + * backend code generation problem. Consider asserting instead.
> */
>
> Which I think captures some of your v1 comments as well.
>
>
> r~
>
Very clear thanks,
Reviewed-by: Claudio Fontana <cfontana@suse.de>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/6] target/i386: Use cpu_unwind_state_data for tpr access
2022-10-27 10:02 ` [PATCH v2 2/6] target/i386: Use cpu_unwind_state_data for tpr access Richard Henderson
@ 2022-10-27 12:22 ` Claudio Fontana
2022-10-27 20:13 ` Richard Henderson
0 siblings, 1 reply; 17+ messages in thread
From: Claudio Fontana @ 2022-10-27 12:22 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
On 10/27/22 12:02, Richard Henderson wrote:
> Avoid cpu_restore_state, and modifying env->eip out from
> underneath the translator with TARGET_TB_PCREL. There is
> some slight duplication from x86_restore_state_to_opc,
> but it's just a few lines.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1269
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/i386/helper.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/helper.c b/target/i386/helper.c
> index b62a1e48e2..2cd1756f1a 100644
> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -509,6 +509,23 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank,
> }
> }
>
> +static target_ulong get_memio_eip(CPUX86State *env)
> +{
> + uint64_t data[TARGET_INSN_START_WORDS];
> + CPUState *cs = env_cpu(env);
> +
> + if (!cpu_unwind_state_data(cs, cs->mem_io_pc, data)) {
> + return env->eip;
> + }
> +
> + /* Per x86_restore_state_to_opc. */
> + if (TARGET_TB_PCREL) {
> + return (env->eip & TARGET_PAGE_MASK) | data[0];
> + } else {
> + return data[0] - env->segs[R_CS].base;
here we switch from taking cs_base from the TranslationBlock to taking it from env-> .
I traced the tb->cs_base use back to
cpu_exec() and cpu_exec_step_atomic()
and from there it seems ok, as the sequence is
cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags), which gets it from env,
followed by
tb_gen_code(...cs_base) which sets the TranslationBlock cs_base, and tb->cs_base does not seem to change again.
I mention this in the case there can be some weird situation in which env and tb can end up not being consistent,
does a TranslationBlock that is initialized with a certain cs_base from the env that contains user code to load / change the CS segment base potentially constitute a problem?
Ciao,
Claudio
> + }
> +}
> +
> void cpu_report_tpr_access(CPUX86State *env, TPRAccess access)
> {
> X86CPU *cpu = env_archcpu(env);
> @@ -519,9 +536,9 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access)
>
> cpu_interrupt(cs, CPU_INTERRUPT_TPR);
> } else if (tcg_enabled()) {
> - cpu_restore_state(cs, cs->mem_io_pc, false);
> + target_ulong eip = get_memio_eip(env);
>
> - apic_handle_tpr_access_report(cpu->apic_state, env->eip, access);
> + apic_handle_tpr_access_report(cpu->apic_state, eip, access);
> }
> }
> #endif /* !CONFIG_USER_ONLY */
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/6] target/i386: Use cpu_unwind_state_data for tpr access
2022-10-27 12:22 ` Claudio Fontana
@ 2022-10-27 20:13 ` Richard Henderson
2022-10-28 8:10 ` Claudio Fontana
0 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2022-10-27 20:13 UTC (permalink / raw)
To: Claudio Fontana, qemu-devel
On 10/27/22 22:22, Claudio Fontana wrote:
> On 10/27/22 12:02, Richard Henderson wrote:
>> + /* Per x86_restore_state_to_opc. */
>> + if (TARGET_TB_PCREL) {
>> + return (env->eip & TARGET_PAGE_MASK) | data[0];
>> + } else {
>> + return data[0] - env->segs[R_CS].base;
>
> here we switch from taking cs_base from the TranslationBlock to taking it from env-> .
>
> I traced the tb->cs_base use back to
>
> cpu_exec() and cpu_exec_step_atomic()
>
> and from there it seems ok, as the sequence is
>
> cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags), which gets it from env,
>
> followed by
>
> tb_gen_code(...cs_base) which sets the TranslationBlock cs_base, and tb->cs_base does
> not seem to change again.
Correct. I wondered if I'd made a mistake by not returning the TB located during the
search, but it doesn't seem to have mattered for the two users.
> I mention this in the case there can be some weird situation in which env and tb can
> end up not being consistent, does a TranslationBlock that is initialized with a certain
> cs_base from the env that contains user code to load / change the CS segment base
> potentially constitute a problem?
The only way to load/change a CS segment base is a branch instruction, which will of
course end the TB. There should be no way to change CS that continues the TB.
r~
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/6] target/i386: Use cpu_unwind_state_data for tpr access
2022-10-27 20:13 ` Richard Henderson
@ 2022-10-28 8:10 ` Claudio Fontana
0 siblings, 0 replies; 17+ messages in thread
From: Claudio Fontana @ 2022-10-28 8:10 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
On 10/27/22 22:13, Richard Henderson wrote:
> On 10/27/22 22:22, Claudio Fontana wrote:
>> On 10/27/22 12:02, Richard Henderson wrote:
>>> + /* Per x86_restore_state_to_opc. */
>>> + if (TARGET_TB_PCREL) {
>>> + return (env->eip & TARGET_PAGE_MASK) | data[0];
>>> + } else {
>>> + return data[0] - env->segs[R_CS].base;
>>
>> here we switch from taking cs_base from the TranslationBlock to taking it from env-> .
>>
>> I traced the tb->cs_base use back to
>>
>> cpu_exec() and cpu_exec_step_atomic()
>>
>> and from there it seems ok, as the sequence is
>>
>> cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags), which gets it from env,
>>
>> followed by
>>
>> tb_gen_code(...cs_base) which sets the TranslationBlock cs_base, and tb->cs_base does
>> not seem to change again.
> Correct. I wondered if I'd made a mistake by not returning the TB located during the
> search, but it doesn't seem to have mattered for the two users.
>
>> I mention this in the case there can be some weird situation in which env and tb can
>> end up not being consistent, does a TranslationBlock that is initialized with a certain
>> cs_base from the env that contains user code to load / change the CS segment base
>> potentially constitute a problem?
> The only way to load/change a CS segment base is a branch instruction, which will of
> course end the TB. There should be no way to change CS that continues the TB.
>
>
> r~
Reviewed-by: Claudio Fontana <cfontana@suse.de>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/6] target/openrisc: Use cpu_unwind_state_data for mfspr
2022-10-27 10:02 ` [PATCH v2 4/6] target/openrisc: Use cpu_unwind_state_data for mfspr Richard Henderson
@ 2022-10-28 9:16 ` Claudio Fontana
2022-10-28 18:46 ` Richard Henderson
0 siblings, 1 reply; 17+ messages in thread
From: Claudio Fontana @ 2022-10-28 9:16 UTC (permalink / raw)
To: Richard Henderson, Philippe Mathieu-Daudé; +Cc: qemu-devel
On 10/27/22 12:02, Richard Henderson wrote:
> Since we do not plan to exit, use cpu_unwind_state_data
> and extract exactly the data requested.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/openrisc/sys_helper.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
> index a3508e421d..dde2fa1623 100644
> --- a/target/openrisc/sys_helper.c
> +++ b/target/openrisc/sys_helper.c
> @@ -199,6 +199,7 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
> target_ulong spr)
> {
> #ifndef CONFIG_USER_ONLY
> + uint64_t data[TARGET_INSN_START_WORDS];
> MachineState *ms = MACHINE(qdev_get_machine());
> OpenRISCCPU *cpu = env_archcpu(env);
> CPUState *cs = env_cpu(env);
> @@ -232,14 +233,20 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
> return env->evbar;
>
> case TO_SPR(0, 16): /* NPC (equals PC) */
> - cpu_restore_state(cs, GETPC(), false);
> + if (cpu_unwind_state_data(cs, GETPC(), data)) {
> + return data[0];
> + }
> return env->pc;
>
> case TO_SPR(0, 17): /* SR */
> return cpu_get_sr(env);
>
> case TO_SPR(0, 18): /* PPC */
> - cpu_restore_state(cs, GETPC(), false);
> + if (cpu_unwind_state_data(cs, GETPC(), data)) {
> + if (data[1] & 2) {
> + return data[0] - 4;
> + }
> + }
> return env->ppc;
>
> case TO_SPR(0, 32): /* EPCR */
I am struggling to understand if the fact that we are not setting cpu->env.dflag anymore in the mfspr helper is fine;
here I am unfamiliar with the arch, also Ccing Philippe in case he wants to step in to review this bit.
Thanks,
CLaudio
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/6] target/openrisc: Use cpu_unwind_state_data for mfspr
2022-10-28 9:16 ` Claudio Fontana
@ 2022-10-28 18:46 ` Richard Henderson
0 siblings, 0 replies; 17+ messages in thread
From: Richard Henderson @ 2022-10-28 18:46 UTC (permalink / raw)
To: Claudio Fontana, Philippe Mathieu-Daudé; +Cc: qemu-devel, Stafford Horne
On 10/28/22 20:16, Claudio Fontana wrote:
> On 10/27/22 12:02, Richard Henderson wrote:
>> Since we do not plan to exit, use cpu_unwind_state_data
>> and extract exactly the data requested.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> target/openrisc/sys_helper.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
>> index a3508e421d..dde2fa1623 100644
>> --- a/target/openrisc/sys_helper.c
>> +++ b/target/openrisc/sys_helper.c
>> @@ -199,6 +199,7 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
>> target_ulong spr)
>> {
>> #ifndef CONFIG_USER_ONLY
>> + uint64_t data[TARGET_INSN_START_WORDS];
>> MachineState *ms = MACHINE(qdev_get_machine());
>> OpenRISCCPU *cpu = env_archcpu(env);
>> CPUState *cs = env_cpu(env);
>> @@ -232,14 +233,20 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
>> return env->evbar;
>>
>> case TO_SPR(0, 16): /* NPC (equals PC) */
>> - cpu_restore_state(cs, GETPC(), false);
>> + if (cpu_unwind_state_data(cs, GETPC(), data)) {
>> + return data[0];
>> + }
>> return env->pc;
>>
>> case TO_SPR(0, 17): /* SR */
>> return cpu_get_sr(env);
>>
>> case TO_SPR(0, 18): /* PPC */
>> - cpu_restore_state(cs, GETPC(), false);
>> + if (cpu_unwind_state_data(cs, GETPC(), data)) {
>> + if (data[1] & 2) {
>> + return data[0] - 4;
>> + }
>> + }
>> return env->ppc;
>>
>> case TO_SPR(0, 32): /* EPCR */
>
> I am struggling to understand if the fact that we are not setting cpu->env.dflag anymore in the mfspr helper is fine;
That we might do so before was buggy. We manage dflag in openrisc_tr_tb_stop expecting a
particular value. I should expand the patch comment on this.
Consider:
l.j L2 // branch
l.mfspr r1, ppc // delay
L1: boom
L2: l.lwa r3, (r4)
With "l.mfspr reg, ppc" executed in a delay slot, dflag would be set, but it would not be
cleared by openrisc_tr_tb_stop on exiting the TB, because tb_stop thinks the value is
already zero.
The next TB begins at L2 with dflag set when it should not. If the load has a tlb miss,
then the interrupt will be delivered with DSX set in the status register, and the pc
decremented (openrisc implements restart after delay slot by re-executing the branch).
This will cause the return from interrupt to go to L1, and boom!
r~
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269)
2022-10-27 10:02 [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
` (5 preceding siblings ...)
2022-10-27 10:02 ` [PATCH v2 6/6] accel/tcg: Remove reset_icount argument from cpu_restore_state_from_tb Richard Henderson
@ 2022-10-31 0:40 ` Richard Henderson
6 siblings, 0 replies; 17+ messages in thread
From: Richard Henderson @ 2022-10-31 0:40 UTC (permalink / raw)
To: qemu-devel; +Cc: cfontana
On 10/27/22 21:02, Richard Henderson wrote:
> As per #1269, this affects NetBSD installer boot.
>
> The problem is that one of the x86 acpi callbacks modifies
> env->eip during an mmio store, which means that the tracking
> that translate.c does is thrown out of whack.
>
> Introduce a method to extract unwind data without the
> writeback to env. This isn't a perfect abstraction, but I
> couldn't think of anything better. There's a couple of lines
> of code duplication, but probably less than any abstration
> that we might put on top
>
> Changes for v2:
> * Rebase on master, 23 patches merged.
> * Comments adjusted per review (claudio)
>
>
> r~
Queuing to tcg-next.
r~
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-10-31 5:47 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-27 10:02 [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
2022-10-27 10:02 ` [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data Richard Henderson
2022-10-27 10:40 ` Claudio Fontana
2022-10-27 11:30 ` Richard Henderson
2022-10-27 11:39 ` Claudio Fontana
2022-10-27 10:02 ` [PATCH v2 2/6] target/i386: Use cpu_unwind_state_data for tpr access Richard Henderson
2022-10-27 12:22 ` Claudio Fontana
2022-10-27 20:13 ` Richard Henderson
2022-10-28 8:10 ` Claudio Fontana
2022-10-27 10:02 ` [PATCH v2 3/6] target/openrisc: Always exit after mtspr npc Richard Henderson
2022-10-27 10:49 ` Philippe Mathieu-Daudé
2022-10-27 10:02 ` [PATCH v2 4/6] target/openrisc: Use cpu_unwind_state_data for mfspr Richard Henderson
2022-10-28 9:16 ` Claudio Fontana
2022-10-28 18:46 ` Richard Henderson
2022-10-27 10:02 ` [PATCH v2 5/6] accel/tcg: Remove will_exit argument from cpu_restore_state Richard Henderson
2022-10-27 10:02 ` [PATCH v2 6/6] accel/tcg: Remove reset_icount argument from cpu_restore_state_from_tb Richard Henderson
2022-10-31 0:40 ` [PATCH v2 0/6] tcg: Fix x86 TARGET_TB_PCREL (#1269) Richard Henderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.