* [PATCH v5 1/9] target/arm: Introduce curr_insn_len
2022-09-30 22:03 [PATCH v5 0/9] target/arm: pc-relative translation blocks Richard Henderson
@ 2022-09-30 22:03 ` Richard Henderson
2022-09-30 22:03 ` [PATCH v5 2/9] target/arm: Change gen_goto_tb to work on displacements Richard Henderson
` (7 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-09-30 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Philippe Mathieu-Daudé
A simple helper to retrieve the length of the current insn.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate.h | 5 +++++
target/arm/translate-vfp.c | 2 +-
target/arm/translate.c | 5 ++---
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/target/arm/translate.h b/target/arm/translate.h
index af5d4a7086..90bf7c57fc 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -226,6 +226,11 @@ static inline void disas_set_insn_syndrome(DisasContext *s, uint32_t syn)
s->insn_start = NULL;
}
+static inline int curr_insn_len(DisasContext *s)
+{
+ return s->base.pc_next - s->pc_curr;
+}
+
/* is_jmp field values */
#define DISAS_JUMP DISAS_TARGET_0 /* only pc was modified dynamically */
/* CPU state was modified dynamically; exit to main loop for interrupts. */
diff --git a/target/arm/translate-vfp.c b/target/arm/translate-vfp.c
index bd5ae27d09..94cc1e4b77 100644
--- a/target/arm/translate-vfp.c
+++ b/target/arm/translate-vfp.c
@@ -242,7 +242,7 @@ static bool vfp_access_check_a(DisasContext *s, bool ignore_vfp_enabled)
if (s->sme_trap_nonstreaming) {
gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
syn_smetrap(SME_ET_Streaming,
- s->base.pc_next - s->pc_curr == 2));
+ curr_insn_len(s) == 2));
return false;
}
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 5aaccbbf71..42e11102f7 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -6654,7 +6654,7 @@ static ISSInfo make_issinfo(DisasContext *s, int rd, bool p, bool w)
/* ISS not valid if writeback */
if (p && !w) {
ret = rd;
- if (s->base.pc_next - s->pc_curr == 2) {
+ if (curr_insn_len(s) == 2) {
ret |= ISSIs16Bit;
}
} else {
@@ -9817,8 +9817,7 @@ static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
/* nothing more to generate */
break;
case DISAS_WFI:
- gen_helper_wfi(cpu_env,
- tcg_constant_i32(dc->base.pc_next - dc->pc_curr));
+ gen_helper_wfi(cpu_env, tcg_constant_i32(curr_insn_len(dc)));
/*
* The helper doesn't necessarily throw an exception, but we
* must go back to the main loop to check for interrupts anyway.
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 2/9] target/arm: Change gen_goto_tb to work on displacements
2022-09-30 22:03 [PATCH v5 0/9] target/arm: pc-relative translation blocks Richard Henderson
2022-09-30 22:03 ` [PATCH v5 1/9] target/arm: Introduce curr_insn_len Richard Henderson
@ 2022-09-30 22:03 ` Richard Henderson
2022-09-30 22:03 ` [PATCH v5 3/9] target/arm: Change gen_*set_pc_im to gen_*update_pc Richard Henderson
` (6 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-09-30 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Peter Maydell
In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a64.c | 40 ++++++++++++++++++++------------------
target/arm/translate.c | 10 ++++++----
2 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 78b2d91ed4..8f5c2675f7 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -378,8 +378,10 @@ static inline bool use_goto_tb(DisasContext *s, uint64_t dest)
return translator_use_goto_tb(&s->base, dest);
}
-static inline void gen_goto_tb(DisasContext *s, int n, uint64_t dest)
+static void gen_goto_tb(DisasContext *s, int n, int64_t diff)
{
+ uint64_t dest = s->pc_curr + diff;
+
if (use_goto_tb(s, dest)) {
tcg_gen_goto_tb(n);
gen_a64_set_pc_im(dest);
@@ -1362,7 +1364,7 @@ static inline AArch64DecodeFn *lookup_disas_fn(const AArch64DecodeTable *table,
*/
static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
{
- uint64_t addr = s->pc_curr + sextract32(insn, 0, 26) * 4;
+ int64_t diff = sextract32(insn, 0, 26) * 4;
if (insn & (1U << 31)) {
/* BL Branch with link */
@@ -1371,7 +1373,7 @@ static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
/* B Branch / BL Branch with link */
reset_btype(s);
- gen_goto_tb(s, 0, addr);
+ gen_goto_tb(s, 0, diff);
}
/* Compare and branch (immediate)
@@ -1383,14 +1385,14 @@ static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
static void disas_comp_b_imm(DisasContext *s, uint32_t insn)
{
unsigned int sf, op, rt;
- uint64_t addr;
+ int64_t diff;
TCGLabel *label_match;
TCGv_i64 tcg_cmp;
sf = extract32(insn, 31, 1);
op = extract32(insn, 24, 1); /* 0: CBZ; 1: CBNZ */
rt = extract32(insn, 0, 5);
- addr = s->pc_curr + sextract32(insn, 5, 19) * 4;
+ diff = sextract32(insn, 5, 19) * 4;
tcg_cmp = read_cpu_reg(s, rt, sf);
label_match = gen_new_label();
@@ -1399,9 +1401,9 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t insn)
tcg_gen_brcondi_i64(op ? TCG_COND_NE : TCG_COND_EQ,
tcg_cmp, 0, label_match);
- gen_goto_tb(s, 0, s->base.pc_next);
+ gen_goto_tb(s, 0, 4);
gen_set_label(label_match);
- gen_goto_tb(s, 1, addr);
+ gen_goto_tb(s, 1, diff);
}
/* Test and branch (immediate)
@@ -1413,13 +1415,13 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t insn)
static void disas_test_b_imm(DisasContext *s, uint32_t insn)
{
unsigned int bit_pos, op, rt;
- uint64_t addr;
+ int64_t diff;
TCGLabel *label_match;
TCGv_i64 tcg_cmp;
bit_pos = (extract32(insn, 31, 1) << 5) | extract32(insn, 19, 5);
op = extract32(insn, 24, 1); /* 0: TBZ; 1: TBNZ */
- addr = s->pc_curr + sextract32(insn, 5, 14) * 4;
+ diff = sextract32(insn, 5, 14) * 4;
rt = extract32(insn, 0, 5);
tcg_cmp = tcg_temp_new_i64();
@@ -1430,9 +1432,9 @@ static void disas_test_b_imm(DisasContext *s, uint32_t insn)
tcg_gen_brcondi_i64(op ? TCG_COND_NE : TCG_COND_EQ,
tcg_cmp, 0, label_match);
tcg_temp_free_i64(tcg_cmp);
- gen_goto_tb(s, 0, s->base.pc_next);
+ gen_goto_tb(s, 0, 4);
gen_set_label(label_match);
- gen_goto_tb(s, 1, addr);
+ gen_goto_tb(s, 1, diff);
}
/* Conditional branch (immediate)
@@ -1444,13 +1446,13 @@ static void disas_test_b_imm(DisasContext *s, uint32_t insn)
static void disas_cond_b_imm(DisasContext *s, uint32_t insn)
{
unsigned int cond;
- uint64_t addr;
+ int64_t diff;
if ((insn & (1 << 4)) || (insn & (1 << 24))) {
unallocated_encoding(s);
return;
}
- addr = s->pc_curr + sextract32(insn, 5, 19) * 4;
+ diff = sextract32(insn, 5, 19) * 4;
cond = extract32(insn, 0, 4);
reset_btype(s);
@@ -1458,12 +1460,12 @@ static void disas_cond_b_imm(DisasContext *s, uint32_t insn)
/* genuinely conditional branches */
TCGLabel *label_match = gen_new_label();
arm_gen_test_cc(cond, label_match);
- gen_goto_tb(s, 0, s->base.pc_next);
+ gen_goto_tb(s, 0, 4);
gen_set_label(label_match);
- gen_goto_tb(s, 1, addr);
+ gen_goto_tb(s, 1, diff);
} else {
/* 0xe and 0xf are both "always" conditions */
- gen_goto_tb(s, 0, addr);
+ gen_goto_tb(s, 0, diff);
}
}
@@ -1637,7 +1639,7 @@ static void handle_sync(DisasContext *s, uint32_t insn,
* any pending interrupts immediately.
*/
reset_btype(s);
- gen_goto_tb(s, 0, s->base.pc_next);
+ gen_goto_tb(s, 0, 4);
return;
case 7: /* SB */
@@ -1649,7 +1651,7 @@ static void handle_sync(DisasContext *s, uint32_t insn,
* MB and end the TB instead.
*/
tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
- gen_goto_tb(s, 0, s->base.pc_next);
+ gen_goto_tb(s, 0, 4);
return;
default:
@@ -14955,7 +14957,7 @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
switch (dc->base.is_jmp) {
case DISAS_NEXT:
case DISAS_TOO_MANY:
- gen_goto_tb(dc, 1, dc->base.pc_next);
+ gen_goto_tb(dc, 1, 4);
break;
default:
case DISAS_UPDATE_EXIT:
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 42e11102f7..6855128fb1 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -2594,8 +2594,10 @@ static void gen_goto_ptr(void)
* cpu_loop_exec. Any live exit_requests will be processed as we
* enter the next TB.
*/
-static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)
+static void gen_goto_tb(DisasContext *s, int n, int diff)
{
+ target_ulong dest = s->pc_curr + diff;
+
if (translator_use_goto_tb(&s->base, dest)) {
tcg_gen_goto_tb(n);
gen_set_pc_im(s, dest);
@@ -2629,7 +2631,7 @@ static inline void gen_jmp_tb(DisasContext *s, uint32_t dest, int tbno)
* gen_jmp();
* on the second call to gen_jmp().
*/
- gen_goto_tb(s, tbno, dest);
+ gen_goto_tb(s, tbno, dest - s->pc_curr);
break;
case DISAS_UPDATE_NOCHAIN:
case DISAS_UPDATE_EXIT:
@@ -9798,7 +9800,7 @@ static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
switch (dc->base.is_jmp) {
case DISAS_NEXT:
case DISAS_TOO_MANY:
- gen_goto_tb(dc, 1, dc->base.pc_next);
+ gen_goto_tb(dc, 1, curr_insn_len(dc));
break;
case DISAS_UPDATE_NOCHAIN:
gen_set_pc_im(dc, dc->base.pc_next);
@@ -9850,7 +9852,7 @@ static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
gen_set_pc_im(dc, dc->base.pc_next);
gen_singlestep_exception(dc);
} else {
- gen_goto_tb(dc, 1, dc->base.pc_next);
+ gen_goto_tb(dc, 1, curr_insn_len(dc));
}
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 3/9] target/arm: Change gen_*set_pc_im to gen_*update_pc
2022-09-30 22:03 [PATCH v5 0/9] target/arm: pc-relative translation blocks Richard Henderson
2022-09-30 22:03 ` [PATCH v5 1/9] target/arm: Introduce curr_insn_len Richard Henderson
2022-09-30 22:03 ` [PATCH v5 2/9] target/arm: Change gen_goto_tb to work on displacements Richard Henderson
@ 2022-09-30 22:03 ` Richard Henderson
2022-10-03 14:18 ` Philippe Mathieu-Daudé via
2022-09-30 22:03 ` [PATCH v5 4/9] target/arm: Change gen_exception_insn* to work on displacements Richard Henderson
` (5 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2022-09-30 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
In preparation for TARGET_TB_PCREL, reduce reliance on
absolute values by passing in pc difference.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a32.h | 2 +-
target/arm/translate.h | 6 ++--
target/arm/translate-a64.c | 32 +++++++++---------
target/arm/translate-vfp.c | 2 +-
target/arm/translate.c | 68 ++++++++++++++++++++------------------
5 files changed, 56 insertions(+), 54 deletions(-)
diff --git a/target/arm/translate-a32.h b/target/arm/translate-a32.h
index 78a84c1414..5339c22f1e 100644
--- a/target/arm/translate-a32.h
+++ b/target/arm/translate-a32.h
@@ -40,7 +40,7 @@ void write_neon_element64(TCGv_i64 src, int reg, int ele, MemOp memop);
TCGv_i32 add_reg_for_lit(DisasContext *s, int reg, int ofs);
void gen_set_cpsr(TCGv_i32 var, uint32_t mask);
void gen_set_condexec(DisasContext *s);
-void gen_set_pc_im(DisasContext *s, target_ulong val);
+void gen_update_pc(DisasContext *s, target_long diff);
void gen_lookup_tb(DisasContext *s);
long vfp_reg_offset(bool dp, unsigned reg);
long neon_full_reg_offset(unsigned reg);
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 90bf7c57fc..d651044855 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -254,7 +254,7 @@ static inline int curr_insn_len(DisasContext *s)
* For instructions which want an immediate exit to the main loop, as opposed
* to attempting to use lookup_and_goto_ptr. Unlike DISAS_UPDATE_EXIT, this
* doesn't write the PC on exiting the translation loop so you need to ensure
- * something (gen_a64_set_pc_im or runtime helper) has done so before we reach
+ * something (gen_a64_update_pc or runtime helper) has done so before we reach
* return from cpu_tb_exec.
*/
#define DISAS_EXIT DISAS_TARGET_9
@@ -263,14 +263,14 @@ static inline int curr_insn_len(DisasContext *s)
#ifdef TARGET_AARCH64
void a64_translate_init(void);
-void gen_a64_set_pc_im(uint64_t val);
+void gen_a64_update_pc(DisasContext *s, target_long diff);
extern const TranslatorOps aarch64_translator_ops;
#else
static inline void a64_translate_init(void)
{
}
-static inline void gen_a64_set_pc_im(uint64_t val)
+static inline void gen_a64_update_pc(DisasContext *s, target_long diff)
{
}
#endif
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 8f5c2675f7..914c789187 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -148,9 +148,9 @@ static void reset_btype(DisasContext *s)
}
}
-void gen_a64_set_pc_im(uint64_t val)
+void gen_a64_update_pc(DisasContext *s, target_long diff)
{
- tcg_gen_movi_i64(cpu_pc, val);
+ tcg_gen_movi_i64(cpu_pc, s->pc_curr + diff);
}
/*
@@ -342,14 +342,14 @@ static void gen_exception_internal(int excp)
static void gen_exception_internal_insn(DisasContext *s, uint64_t pc, int excp)
{
- gen_a64_set_pc_im(pc);
+ gen_a64_update_pc(s, pc - s->pc_curr);
gen_exception_internal(excp);
s->base.is_jmp = DISAS_NORETURN;
}
static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syndrome)
{
- gen_a64_set_pc_im(s->pc_curr);
+ gen_a64_update_pc(s, 0);
gen_helper_exception_bkpt_insn(cpu_env, tcg_constant_i32(syndrome));
s->base.is_jmp = DISAS_NORETURN;
}
@@ -384,11 +384,11 @@ static void gen_goto_tb(DisasContext *s, int n, int64_t diff)
if (use_goto_tb(s, dest)) {
tcg_gen_goto_tb(n);
- gen_a64_set_pc_im(dest);
+ gen_a64_update_pc(s, diff);
tcg_gen_exit_tb(s->base.tb, n);
s->base.is_jmp = DISAS_NORETURN;
} else {
- gen_a64_set_pc_im(dest);
+ gen_a64_update_pc(s, diff);
if (s->ss_active) {
gen_step_complete_exception(s);
} else {
@@ -1960,7 +1960,7 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
uint32_t syndrome;
syndrome = syn_aa64_sysregtrap(op0, op1, op2, crn, crm, rt, isread);
- gen_a64_set_pc_im(s->pc_curr);
+ gen_a64_update_pc(s, 0);
gen_helper_access_check_cp_reg(cpu_env,
tcg_constant_ptr(ri),
tcg_constant_i32(syndrome),
@@ -1970,7 +1970,7 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
* The readfn or writefn might raise an exception;
* synchronize the CPU state in case it does.
*/
- gen_a64_set_pc_im(s->pc_curr);
+ gen_a64_update_pc(s, 0);
}
/* Handle special cases first */
@@ -2180,7 +2180,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
/* The pre HVC helper handles cases when HVC gets trapped
* as an undefined insn by runtime configuration.
*/
- gen_a64_set_pc_im(s->pc_curr);
+ gen_a64_update_pc(s, 0);
gen_helper_pre_hvc(cpu_env);
gen_ss_advance(s);
gen_exception_insn_el(s, s->base.pc_next, EXCP_HVC,
@@ -2191,7 +2191,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
unallocated_encoding(s);
break;
}
- gen_a64_set_pc_im(s->pc_curr);
+ gen_a64_update_pc(s, 0);
gen_helper_pre_smc(cpu_env, tcg_constant_i32(syn_aa64_smc(imm16)));
gen_ss_advance(s);
gen_exception_insn_el(s, s->base.pc_next, EXCP_SMC,
@@ -14944,7 +14944,7 @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
*/
switch (dc->base.is_jmp) {
default:
- gen_a64_set_pc_im(dc->base.pc_next);
+ gen_a64_update_pc(dc, 4);
/* fall through */
case DISAS_EXIT:
case DISAS_JUMP:
@@ -14961,13 +14961,13 @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
break;
default:
case DISAS_UPDATE_EXIT:
- gen_a64_set_pc_im(dc->base.pc_next);
+ gen_a64_update_pc(dc, 4);
/* fall through */
case DISAS_EXIT:
tcg_gen_exit_tb(NULL, 0);
break;
case DISAS_UPDATE_NOCHAIN:
- gen_a64_set_pc_im(dc->base.pc_next);
+ gen_a64_update_pc(dc, 4);
/* fall through */
case DISAS_JUMP:
tcg_gen_lookup_and_goto_ptr();
@@ -14976,11 +14976,11 @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
case DISAS_SWI:
break;
case DISAS_WFE:
- gen_a64_set_pc_im(dc->base.pc_next);
+ gen_a64_update_pc(dc, 4);
gen_helper_wfe(cpu_env);
break;
case DISAS_YIELD:
- gen_a64_set_pc_im(dc->base.pc_next);
+ gen_a64_update_pc(dc, 4);
gen_helper_yield(cpu_env);
break;
case DISAS_WFI:
@@ -14988,7 +14988,7 @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
* This is a special case because we don't want to just halt
* the CPU if trying to debug across a WFI.
*/
- gen_a64_set_pc_im(dc->base.pc_next);
+ gen_a64_update_pc(dc, 4);
gen_helper_wfi(cpu_env, tcg_constant_i32(4));
/*
* The helper doesn't necessarily throw an exception, but we
diff --git a/target/arm/translate-vfp.c b/target/arm/translate-vfp.c
index 94cc1e4b77..070f465b17 100644
--- a/target/arm/translate-vfp.c
+++ b/target/arm/translate-vfp.c
@@ -856,7 +856,7 @@ static bool trans_VMSR_VMRS(DisasContext *s, arg_VMSR_VMRS *a)
case ARM_VFP_FPSID:
if (s->current_el == 1) {
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc_curr);
+ gen_update_pc(s, 0);
gen_helper_check_hcr_el2_trap(cpu_env,
tcg_constant_i32(a->rt),
tcg_constant_i32(a->reg));
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6855128fb1..01b7536c7e 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -772,9 +772,9 @@ void gen_set_condexec(DisasContext *s)
}
}
-void gen_set_pc_im(DisasContext *s, target_ulong val)
+void gen_update_pc(DisasContext *s, target_long diff)
{
- tcg_gen_movi_i32(cpu_R[15], val);
+ tcg_gen_movi_i32(cpu_R[15], s->pc_curr + diff);
}
/* Set PC and Thumb state from var. var is marked as dead. */
@@ -866,7 +866,7 @@ static inline void gen_bxns(DisasContext *s, int rm)
/* The bxns helper may raise an EXCEPTION_EXIT exception, so in theory
* we need to sync state before calling it, but:
- * - we don't need to do gen_set_pc_im() because the bxns helper will
+ * - we don't need to do gen_update_pc() because the bxns helper will
* always set the PC itself
* - we don't need to do gen_set_condexec() because BXNS is UNPREDICTABLE
* unless it's outside an IT block or the last insn in an IT block,
@@ -887,7 +887,7 @@ static inline void gen_blxns(DisasContext *s, int rm)
* We do however need to set the PC, because the blxns helper reads it.
* The blxns helper may throw an exception.
*/
- gen_set_pc_im(s, s->base.pc_next);
+ gen_update_pc(s, curr_insn_len(s));
gen_helper_v7m_blxns(cpu_env, var);
tcg_temp_free_i32(var);
s->base.is_jmp = DISAS_EXIT;
@@ -1055,7 +1055,7 @@ static inline void gen_hvc(DisasContext *s, int imm16)
* as an undefined insn by runtime configuration (ie before
* the insn really executes).
*/
- gen_set_pc_im(s, s->pc_curr);
+ gen_update_pc(s, 0);
gen_helper_pre_hvc(cpu_env);
/* Otherwise we will treat this as a real exception which
* happens after execution of the insn. (The distinction matters
@@ -1063,7 +1063,7 @@ static inline void gen_hvc(DisasContext *s, int imm16)
* for single stepping.)
*/
s->svc_imm = imm16;
- gen_set_pc_im(s, s->base.pc_next);
+ gen_update_pc(s, curr_insn_len(s));
s->base.is_jmp = DISAS_HVC;
}
@@ -1072,16 +1072,16 @@ static inline void gen_smc(DisasContext *s)
/* As with HVC, we may take an exception either before or after
* the insn executes.
*/
- gen_set_pc_im(s, s->pc_curr);
+ gen_update_pc(s, 0);
gen_helper_pre_smc(cpu_env, tcg_constant_i32(syn_aa32_smc()));
- gen_set_pc_im(s, s->base.pc_next);
+ gen_update_pc(s, curr_insn_len(s));
s->base.is_jmp = DISAS_SMC;
}
static void gen_exception_internal_insn(DisasContext *s, uint32_t pc, int excp)
{
gen_set_condexec(s);
- gen_set_pc_im(s, pc);
+ gen_update_pc(s, pc - s->pc_curr);
gen_exception_internal(excp);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -1107,10 +1107,10 @@ static void gen_exception_insn_el_v(DisasContext *s, uint64_t pc, int excp,
uint32_t syn, TCGv_i32 tcg_el)
{
if (s->aarch64) {
- gen_a64_set_pc_im(pc);
+ gen_a64_update_pc(s, pc - s->pc_curr);
} else {
gen_set_condexec(s);
- gen_set_pc_im(s, pc);
+ gen_update_pc(s, pc - s->pc_curr);
}
gen_exception_el_v(excp, syn, tcg_el);
s->base.is_jmp = DISAS_NORETURN;
@@ -1125,10 +1125,10 @@ void gen_exception_insn_el(DisasContext *s, uint64_t pc, int excp,
void gen_exception_insn(DisasContext *s, uint64_t pc, int excp, uint32_t syn)
{
if (s->aarch64) {
- gen_a64_set_pc_im(pc);
+ gen_a64_update_pc(s, pc - s->pc_curr);
} else {
gen_set_condexec(s);
- gen_set_pc_im(s, pc);
+ gen_update_pc(s, pc - s->pc_curr);
}
gen_exception(excp, syn);
s->base.is_jmp = DISAS_NORETURN;
@@ -1137,7 +1137,7 @@ void gen_exception_insn(DisasContext *s, uint64_t pc, int excp, uint32_t syn)
static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syn)
{
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc_curr);
+ gen_update_pc(s, 0);
gen_helper_exception_bkpt_insn(cpu_env, tcg_constant_i32(syn));
s->base.is_jmp = DISAS_NORETURN;
}
@@ -2600,10 +2600,10 @@ static void gen_goto_tb(DisasContext *s, int n, int diff)
if (translator_use_goto_tb(&s->base, dest)) {
tcg_gen_goto_tb(n);
- gen_set_pc_im(s, dest);
+ gen_update_pc(s, diff);
tcg_gen_exit_tb(s->base.tb, n);
} else {
- gen_set_pc_im(s, dest);
+ gen_update_pc(s, diff);
gen_goto_ptr();
}
s->base.is_jmp = DISAS_NORETURN;
@@ -2612,9 +2612,11 @@ static void gen_goto_tb(DisasContext *s, int n, int diff)
/* Jump, specifying which TB number to use if we gen_goto_tb() */
static inline void gen_jmp_tb(DisasContext *s, uint32_t dest, int tbno)
{
+ int diff = dest - s->pc_curr;
+
if (unlikely(s->ss_active)) {
/* An indirect jump so that we still trigger the debug exception. */
- gen_set_pc_im(s, dest);
+ gen_update_pc(s, diff);
s->base.is_jmp = DISAS_JUMP;
return;
}
@@ -2631,7 +2633,7 @@ static inline void gen_jmp_tb(DisasContext *s, uint32_t dest, int tbno)
* gen_jmp();
* on the second call to gen_jmp().
*/
- gen_goto_tb(s, tbno, dest - s->pc_curr);
+ gen_goto_tb(s, tbno, diff);
break;
case DISAS_UPDATE_NOCHAIN:
case DISAS_UPDATE_EXIT:
@@ -2640,7 +2642,7 @@ static inline void gen_jmp_tb(DisasContext *s, uint32_t dest, int tbno)
* Avoid using goto_tb so we really do exit back to the main loop
* and don't chain to another TB.
*/
- gen_set_pc_im(s, dest);
+ gen_update_pc(s, diff);
gen_goto_ptr();
s->base.is_jmp = DISAS_NORETURN;
break;
@@ -2908,7 +2910,7 @@ static void gen_msr_banked(DisasContext *s, int r, int sysm, int rn)
/* Sync state because msr_banked() can raise exceptions */
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc_curr);
+ gen_update_pc(s, 0);
tcg_reg = load_reg(s, rn);
gen_helper_msr_banked(cpu_env, tcg_reg,
tcg_constant_i32(tgtmode),
@@ -2928,7 +2930,7 @@ static void gen_mrs_banked(DisasContext *s, int r, int sysm, int rn)
/* Sync state because mrs_banked() can raise exceptions */
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc_curr);
+ gen_update_pc(s, 0);
tcg_reg = tcg_temp_new_i32();
gen_helper_mrs_banked(tcg_reg, cpu_env,
tcg_constant_i32(tgtmode),
@@ -4749,7 +4751,7 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
}
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc_curr);
+ gen_update_pc(s, 0);
gen_helper_access_check_cp_reg(cpu_env,
tcg_constant_ptr(ri),
tcg_constant_i32(syndrome),
@@ -4760,7 +4762,7 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
* synchronize the CPU state in case it does.
*/
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc_curr);
+ gen_update_pc(s, 0);
}
/* Handle special cases first */
@@ -4774,7 +4776,7 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
unallocated_encoding(s);
return;
}
- gen_set_pc_im(s, s->base.pc_next);
+ gen_update_pc(s, curr_insn_len(s));
s->base.is_jmp = DISAS_WFI;
return;
default:
@@ -5161,7 +5163,7 @@ static void gen_srs(DisasContext *s,
addr = tcg_temp_new_i32();
/* get_r13_banked() will raise an exception if called from System mode */
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc_curr);
+ gen_update_pc(s, 0);
gen_helper_get_r13_banked(addr, cpu_env, tcg_constant_i32(mode));
switch (amode) {
case 0: /* DA */
@@ -6230,7 +6232,7 @@ static bool trans_YIELD(DisasContext *s, arg_YIELD *a)
* scheduling of other vCPUs.
*/
if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
- gen_set_pc_im(s, s->base.pc_next);
+ gen_update_pc(s, curr_insn_len(s));
s->base.is_jmp = DISAS_YIELD;
}
return true;
@@ -6246,7 +6248,7 @@ static bool trans_WFE(DisasContext *s, arg_WFE *a)
* implemented so we can't sleep like WFI does.
*/
if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
- gen_set_pc_im(s, s->base.pc_next);
+ gen_update_pc(s, curr_insn_len(s));
s->base.is_jmp = DISAS_WFE;
}
return true;
@@ -6255,7 +6257,7 @@ static bool trans_WFE(DisasContext *s, arg_WFE *a)
static bool trans_WFI(DisasContext *s, arg_WFI *a)
{
/* For WFI, halt the vCPU until an IRQ. */
- gen_set_pc_im(s, s->base.pc_next);
+ gen_update_pc(s, curr_insn_len(s));
s->base.is_jmp = DISAS_WFI;
return true;
}
@@ -8765,7 +8767,7 @@ static bool trans_SVC(DisasContext *s, arg_SVC *a)
(a->imm == semihost_imm)) {
gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
} else {
- gen_set_pc_im(s, s->base.pc_next);
+ gen_update_pc(s, curr_insn_len(s));
s->svc_imm = a->imm;
s->base.is_jmp = DISAS_SWI;
}
@@ -9779,7 +9781,7 @@ static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
case DISAS_TOO_MANY:
case DISAS_UPDATE_EXIT:
case DISAS_UPDATE_NOCHAIN:
- gen_set_pc_im(dc, dc->base.pc_next);
+ gen_update_pc(dc, curr_insn_len(dc));
/* fall through */
default:
/* FIXME: Single stepping a WFI insn will not halt the CPU. */
@@ -9803,13 +9805,13 @@ static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
gen_goto_tb(dc, 1, curr_insn_len(dc));
break;
case DISAS_UPDATE_NOCHAIN:
- gen_set_pc_im(dc, dc->base.pc_next);
+ gen_update_pc(dc, curr_insn_len(dc));
/* fall through */
case DISAS_JUMP:
gen_goto_ptr();
break;
case DISAS_UPDATE_EXIT:
- gen_set_pc_im(dc, dc->base.pc_next);
+ gen_update_pc(dc, curr_insn_len(dc));
/* fall through */
default:
/* indicate that the hash table must be used to find the next TB */
@@ -9849,7 +9851,7 @@ static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
gen_set_label(dc->condlabel);
gen_set_condexec(dc);
if (unlikely(dc->ss_active)) {
- gen_set_pc_im(dc, dc->base.pc_next);
+ gen_update_pc(dc, curr_insn_len(dc));
gen_singlestep_exception(dc);
} else {
gen_goto_tb(dc, 1, curr_insn_len(dc));
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 3/9] target/arm: Change gen_*set_pc_im to gen_*update_pc
2022-09-30 22:03 ` [PATCH v5 3/9] target/arm: Change gen_*set_pc_im to gen_*update_pc Richard Henderson
@ 2022-10-03 14:18 ` Philippe Mathieu-Daudé via
0 siblings, 0 replies; 24+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-10-03 14:18 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-arm
On 1/10/22 00:03, Richard Henderson wrote:
> In preparation for TARGET_TB_PCREL, reduce reliance on
> absolute values by passing in pc difference.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/translate-a32.h | 2 +-
> target/arm/translate.h | 6 ++--
> target/arm/translate-a64.c | 32 +++++++++---------
> target/arm/translate-vfp.c | 2 +-
> target/arm/translate.c | 68 ++++++++++++++++++++------------------
> 5 files changed, 56 insertions(+), 54 deletions(-)
> -void gen_a64_set_pc_im(uint64_t val)
> +void gen_a64_update_pc(DisasContext *s, target_long diff)
> {
> - tcg_gen_movi_i64(cpu_pc, val);
> + tcg_gen_movi_i64(cpu_pc, s->pc_curr + diff);
> }
> @@ -384,11 +384,11 @@ static void gen_goto_tb(DisasContext *s, int n, int64_t diff)
>
Adding more context from previous patch to ease review:
> uint64_t dest = s->pc_curr + diff;
> if (use_goto_tb(s, dest)) {
> tcg_gen_goto_tb(n);
> - gen_a64_set_pc_im(dest);
> + gen_a64_update_pc(s, diff);
> tcg_gen_exit_tb(s->base.tb, n);
> s->base.is_jmp = DISAS_NORETURN;
> } else {
> - gen_a64_set_pc_im(dest);
> + gen_a64_update_pc(s, diff);
> if (s->ss_active) {
> gen_step_complete_exception(s);
> } else {
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 6855128fb1..01b7536c7e 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -772,9 +772,9 @@ void gen_set_condexec(DisasContext *s)
> }
> }
>
> -void gen_set_pc_im(DisasContext *s, target_ulong val)
> +void gen_update_pc(DisasContext *s, target_long diff)
> {
> - tcg_gen_movi_i32(cpu_R[15], val);
> + tcg_gen_movi_i32(cpu_R[15], s->pc_curr + diff);
> }
> @@ -2600,10 +2600,10 @@ static void gen_goto_tb(DisasContext *s, int n, int diff)
>
Ditto:
> target_ulong dest = s->pc_curr + diff;
> if (translator_use_goto_tb(&s->base, dest)) {
> tcg_gen_goto_tb(n);
> - gen_set_pc_im(s, dest);
> + gen_update_pc(s, diff);
> tcg_gen_exit_tb(s->base.tb, n);
> } else {
> - gen_set_pc_im(s, dest);
> + gen_update_pc(s, diff);
> gen_goto_ptr();
> }
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 4/9] target/arm: Change gen_exception_insn* to work on displacements
2022-09-30 22:03 [PATCH v5 0/9] target/arm: pc-relative translation blocks Richard Henderson
` (2 preceding siblings ...)
2022-09-30 22:03 ` [PATCH v5 3/9] target/arm: Change gen_*set_pc_im to gen_*update_pc Richard Henderson
@ 2022-09-30 22:03 ` Richard Henderson
2022-10-03 14:21 ` Philippe Mathieu-Daudé via
2022-09-30 22:03 ` [PATCH v5 5/9] target/arm: Remove gen_exception_internal_insn pc argument Richard Henderson
` (4 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2022-09-30 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate.h | 5 +++--
target/arm/translate-a64.c | 28 ++++++++++-------------
target/arm/translate-m-nocp.c | 6 ++---
target/arm/translate-mve.c | 2 +-
target/arm/translate-vfp.c | 6 ++---
target/arm/translate.c | 42 +++++++++++++++++------------------
6 files changed, 43 insertions(+), 46 deletions(-)
diff --git a/target/arm/translate.h b/target/arm/translate.h
index d651044855..4aa239e23c 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -281,9 +281,10 @@ void arm_jump_cc(DisasCompare *cmp, TCGLabel *label);
void arm_gen_test_cc(int cc, TCGLabel *label);
MemOp pow2_align(unsigned i);
void unallocated_encoding(DisasContext *s);
-void gen_exception_insn_el(DisasContext *s, uint64_t pc, int excp,
+void gen_exception_insn_el(DisasContext *s, target_long pc_diff, int excp,
uint32_t syn, uint32_t target_el);
-void gen_exception_insn(DisasContext *s, uint64_t pc, int excp, uint32_t syn);
+void gen_exception_insn(DisasContext *s, target_long pc_diff,
+ int excp, uint32_t syn);
/* Return state of Alternate Half-precision flag, caller frees result */
static inline TCGv_i32 get_ahp_flag(void)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 914c789187..2621b3b36a 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1163,7 +1163,7 @@ static bool fp_access_check_only(DisasContext *s)
assert(!s->fp_access_checked);
s->fp_access_checked = true;
- gen_exception_insn_el(s, s->pc_curr, EXCP_UDEF,
+ gen_exception_insn_el(s, 0, EXCP_UDEF,
syn_fp_access_trap(1, 0xe, false, 0),
s->fp_excp_el);
return false;
@@ -1178,7 +1178,7 @@ static bool fp_access_check(DisasContext *s)
return false;
}
if (s->sme_trap_nonstreaming && s->is_nonstreaming) {
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ gen_exception_insn(s, 0, EXCP_UDEF,
syn_smetrap(SME_ET_Streaming, false));
return false;
}
@@ -1198,7 +1198,7 @@ bool sve_access_check(DisasContext *s)
goto fail_exit;
}
} else if (s->sve_excp_el) {
- gen_exception_insn_el(s, s->pc_curr, EXCP_UDEF,
+ gen_exception_insn_el(s, 0, EXCP_UDEF,
syn_sve_access_trap(), s->sve_excp_el);
goto fail_exit;
}
@@ -1220,7 +1220,7 @@ bool sve_access_check(DisasContext *s)
static bool sme_access_check(DisasContext *s)
{
if (s->sme_excp_el) {
- gen_exception_insn_el(s, s->pc_curr, EXCP_UDEF,
+ gen_exception_insn_el(s, 0, EXCP_UDEF,
syn_smetrap(SME_ET_AccessTrap, false),
s->sme_excp_el);
return false;
@@ -1250,12 +1250,12 @@ bool sme_enabled_check_with_svcr(DisasContext *s, unsigned req)
return false;
}
if (FIELD_EX64(req, SVCR, SM) && !s->pstate_sm) {
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ gen_exception_insn(s, 0, EXCP_UDEF,
syn_smetrap(SME_ET_NotStreaming, false));
return false;
}
if (FIELD_EX64(req, SVCR, ZA) && !s->pstate_za) {
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ gen_exception_insn(s, 0, EXCP_UDEF,
syn_smetrap(SME_ET_InactiveZA, false));
return false;
}
@@ -1915,7 +1915,7 @@ static void gen_sysreg_undef(DisasContext *s, bool isread,
} else {
syndrome = syn_uncategorized();
}
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syndrome);
+ gen_exception_insn(s, 0, EXCP_UDEF, syndrome);
}
/* MRS - move from system register
@@ -2169,8 +2169,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
switch (op2_ll) {
case 1: /* SVC */
gen_ss_advance(s);
- gen_exception_insn(s, s->base.pc_next, EXCP_SWI,
- syn_aa64_svc(imm16));
+ gen_exception_insn(s, 4, EXCP_SWI, syn_aa64_svc(imm16));
break;
case 2: /* HVC */
if (s->current_el == 0) {
@@ -2183,8 +2182,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
gen_a64_update_pc(s, 0);
gen_helper_pre_hvc(cpu_env);
gen_ss_advance(s);
- gen_exception_insn_el(s, s->base.pc_next, EXCP_HVC,
- syn_aa64_hvc(imm16), 2);
+ gen_exception_insn_el(s, 4, EXCP_HVC, syn_aa64_hvc(imm16), 2);
break;
case 3: /* SMC */
if (s->current_el == 0) {
@@ -2194,8 +2192,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
gen_a64_update_pc(s, 0);
gen_helper_pre_smc(cpu_env, tcg_constant_i32(syn_aa64_smc(imm16)));
gen_ss_advance(s);
- gen_exception_insn_el(s, s->base.pc_next, EXCP_SMC,
- syn_aa64_smc(imm16), 3);
+ gen_exception_insn_el(s, 4, EXCP_SMC, syn_aa64_smc(imm16), 3);
break;
default:
unallocated_encoding(s);
@@ -14833,7 +14830,7 @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
* Illegal execution state. This has priority over BTI
* exceptions, but comes after instruction abort exceptions.
*/
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_illegalstate());
+ gen_exception_insn(s, 0, EXCP_UDEF, syn_illegalstate());
return;
}
@@ -14864,8 +14861,7 @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
if (s->btype != 0
&& s->guarded_page
&& !btype_destination_ok(insn, s->bt, s->btype)) {
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
- syn_btitrap(s->btype));
+ gen_exception_insn(s, 0, EXCP_UDEF, syn_btitrap(s->btype));
return;
}
} else {
diff --git a/target/arm/translate-m-nocp.c b/target/arm/translate-m-nocp.c
index 4029d7fdd4..694fae7e2e 100644
--- a/target/arm/translate-m-nocp.c
+++ b/target/arm/translate-m-nocp.c
@@ -143,7 +143,7 @@ static bool trans_VSCCLRM(DisasContext *s, arg_VSCCLRM *a)
tcg_gen_brcondi_i32(TCG_COND_EQ, sfpa, 0, s->condlabel);
if (s->fp_excp_el != 0) {
- gen_exception_insn_el(s, s->pc_curr, EXCP_NOCP,
+ gen_exception_insn_el(s, 0, EXCP_NOCP,
syn_uncategorized(), s->fp_excp_el);
return true;
}
@@ -765,12 +765,12 @@ static bool trans_NOCP(DisasContext *s, arg_nocp *a)
}
if (a->cp != 10) {
- gen_exception_insn(s, s->pc_curr, EXCP_NOCP, syn_uncategorized());
+ gen_exception_insn(s, 0, EXCP_NOCP, syn_uncategorized());
return true;
}
if (s->fp_excp_el != 0) {
- gen_exception_insn_el(s, s->pc_curr, EXCP_NOCP,
+ gen_exception_insn_el(s, 0, EXCP_NOCP,
syn_uncategorized(), s->fp_excp_el);
return true;
}
diff --git a/target/arm/translate-mve.c b/target/arm/translate-mve.c
index 0cf1b5ea4f..db7ea3f603 100644
--- a/target/arm/translate-mve.c
+++ b/target/arm/translate-mve.c
@@ -100,7 +100,7 @@ bool mve_eci_check(DisasContext *s)
return true;
default:
/* Reserved value: INVSTATE UsageFault */
- gen_exception_insn(s, s->pc_curr, EXCP_INVSTATE, syn_uncategorized());
+ gen_exception_insn(s, 0, EXCP_INVSTATE, syn_uncategorized());
return false;
}
}
diff --git a/target/arm/translate-vfp.c b/target/arm/translate-vfp.c
index 070f465b17..5c5d58d2c6 100644
--- a/target/arm/translate-vfp.c
+++ b/target/arm/translate-vfp.c
@@ -230,7 +230,7 @@ static bool vfp_access_check_a(DisasContext *s, bool ignore_vfp_enabled)
int coproc = arm_dc_feature(s, ARM_FEATURE_V8) ? 0 : 0xa;
uint32_t syn = syn_fp_access_trap(1, 0xe, false, coproc);
- gen_exception_insn_el(s, s->pc_curr, EXCP_UDEF, syn, s->fp_excp_el);
+ gen_exception_insn_el(s, 0, EXCP_UDEF, syn, s->fp_excp_el);
return false;
}
@@ -240,7 +240,7 @@ static bool vfp_access_check_a(DisasContext *s, bool ignore_vfp_enabled)
* appear to be any insns which touch VFP which are allowed.
*/
if (s->sme_trap_nonstreaming) {
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ gen_exception_insn(s, 0, EXCP_UDEF,
syn_smetrap(SME_ET_Streaming,
curr_insn_len(s) == 2));
return false;
@@ -272,7 +272,7 @@ bool vfp_access_check_m(DisasContext *s, bool skip_context_update)
* the encoding space handled by the patterns in m-nocp.decode,
* and for them we may need to raise NOCP here.
*/
- gen_exception_insn_el(s, s->pc_curr, EXCP_NOCP,
+ gen_exception_insn_el(s, 0, EXCP_NOCP,
syn_uncategorized(), s->fp_excp_el);
return false;
}
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 01b7536c7e..f9d3128656 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1103,32 +1103,34 @@ static void gen_exception(int excp, uint32_t syndrome)
tcg_constant_i32(syndrome));
}
-static void gen_exception_insn_el_v(DisasContext *s, uint64_t pc, int excp,
- uint32_t syn, TCGv_i32 tcg_el)
+static void gen_exception_insn_el_v(DisasContext *s, target_long pc_diff,
+ int excp, uint32_t syn, TCGv_i32 tcg_el)
{
if (s->aarch64) {
- gen_a64_update_pc(s, pc - s->pc_curr);
+ gen_a64_update_pc(s, pc_diff);
} else {
gen_set_condexec(s);
- gen_update_pc(s, pc - s->pc_curr);
+ gen_update_pc(s, pc_diff);
}
gen_exception_el_v(excp, syn, tcg_el);
s->base.is_jmp = DISAS_NORETURN;
}
-void gen_exception_insn_el(DisasContext *s, uint64_t pc, int excp,
+void gen_exception_insn_el(DisasContext *s, target_long pc_diff, int excp,
uint32_t syn, uint32_t target_el)
{
- gen_exception_insn_el_v(s, pc, excp, syn, tcg_constant_i32(target_el));
+ gen_exception_insn_el_v(s, pc_diff, excp, syn,
+ tcg_constant_i32(target_el));
}
-void gen_exception_insn(DisasContext *s, uint64_t pc, int excp, uint32_t syn)
+void gen_exception_insn(DisasContext *s, target_long pc_diff,
+ int excp, uint32_t syn)
{
if (s->aarch64) {
- gen_a64_update_pc(s, pc - s->pc_curr);
+ gen_a64_update_pc(s, pc_diff);
} else {
gen_set_condexec(s);
- gen_update_pc(s, pc - s->pc_curr);
+ gen_update_pc(s, pc_diff);
}
gen_exception(excp, syn);
s->base.is_jmp = DISAS_NORETURN;
@@ -1145,7 +1147,7 @@ static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syn)
void unallocated_encoding(DisasContext *s)
{
/* Unallocated and reserved encodings are uncategorized */
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized());
+ gen_exception_insn(s, 0, EXCP_UDEF, syn_uncategorized());
}
/* Force a TB lookup after an instruction that changes the CPU state. */
@@ -2869,7 +2871,7 @@ static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,
tcg_el = tcg_constant_i32(3);
}
- gen_exception_insn_el_v(s, s->pc_curr, EXCP_UDEF,
+ gen_exception_insn_el_v(s, 0, EXCP_UDEF,
syn_uncategorized(), tcg_el);
tcg_temp_free_i32(tcg_el);
return false;
@@ -2895,7 +2897,7 @@ static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,
undef:
/* If we get here then some access check did not pass */
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized());
+ gen_exception_insn(s, 0, EXCP_UDEF, syn_uncategorized());
return false;
}
@@ -5119,8 +5121,7 @@ static void gen_srs(DisasContext *s,
* For the UNPREDICTABLE cases we choose to UNDEF.
*/
if (s->current_el == 1 && !s->ns && mode == ARM_CPU_MODE_MON) {
- gen_exception_insn_el(s, s->pc_curr, EXCP_UDEF,
- syn_uncategorized(), 3);
+ gen_exception_insn_el(s, 0, EXCP_UDEF, syn_uncategorized(), 3);
return;
}
@@ -8502,7 +8503,7 @@ static bool trans_WLS(DisasContext *s, arg_WLS *a)
* Do the check-and-raise-exception by hand.
*/
if (s->fp_excp_el) {
- gen_exception_insn_el(s, s->pc_curr, EXCP_NOCP,
+ gen_exception_insn_el(s, 0, EXCP_NOCP,
syn_uncategorized(), s->fp_excp_el);
return true;
}
@@ -8605,7 +8606,7 @@ static bool trans_LE(DisasContext *s, arg_LE *a)
tmp = load_cpu_field(v7m.ltpsize);
tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 4, skipexc);
tcg_temp_free_i32(tmp);
- gen_exception_insn(s, s->pc_curr, EXCP_INVSTATE, syn_uncategorized());
+ gen_exception_insn(s, 0, EXCP_INVSTATE, syn_uncategorized());
gen_set_label(skipexc);
}
@@ -9073,7 +9074,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
* UsageFault exception.
*/
if (arm_dc_feature(s, ARM_FEATURE_M)) {
- gen_exception_insn(s, s->pc_curr, EXCP_INVSTATE, syn_uncategorized());
+ gen_exception_insn(s, 0, EXCP_INVSTATE, syn_uncategorized());
return;
}
@@ -9082,7 +9083,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
* Illegal execution state. This has priority over BTI
* exceptions, but comes after instruction abort exceptions.
*/
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_illegalstate());
+ gen_exception_insn(s, 0, EXCP_UDEF, syn_illegalstate());
return;
}
@@ -9647,7 +9648,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
* Illegal execution state. This has priority over BTI
* exceptions, but comes after instruction abort exceptions.
*/
- gen_exception_insn(dc, dc->pc_curr, EXCP_UDEF, syn_illegalstate());
+ gen_exception_insn(dc, 0, EXCP_UDEF, syn_illegalstate());
return;
}
@@ -9720,8 +9721,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
*/
tcg_remove_ops_after(dc->insn_eci_rewind);
dc->condjmp = 0;
- gen_exception_insn(dc, dc->pc_curr, EXCP_INVSTATE,
- syn_uncategorized());
+ gen_exception_insn(dc, 0, EXCP_INVSTATE, syn_uncategorized());
}
arm_post_translate_insn(dc);
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 4/9] target/arm: Change gen_exception_insn* to work on displacements
2022-09-30 22:03 ` [PATCH v5 4/9] target/arm: Change gen_exception_insn* to work on displacements Richard Henderson
@ 2022-10-03 14:21 ` Philippe Mathieu-Daudé via
0 siblings, 0 replies; 24+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-10-03 14:21 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-arm
On 1/10/22 00:03, Richard Henderson wrote:
> In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/translate.h | 5 +++--
> target/arm/translate-a64.c | 28 ++++++++++-------------
> target/arm/translate-m-nocp.c | 6 ++---
> target/arm/translate-mve.c | 2 +-
> target/arm/translate-vfp.c | 6 ++---
> target/arm/translate.c | 42 +++++++++++++++++------------------
> 6 files changed, 43 insertions(+), 46 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 5/9] target/arm: Remove gen_exception_internal_insn pc argument
2022-09-30 22:03 [PATCH v5 0/9] target/arm: pc-relative translation blocks Richard Henderson
` (3 preceding siblings ...)
2022-09-30 22:03 ` [PATCH v5 4/9] target/arm: Change gen_exception_insn* to work on displacements Richard Henderson
@ 2022-09-30 22:03 ` Richard Henderson
2022-10-03 14:22 ` Philippe Mathieu-Daudé via
2022-09-30 22:03 ` [PATCH v5 6/9] target/arm: Change gen_jmp* to work on displacements Richard Henderson
` (3 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2022-09-30 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
Since we always pass dc->pc_curr, fold the arithmetic to zero displacement.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a64.c | 6 +++---
target/arm/translate.c | 10 +++++-----
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 2621b3b36a..005fd767fb 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -340,9 +340,9 @@ static void gen_exception_internal(int excp)
gen_helper_exception_internal(cpu_env, tcg_constant_i32(excp));
}
-static void gen_exception_internal_insn(DisasContext *s, uint64_t pc, int excp)
+static void gen_exception_internal_insn(DisasContext *s, int excp)
{
- gen_a64_update_pc(s, pc - s->pc_curr);
+ gen_a64_update_pc(s, 0);
gen_exception_internal(excp);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -2219,7 +2219,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
* Secondly, "HLT 0xf000" is the A64 semihosting syscall instruction.
*/
if (semihosting_enabled(s->current_el == 0) && imm16 == 0xf000) {
- gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
+ gen_exception_internal_insn(s, EXCP_SEMIHOST);
} else {
unallocated_encoding(s);
}
diff --git a/target/arm/translate.c b/target/arm/translate.c
index f9d3128656..e0b1d415a2 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1078,10 +1078,10 @@ static inline void gen_smc(DisasContext *s)
s->base.is_jmp = DISAS_SMC;
}
-static void gen_exception_internal_insn(DisasContext *s, uint32_t pc, int excp)
+static void gen_exception_internal_insn(DisasContext *s, int excp)
{
gen_set_condexec(s);
- gen_update_pc(s, pc - s->pc_curr);
+ gen_update_pc(s, 0);
gen_exception_internal(excp);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -1173,7 +1173,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
*/
if (semihosting_enabled(s->current_el != 0) &&
(imm == (s->thumb ? 0x3c : 0xf000))) {
- gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
+ gen_exception_internal_insn(s, EXCP_SEMIHOST);
return;
}
@@ -6560,7 +6560,7 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
if (arm_dc_feature(s, ARM_FEATURE_M) &&
semihosting_enabled(s->current_el == 0) &&
(a->imm == 0xab)) {
- gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
+ gen_exception_internal_insn(s, EXCP_SEMIHOST);
} else {
gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
}
@@ -8766,7 +8766,7 @@ static bool trans_SVC(DisasContext *s, arg_SVC *a)
if (!arm_dc_feature(s, ARM_FEATURE_M) &&
semihosting_enabled(s->current_el == 0) &&
(a->imm == semihost_imm)) {
- gen_exception_internal_insn(s, s->pc_curr, EXCP_SEMIHOST);
+ gen_exception_internal_insn(s, EXCP_SEMIHOST);
} else {
gen_update_pc(s, curr_insn_len(s));
s->svc_imm = a->imm;
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 6/9] target/arm: Change gen_jmp* to work on displacements
2022-09-30 22:03 [PATCH v5 0/9] target/arm: pc-relative translation blocks Richard Henderson
` (4 preceding siblings ...)
2022-09-30 22:03 ` [PATCH v5 5/9] target/arm: Remove gen_exception_internal_insn pc argument Richard Henderson
@ 2022-09-30 22:03 ` Richard Henderson
2022-10-04 15:58 ` Peter Maydell
2022-09-30 22:03 ` [PATCH v5 7/9] target/arm: Introduce gen_pc_plus_diff for aarch64 Richard Henderson
` (2 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2022-09-30 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index e0b1d415a2..fd35db8c8c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -270,6 +270,12 @@ static uint32_t read_pc(DisasContext *s)
return s->pc_curr + (s->thumb ? 4 : 8);
}
+/* The pc_curr difference for an architectural jump. */
+static target_long jmp_diff(DisasContext *s, target_long diff)
+{
+ return diff + (s->thumb ? 4 : 8);
+}
+
/* Set a variable to the value of a CPU register. */
void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
{
@@ -2596,7 +2602,7 @@ static void gen_goto_ptr(void)
* cpu_loop_exec. Any live exit_requests will be processed as we
* enter the next TB.
*/
-static void gen_goto_tb(DisasContext *s, int n, int diff)
+static void gen_goto_tb(DisasContext *s, int n, target_long diff)
{
target_ulong dest = s->pc_curr + diff;
@@ -2612,10 +2618,8 @@ static void gen_goto_tb(DisasContext *s, int n, int diff)
}
/* Jump, specifying which TB number to use if we gen_goto_tb() */
-static inline void gen_jmp_tb(DisasContext *s, uint32_t dest, int tbno)
+static void gen_jmp_tb(DisasContext *s, target_long diff, int tbno)
{
- int diff = dest - s->pc_curr;
-
if (unlikely(s->ss_active)) {
/* An indirect jump so that we still trigger the debug exception. */
gen_update_pc(s, diff);
@@ -2657,9 +2661,9 @@ static inline void gen_jmp_tb(DisasContext *s, uint32_t dest, int tbno)
}
}
-static inline void gen_jmp(DisasContext *s, uint32_t dest)
+static inline void gen_jmp(DisasContext *s, target_long diff)
{
- gen_jmp_tb(s, dest, 0);
+ gen_jmp_tb(s, diff, 0);
}
static inline void gen_mulxy(TCGv_i32 t0, TCGv_i32 t1, int x, int y)
@@ -8326,7 +8330,7 @@ static bool trans_CLRM(DisasContext *s, arg_CLRM *a)
static bool trans_B(DisasContext *s, arg_i *a)
{
- gen_jmp(s, read_pc(s) + a->imm);
+ gen_jmp(s, jmp_diff(s, a->imm));
return true;
}
@@ -8341,14 +8345,14 @@ static bool trans_B_cond_thumb(DisasContext *s, arg_ci *a)
return true;
}
arm_skip_unless(s, a->cond);
- gen_jmp(s, read_pc(s) + a->imm);
+ gen_jmp(s, jmp_diff(s, a->imm));
return true;
}
static bool trans_BL(DisasContext *s, arg_i *a)
{
tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | s->thumb);
- gen_jmp(s, read_pc(s) + a->imm);
+ gen_jmp(s, jmp_diff(s, a->imm));
return true;
}
@@ -8368,7 +8372,8 @@ static bool trans_BLX_i(DisasContext *s, arg_BLX_i *a)
}
tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | s->thumb);
store_cpu_field_constant(!s->thumb, thumb);
- gen_jmp(s, (read_pc(s) & ~3) + a->imm);
+ /* This difference computes a page offset so ok for TARGET_TB_PCREL. */
+ gen_jmp(s, (read_pc(s) & ~3) - s->pc_curr + a->imm);
return true;
}
@@ -8529,10 +8534,10 @@ static bool trans_WLS(DisasContext *s, arg_WLS *a)
* when we take this upcoming exit from this TB, so gen_jmp_tb() is OK.
*/
}
- gen_jmp_tb(s, s->base.pc_next, 1);
+ gen_jmp_tb(s, curr_insn_len(s), 1);
gen_set_label(nextlabel);
- gen_jmp(s, read_pc(s) + a->imm);
+ gen_jmp(s, jmp_diff(s, a->imm));
return true;
}
@@ -8612,7 +8617,7 @@ static bool trans_LE(DisasContext *s, arg_LE *a)
if (a->f) {
/* Loop-forever: just jump back to the loop start */
- gen_jmp(s, read_pc(s) - a->imm);
+ gen_jmp(s, jmp_diff(s, -a->imm));
return true;
}
@@ -8643,7 +8648,7 @@ static bool trans_LE(DisasContext *s, arg_LE *a)
tcg_temp_free_i32(decr);
}
/* Jump back to the loop start */
- gen_jmp(s, read_pc(s) - a->imm);
+ gen_jmp(s, jmp_diff(s, -a->imm));
gen_set_label(loopend);
if (a->tp) {
@@ -8651,7 +8656,7 @@ static bool trans_LE(DisasContext *s, arg_LE *a)
store_cpu_field(tcg_constant_i32(4), v7m.ltpsize);
}
/* End TB, continuing to following insn */
- gen_jmp_tb(s, s->base.pc_next, 1);
+ gen_jmp_tb(s, curr_insn_len(s), 1);
return true;
}
@@ -8750,7 +8755,7 @@ static bool trans_CBZ(DisasContext *s, arg_CBZ *a)
tcg_gen_brcondi_i32(a->nz ? TCG_COND_EQ : TCG_COND_NE,
tmp, 0, s->condlabel);
tcg_temp_free_i32(tmp);
- gen_jmp(s, read_pc(s) + a->imm);
+ gen_jmp(s, jmp_diff(s, a->imm));
return true;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 6/9] target/arm: Change gen_jmp* to work on displacements
2022-09-30 22:03 ` [PATCH v5 6/9] target/arm: Change gen_jmp* to work on displacements Richard Henderson
@ 2022-10-04 15:58 ` Peter Maydell
2022-10-04 20:57 ` Richard Henderson
0 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2022-10-04 15:58 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm
On Fri, 30 Sept 2022 at 23:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/translate.c | 37 +++++++++++++++++++++----------------
> 1 file changed, 21 insertions(+), 16 deletions(-)
> @@ -8368,7 +8372,8 @@ static bool trans_BLX_i(DisasContext *s, arg_BLX_i *a)
> }
> tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | s->thumb);
> store_cpu_field_constant(!s->thumb, thumb);
> - gen_jmp(s, (read_pc(s) & ~3) + a->imm);
> + /* This difference computes a page offset so ok for TARGET_TB_PCREL. */
> + gen_jmp(s, (read_pc(s) & ~3) - s->pc_curr + a->imm);
Could we just calculate the offset of the jump target instead?
read_pc() returns s->pc_curr + a constant, so the s->pc_curr cancels
out anyway:
(read_pc(s) & ~3) - s->pc_curr + a->imm
==
(pc_curr + (s->thumb ? 4 : 8) & ~3) - pc_curr + imm
== pc_curr - pc_curr_low_bits - pc_curr + 4-or-8 + imm
== imm + 4-or-8 - low_bits_of_pc
That's then more obviously not dependent on the absolute value
of the PC.
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 6/9] target/arm: Change gen_jmp* to work on displacements
2022-10-04 15:58 ` Peter Maydell
@ 2022-10-04 20:57 ` Richard Henderson
2022-10-05 14:15 ` Peter Maydell
0 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2022-10-04 20:57 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, qemu-arm
On 10/4/22 08:58, Peter Maydell wrote:
> On Fri, 30 Sept 2022 at 23:10, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> target/arm/translate.c | 37 +++++++++++++++++++++----------------
>> 1 file changed, 21 insertions(+), 16 deletions(-)
>
>> @@ -8368,7 +8372,8 @@ static bool trans_BLX_i(DisasContext *s, arg_BLX_i *a)
>> }
>> tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | s->thumb);
>> store_cpu_field_constant(!s->thumb, thumb);
>> - gen_jmp(s, (read_pc(s) & ~3) + a->imm);
>> + /* This difference computes a page offset so ok for TARGET_TB_PCREL. */
>> + gen_jmp(s, (read_pc(s) & ~3) - s->pc_curr + a->imm);
>
> Could we just calculate the offset of the jump target instead?
> read_pc() returns s->pc_curr + a constant, so the s->pc_curr cancels
> out anyway:
>
> (read_pc(s) & ~3) - s->pc_curr + a->imm
> ==
> (pc_curr + (s->thumb ? 4 : 8) & ~3) - pc_curr + imm
> == pc_curr - pc_curr_low_bits - pc_curr + 4-or-8 + imm
> == imm + 4-or-8 - low_bits_of_pc
>
> That's then more obviously not dependent on the absolute value
> of the PC.
Yes, this works:
- gen_jmp(s, (read_pc(s) & ~3) + a->imm);
+ /* This jump is computed from an aligned PC: subtract off the low bits. */
+ gen_jmp(s, jmp_diff(s, a->imm - (s->pc_curr & 3)));
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 6/9] target/arm: Change gen_jmp* to work on displacements
2022-10-04 20:57 ` Richard Henderson
@ 2022-10-05 14:15 ` Peter Maydell
0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2022-10-05 14:15 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm
On Tue, 4 Oct 2022 at 21:57, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/4/22 08:58, Peter Maydell wrote:
> > On Fri, 30 Sept 2022 at 23:10, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >> target/arm/translate.c | 37 +++++++++++++++++++++----------------
> >> 1 file changed, 21 insertions(+), 16 deletions(-)
> >
> >> @@ -8368,7 +8372,8 @@ static bool trans_BLX_i(DisasContext *s, arg_BLX_i *a)
> >> }
> >> tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | s->thumb);
> >> store_cpu_field_constant(!s->thumb, thumb);
> >> - gen_jmp(s, (read_pc(s) & ~3) + a->imm);
> >> + /* This difference computes a page offset so ok for TARGET_TB_PCREL. */
> >> + gen_jmp(s, (read_pc(s) & ~3) - s->pc_curr + a->imm);
> >
> > Could we just calculate the offset of the jump target instead?
> > read_pc() returns s->pc_curr + a constant, so the s->pc_curr cancels
> > out anyway:
> >
> > (read_pc(s) & ~3) - s->pc_curr + a->imm
> > ==
> > (pc_curr + (s->thumb ? 4 : 8) & ~3) - pc_curr + imm
> > == pc_curr - pc_curr_low_bits - pc_curr + 4-or-8 + imm
> > == imm + 4-or-8 - low_bits_of_pc
> >
> > That's then more obviously not dependent on the absolute value
> > of the PC.
>
> Yes, this works:
>
> - gen_jmp(s, (read_pc(s) & ~3) + a->imm);
>
> + /* This jump is computed from an aligned PC: subtract off the low bits. */
>
> + gen_jmp(s, jmp_diff(s, a->imm - (s->pc_curr & 3)));
Cool, that looks a lot clearer. With that change,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 7/9] target/arm: Introduce gen_pc_plus_diff for aarch64
2022-09-30 22:03 [PATCH v5 0/9] target/arm: pc-relative translation blocks Richard Henderson
` (5 preceding siblings ...)
2022-09-30 22:03 ` [PATCH v5 6/9] target/arm: Change gen_jmp* to work on displacements Richard Henderson
@ 2022-09-30 22:03 ` Richard Henderson
2022-10-04 16:10 ` Peter Maydell
2022-09-30 22:03 ` [PATCH v5 8/9] target/arm: Introduce gen_pc_plus_diff for aarch32 Richard Henderson
2022-09-30 22:03 ` [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL Richard Henderson
8 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2022-09-30 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a64.c | 41 +++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 12 deletions(-)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 005fd767fb..28a417fb2b 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -148,9 +148,14 @@ static void reset_btype(DisasContext *s)
}
}
+static void gen_pc_plus_diff(DisasContext *s, TCGv_i64 dest, target_long diff)
+{
+ tcg_gen_movi_i64(dest, s->pc_curr + diff);
+}
+
void gen_a64_update_pc(DisasContext *s, target_long diff)
{
- tcg_gen_movi_i64(cpu_pc, s->pc_curr + diff);
+ gen_pc_plus_diff(s, cpu_pc, diff);
}
/*
@@ -1368,7 +1373,7 @@ static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
if (insn & (1U << 31)) {
/* BL Branch with link */
- tcg_gen_movi_i64(cpu_reg(s, 30), s->base.pc_next);
+ gen_pc_plus_diff(s, cpu_reg(s, 30), curr_insn_len(s));
}
/* B Branch / BL Branch with link */
@@ -2309,11 +2314,17 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
default:
goto do_unallocated;
}
- gen_a64_set_pc(s, dst);
/* BLR also needs to load return address */
if (opc == 1) {
- tcg_gen_movi_i64(cpu_reg(s, 30), s->base.pc_next);
+ TCGv_i64 lr = cpu_reg(s, 30);
+ if (dst == lr) {
+ TCGv_i64 tmp = new_tmp_a64(s);
+ tcg_gen_mov_i64(tmp, dst);
+ dst = tmp;
+ }
+ gen_pc_plus_diff(s, lr, curr_insn_len(s));
}
+ gen_a64_set_pc(s, dst);
break;
case 8: /* BRAA */
@@ -2336,11 +2347,17 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
} else {
dst = cpu_reg(s, rn);
}
- gen_a64_set_pc(s, dst);
/* BLRAA also needs to load return address */
if (opc == 9) {
- tcg_gen_movi_i64(cpu_reg(s, 30), s->base.pc_next);
+ TCGv_i64 lr = cpu_reg(s, 30);
+ if (dst == lr) {
+ TCGv_i64 tmp = new_tmp_a64(s);
+ tcg_gen_mov_i64(tmp, dst);
+ dst = tmp;
+ }
+ gen_pc_plus_diff(s, lr, curr_insn_len(s));
}
+ gen_a64_set_pc(s, dst);
break;
case 4: /* ERET */
@@ -2908,7 +2925,8 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn)
tcg_rt = cpu_reg(s, rt);
- clean_addr = tcg_constant_i64(s->pc_curr + imm);
+ clean_addr = new_tmp_a64(s);
+ gen_pc_plus_diff(s, clean_addr, imm);
if (is_vector) {
do_fp_ld(s, rt, clean_addr, size);
} else {
@@ -4252,23 +4270,22 @@ static void disas_ldst(DisasContext *s, uint32_t insn)
static void disas_pc_rel_adr(DisasContext *s, uint32_t insn)
{
unsigned int page, rd;
- uint64_t base;
- uint64_t offset;
+ int64_t offset;
page = extract32(insn, 31, 1);
/* SignExtend(immhi:immlo) -> offset */
offset = sextract64(insn, 5, 19);
offset = offset << 2 | extract32(insn, 29, 2);
rd = extract32(insn, 0, 5);
- base = s->pc_curr;
if (page) {
/* ADRP (page based) */
- base &= ~0xfff;
offset <<= 12;
+ /* The page offset is ok for TARGET_TB_PCREL. */
+ offset -= s->pc_curr & 0xfff;
}
- tcg_gen_movi_i64(cpu_reg(s, rd), base + offset);
+ gen_pc_plus_diff(s, cpu_reg(s, rd), offset);
}
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 8/9] target/arm: Introduce gen_pc_plus_diff for aarch32
2022-09-30 22:03 [PATCH v5 0/9] target/arm: pc-relative translation blocks Richard Henderson
` (6 preceding siblings ...)
2022-09-30 22:03 ` [PATCH v5 7/9] target/arm: Introduce gen_pc_plus_diff for aarch64 Richard Henderson
@ 2022-09-30 22:03 ` Richard Henderson
2022-10-11 14:51 ` Peter Maydell
2022-09-30 22:03 ` [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL Richard Henderson
8 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2022-09-30 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Philippe Mathieu-Daudé
In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index fd35db8c8c..050da9e740 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -276,11 +276,16 @@ static target_long jmp_diff(DisasContext *s, target_long diff)
return diff + (s->thumb ? 4 : 8);
}
+static void gen_pc_plus_diff(DisasContext *s, TCGv_i32 var, target_long diff)
+{
+ tcg_gen_movi_i32(var, s->pc_curr + diff);
+}
+
/* Set a variable to the value of a CPU register. */
void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
{
if (reg == 15) {
- tcg_gen_movi_i32(var, read_pc(s));
+ gen_pc_plus_diff(s, var, jmp_diff(s, 0));
} else {
tcg_gen_mov_i32(var, cpu_R[reg]);
}
@@ -296,7 +301,8 @@ TCGv_i32 add_reg_for_lit(DisasContext *s, int reg, int ofs)
TCGv_i32 tmp = tcg_temp_new_i32();
if (reg == 15) {
- tcg_gen_movi_i32(tmp, (read_pc(s) & ~3) + ofs);
+ /* This difference computes a page offset so ok for TARGET_TB_PCREL. */
+ gen_pc_plus_diff(s, tmp, (read_pc(s) & ~3) - s->pc_curr + ofs);
} else {
tcg_gen_addi_i32(tmp, cpu_R[reg], ofs);
}
@@ -1159,7 +1165,7 @@ void unallocated_encoding(DisasContext *s)
/* Force a TB lookup after an instruction that changes the CPU state. */
void gen_lookup_tb(DisasContext *s)
{
- tcg_gen_movi_i32(cpu_R[15], s->base.pc_next);
+ gen_pc_plus_diff(s, cpu_R[15], curr_insn_len(s));
s->base.is_jmp = DISAS_EXIT;
}
@@ -6483,7 +6489,7 @@ static bool trans_BLX_r(DisasContext *s, arg_BLX_r *a)
return false;
}
tmp = load_reg(s, a->rm);
- tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | s->thumb);
+ gen_pc_plus_diff(s, cpu_R[14], curr_insn_len(s) | s->thumb);
gen_bx(s, tmp);
return true;
}
@@ -8351,7 +8357,7 @@ static bool trans_B_cond_thumb(DisasContext *s, arg_ci *a)
static bool trans_BL(DisasContext *s, arg_i *a)
{
- tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | s->thumb);
+ gen_pc_plus_diff(s, cpu_R[14], curr_insn_len(s) | s->thumb);
gen_jmp(s, jmp_diff(s, a->imm));
return true;
}
@@ -8370,7 +8376,7 @@ static bool trans_BLX_i(DisasContext *s, arg_BLX_i *a)
if (s->thumb && (a->imm & 2)) {
return false;
}
- tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | s->thumb);
+ gen_pc_plus_diff(s, cpu_R[14], curr_insn_len(s) | s->thumb);
store_cpu_field_constant(!s->thumb, thumb);
/* This difference computes a page offset so ok for TARGET_TB_PCREL. */
gen_jmp(s, (read_pc(s) & ~3) - s->pc_curr + a->imm);
@@ -8380,7 +8386,7 @@ static bool trans_BLX_i(DisasContext *s, arg_BLX_i *a)
static bool trans_BL_BLX_prefix(DisasContext *s, arg_BL_BLX_prefix *a)
{
assert(!arm_dc_feature(s, ARM_FEATURE_THUMB2));
- tcg_gen_movi_i32(cpu_R[14], read_pc(s) + (a->imm << 12));
+ gen_pc_plus_diff(s, cpu_R[14], jmp_diff(s, a->imm << 12));
return true;
}
@@ -8390,7 +8396,7 @@ static bool trans_BL_suffix(DisasContext *s, arg_BL_suffix *a)
assert(!arm_dc_feature(s, ARM_FEATURE_THUMB2));
tcg_gen_addi_i32(tmp, cpu_R[14], (a->imm << 1) | 1);
- tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | 1);
+ gen_pc_plus_diff(s, cpu_R[14], curr_insn_len(s) | 1);
gen_bx(s, tmp);
return true;
}
@@ -8406,7 +8412,7 @@ static bool trans_BLX_suffix(DisasContext *s, arg_BLX_suffix *a)
tmp = tcg_temp_new_i32();
tcg_gen_addi_i32(tmp, cpu_R[14], a->imm << 1);
tcg_gen_andi_i32(tmp, tmp, 0xfffffffc);
- tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | 1);
+ gen_pc_plus_diff(s, cpu_R[14], curr_insn_len(s) | 1);
gen_bx(s, tmp);
return true;
}
@@ -8729,10 +8735,11 @@ static bool op_tbranch(DisasContext *s, arg_tbranch *a, bool half)
tcg_gen_add_i32(addr, addr, tmp);
gen_aa32_ld_i32(s, tmp, addr, get_mem_index(s), half ? MO_UW : MO_UB);
- tcg_temp_free_i32(addr);
tcg_gen_add_i32(tmp, tmp, tmp);
- tcg_gen_addi_i32(tmp, tmp, read_pc(s));
+ gen_pc_plus_diff(s, addr, jmp_diff(s, 0));
+ tcg_gen_add_i32(tmp, tmp, addr);
+ tcg_temp_free_i32(addr);
store_reg(s, 15, tmp);
return true;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 8/9] target/arm: Introduce gen_pc_plus_diff for aarch32
2022-09-30 22:03 ` [PATCH v5 8/9] target/arm: Introduce gen_pc_plus_diff for aarch32 Richard Henderson
@ 2022-10-11 14:51 ` Peter Maydell
2022-10-11 15:52 ` Richard Henderson
0 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2022-10-11 14:51 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm, Philippe Mathieu-Daudé
On Fri, 30 Sept 2022 at 23:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/translate.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index fd35db8c8c..050da9e740 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -276,11 +276,16 @@ static target_long jmp_diff(DisasContext *s, target_long diff)
> return diff + (s->thumb ? 4 : 8);
> }
>
> +static void gen_pc_plus_diff(DisasContext *s, TCGv_i32 var, target_long diff)
> +{
> + tcg_gen_movi_i32(var, s->pc_curr + diff);
> +}
> +
> /* Set a variable to the value of a CPU register. */
> void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
> {
> if (reg == 15) {
> - tcg_gen_movi_i32(var, read_pc(s));
> + gen_pc_plus_diff(s, var, jmp_diff(s, 0));
> } else {
> tcg_gen_mov_i32(var, cpu_R[reg]);
> }
> @@ -296,7 +301,8 @@ TCGv_i32 add_reg_for_lit(DisasContext *s, int reg, int ofs)
> TCGv_i32 tmp = tcg_temp_new_i32();
>
> if (reg == 15) {
> - tcg_gen_movi_i32(tmp, (read_pc(s) & ~3) + ofs);
> + /* This difference computes a page offset so ok for TARGET_TB_PCREL. */
> + gen_pc_plus_diff(s, tmp, (read_pc(s) & ~3) - s->pc_curr + ofs);
We could rephrase this one also to not do the "pc_curr - pc_curr" thing,
the way we did for BLX in patch 6, right ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 8/9] target/arm: Introduce gen_pc_plus_diff for aarch32
2022-10-11 14:51 ` Peter Maydell
@ 2022-10-11 15:52 ` Richard Henderson
0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-10-11 15:52 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, qemu-arm, Philippe Mathieu-Daudé
On 10/11/22 07:51, Peter Maydell wrote:
> On Fri, 30 Sept 2022 at 23:05, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> In preparation for TARGET_TB_PCREL, reduce reliance on absolute values.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> target/arm/translate.c | 29 ++++++++++++++++++-----------
>> 1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index fd35db8c8c..050da9e740 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -276,11 +276,16 @@ static target_long jmp_diff(DisasContext *s, target_long diff)
>> return diff + (s->thumb ? 4 : 8);
>> }
>>
>> +static void gen_pc_plus_diff(DisasContext *s, TCGv_i32 var, target_long diff)
>> +{
>> + tcg_gen_movi_i32(var, s->pc_curr + diff);
>> +}
>> +
>> /* Set a variable to the value of a CPU register. */
>> void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
>> {
>> if (reg == 15) {
>> - tcg_gen_movi_i32(var, read_pc(s));
>> + gen_pc_plus_diff(s, var, jmp_diff(s, 0));
>> } else {
>> tcg_gen_mov_i32(var, cpu_R[reg]);
>> }
>> @@ -296,7 +301,8 @@ TCGv_i32 add_reg_for_lit(DisasContext *s, int reg, int ofs)
>> TCGv_i32 tmp = tcg_temp_new_i32();
>>
>> if (reg == 15) {
>> - tcg_gen_movi_i32(tmp, (read_pc(s) & ~3) + ofs);
>> + /* This difference computes a page offset so ok for TARGET_TB_PCREL. */
>> + gen_pc_plus_diff(s, tmp, (read_pc(s) & ~3) - s->pc_curr + ofs);
>
> We could rephrase this one also to not do the "pc_curr - pc_curr" thing,
> the way we did for BLX in patch 6, right ?
Ah yes. I missed that in revision.
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL
2022-09-30 22:03 [PATCH v5 0/9] target/arm: pc-relative translation blocks Richard Henderson
` (7 preceding siblings ...)
2022-09-30 22:03 ` [PATCH v5 8/9] target/arm: Introduce gen_pc_plus_diff for aarch32 Richard Henderson
@ 2022-09-30 22:03 ` Richard Henderson
2022-10-04 16:23 ` Peter Maydell
8 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2022-09-30 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/cpu-param.h | 1 +
target/arm/translate.h | 19 ++++++++++++
target/arm/cpu.c | 23 +++++++-------
target/arm/translate-a64.c | 37 ++++++++++++++++++-----
target/arm/translate.c | 62 ++++++++++++++++++++++++++++++--------
5 files changed, 112 insertions(+), 30 deletions(-)
diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
index 68ffb12427..29c5fc4241 100644
--- a/target/arm/cpu-param.h
+++ b/target/arm/cpu-param.h
@@ -30,6 +30,7 @@
*/
# define TARGET_PAGE_BITS_VARY
# define TARGET_PAGE_BITS_MIN 10
+# define TARGET_TB_PCREL 1
#endif
#define NB_MMU_MODES 15
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 4aa239e23c..41d14cc067 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -12,6 +12,25 @@ typedef struct DisasContext {
/* The address of the current instruction being translated. */
target_ulong pc_curr;
+ /*
+ * For TARGET_TB_PCREL, the full value of cpu_pc is not known
+ * (although the page offset is known). For convenience, the
+ * translation loop uses the full virtual address that triggered
+ * the translation is used, from base.pc_start through pc_curr.
+ * For efficiency, we do not update cpu_pc for every instruction.
+ * Instead, pc_save has the value of pc_curr at the time of the
+ * last update to cpu_pc, which allows us to compute the addend
+ * needed to bring cpu_pc current: pc_curr - pc_save.
+ * If cpu_pc now contains the destiation of an indirect branch,
+ * pc_save contains -1 to indicate that relative updates are no
+ * longer possible.
+ */
+ target_ulong pc_save;
+ /*
+ * Similarly, pc_cond_save contains the value of pc_save at the
+ * beginning of an AArch32 conditional instruction.
+ */
+ target_ulong pc_cond_save;
target_ulong page_start;
uint32_t insn;
/* Nonzero if this instruction has been conditionally skipped. */
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 94ca6f163f..0bc5e9b125 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -76,17 +76,18 @@ static vaddr arm_cpu_get_pc(CPUState *cs)
void arm_cpu_synchronize_from_tb(CPUState *cs,
const TranslationBlock *tb)
{
- ARMCPU *cpu = ARM_CPU(cs);
- CPUARMState *env = &cpu->env;
-
- /*
- * It's OK to look at env for the current mode here, because it's
- * never possible for an AArch64 TB to chain to an AArch32 TB.
- */
- if (is_a64(env)) {
- env->pc = tb_pc(tb);
- } else {
- env->regs[15] = tb_pc(tb);
+ /* The program counter is always up to date with TARGET_TB_PCREL. */
+ if (!TARGET_TB_PCREL) {
+ CPUARMState *env = cs->env_ptr;
+ /*
+ * It's OK to look at env for the current mode here, because it's
+ * never possible for an AArch64 TB to chain to an AArch32 TB.
+ */
+ if (is_a64(env)) {
+ env->pc = tb_pc(tb);
+ } else {
+ env->regs[15] = tb_pc(tb);
+ }
}
}
#endif /* CONFIG_TCG */
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 28a417fb2b..57cfc9f1a9 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -150,12 +150,18 @@ static void reset_btype(DisasContext *s)
static void gen_pc_plus_diff(DisasContext *s, TCGv_i64 dest, target_long diff)
{
- tcg_gen_movi_i64(dest, s->pc_curr + diff);
+ assert(s->pc_save != -1);
+ if (TARGET_TB_PCREL) {
+ tcg_gen_addi_i64(dest, cpu_pc, (s->pc_curr - s->pc_save) + diff);
+ } else {
+ tcg_gen_movi_i64(dest, s->pc_curr + diff);
+ }
}
void gen_a64_update_pc(DisasContext *s, target_long diff)
{
gen_pc_plus_diff(s, cpu_pc, diff);
+ s->pc_save = s->pc_curr + diff;
}
/*
@@ -209,6 +215,7 @@ static void gen_a64_set_pc(DisasContext *s, TCGv_i64 src)
* then loading an address into the PC will clear out any tag.
*/
gen_top_byte_ignore(s, cpu_pc, src, s->tbii);
+ s->pc_save = -1;
}
/*
@@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)
static void gen_exception_internal_insn(DisasContext *s, int excp)
{
+ target_ulong pc_save = s->pc_save;
+
gen_a64_update_pc(s, 0);
gen_exception_internal(excp);
s->base.is_jmp = DISAS_NORETURN;
+ s->pc_save = pc_save;
}
static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syndrome)
{
+ target_ulong pc_save = s->pc_save;
+
gen_a64_update_pc(s, 0);
gen_helper_exception_bkpt_insn(cpu_env, tcg_constant_i32(syndrome));
s->base.is_jmp = DISAS_NORETURN;
+ s->pc_save = pc_save;
}
static void gen_step_complete_exception(DisasContext *s)
@@ -385,11 +398,16 @@ static inline bool use_goto_tb(DisasContext *s, uint64_t dest)
static void gen_goto_tb(DisasContext *s, int n, int64_t diff)
{
- uint64_t dest = s->pc_curr + diff;
+ target_ulong pc_save = s->pc_save;
- if (use_goto_tb(s, dest)) {
- tcg_gen_goto_tb(n);
- gen_a64_update_pc(s, diff);
+ if (use_goto_tb(s, s->pc_curr + diff)) {
+ if (TARGET_TB_PCREL) {
+ gen_a64_update_pc(s, diff);
+ tcg_gen_goto_tb(n);
+ } else {
+ tcg_gen_goto_tb(n);
+ gen_a64_update_pc(s, diff);
+ }
tcg_gen_exit_tb(s->base.tb, n);
s->base.is_jmp = DISAS_NORETURN;
} else {
@@ -401,6 +419,7 @@ static void gen_goto_tb(DisasContext *s, int n, int64_t diff)
s->base.is_jmp = DISAS_NORETURN;
}
}
+ s->pc_save = pc_save;
}
static void init_tmp_a64_array(DisasContext *s)
@@ -14707,7 +14726,7 @@ static void aarch64_tr_init_disas_context(DisasContextBase *dcbase,
dc->isar = &arm_cpu->isar;
dc->condjmp = 0;
-
+ dc->pc_save = dc->base.pc_first;
dc->aarch64 = true;
dc->thumb = false;
dc->sctlr_b = 0;
@@ -14789,8 +14808,12 @@ static void aarch64_tr_tb_start(DisasContextBase *db, CPUState *cpu)
static void aarch64_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
{
DisasContext *dc = container_of(dcbase, DisasContext, base);
+ target_ulong pc_arg = dc->base.pc_next;
- tcg_gen_insn_start(dc->base.pc_next, 0, 0);
+ if (TARGET_TB_PCREL) {
+ pc_arg &= ~TARGET_PAGE_MASK;
+ }
+ tcg_gen_insn_start(pc_arg, 0, 0);
dc->insn_start = tcg_last_op();
}
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 050da9e740..b4b82dc4a5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -164,6 +164,7 @@ void arm_gen_condlabel(DisasContext *s)
if (!s->condjmp) {
s->condlabel = gen_new_label();
s->condjmp = 1;
+ s->pc_cond_save = s->pc_save;
}
}
@@ -278,7 +279,12 @@ static target_long jmp_diff(DisasContext *s, target_long diff)
static void gen_pc_plus_diff(DisasContext *s, TCGv_i32 var, target_long diff)
{
- tcg_gen_movi_i32(var, s->pc_curr + diff);
+ assert(s->pc_save != -1);
+ if (TARGET_TB_PCREL) {
+ tcg_gen_addi_i32(var, cpu_R[15], (s->pc_curr - s->pc_save) + diff);
+ } else {
+ tcg_gen_movi_i32(var, s->pc_curr + diff);
+ }
}
/* Set a variable to the value of a CPU register. */
@@ -321,6 +327,7 @@ void store_reg(DisasContext *s, int reg, TCGv_i32 var)
*/
tcg_gen_andi_i32(var, var, s->thumb ? ~1 : ~3);
s->base.is_jmp = DISAS_JUMP;
+ s->pc_save = -1;
} else if (reg == 13 && arm_dc_feature(s, ARM_FEATURE_M)) {
/* For M-profile SP bits [1:0] are always zero */
tcg_gen_andi_i32(var, var, ~3);
@@ -786,7 +793,8 @@ void gen_set_condexec(DisasContext *s)
void gen_update_pc(DisasContext *s, target_long diff)
{
- tcg_gen_movi_i32(cpu_R[15], s->pc_curr + diff);
+ gen_pc_plus_diff(s, cpu_R[15], diff);
+ s->pc_save = s->pc_curr + diff;
}
/* Set PC and Thumb state from var. var is marked as dead. */
@@ -796,6 +804,7 @@ static inline void gen_bx(DisasContext *s, TCGv_i32 var)
tcg_gen_andi_i32(cpu_R[15], var, ~1);
tcg_gen_andi_i32(var, var, 1);
store_cpu_field(var, thumb);
+ s->pc_save = -1;
}
/*
@@ -1118,6 +1127,8 @@ static void gen_exception(int excp, uint32_t syndrome)
static void gen_exception_insn_el_v(DisasContext *s, target_long pc_diff,
int excp, uint32_t syn, TCGv_i32 tcg_el)
{
+ target_ulong pc_save = s->pc_save;
+
if (s->aarch64) {
gen_a64_update_pc(s, pc_diff);
} else {
@@ -1126,6 +1137,7 @@ static void gen_exception_insn_el_v(DisasContext *s, target_long pc_diff,
}
gen_exception_el_v(excp, syn, tcg_el);
s->base.is_jmp = DISAS_NORETURN;
+ s->pc_save = pc_save;
}
void gen_exception_insn_el(DisasContext *s, target_long pc_diff, int excp,
@@ -1138,6 +1150,8 @@ void gen_exception_insn_el(DisasContext *s, target_long pc_diff, int excp,
void gen_exception_insn(DisasContext *s, target_long pc_diff,
int excp, uint32_t syn)
{
+ target_ulong pc_save = s->pc_save;
+
if (s->aarch64) {
gen_a64_update_pc(s, pc_diff);
} else {
@@ -1146,6 +1160,7 @@ void gen_exception_insn(DisasContext *s, target_long pc_diff,
}
gen_exception(excp, syn);
s->base.is_jmp = DISAS_NORETURN;
+ s->pc_save = pc_save;
}
static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syn)
@@ -2610,11 +2625,14 @@ static void gen_goto_ptr(void)
*/
static void gen_goto_tb(DisasContext *s, int n, target_long diff)
{
- target_ulong dest = s->pc_curr + diff;
-
- if (translator_use_goto_tb(&s->base, dest)) {
- tcg_gen_goto_tb(n);
- gen_update_pc(s, diff);
+ if (translator_use_goto_tb(&s->base, s->pc_curr + diff)) {
+ if (TARGET_TB_PCREL) {
+ gen_update_pc(s, diff);
+ tcg_gen_goto_tb(n);
+ } else {
+ tcg_gen_goto_tb(n);
+ gen_update_pc(s, diff);
+ }
tcg_gen_exit_tb(s->base.tb, n);
} else {
gen_update_pc(s, diff);
@@ -2626,10 +2644,13 @@ static void gen_goto_tb(DisasContext *s, int n, target_long diff)
/* Jump, specifying which TB number to use if we gen_goto_tb() */
static void gen_jmp_tb(DisasContext *s, target_long diff, int tbno)
{
+ target_ulong pc_save = s->pc_save;
+
if (unlikely(s->ss_active)) {
/* An indirect jump so that we still trigger the debug exception. */
gen_update_pc(s, diff);
s->base.is_jmp = DISAS_JUMP;
+ s->pc_save = pc_save;
return;
}
switch (s->base.is_jmp) {
@@ -2665,6 +2686,7 @@ static void gen_jmp_tb(DisasContext *s, target_long diff, int tbno)
*/
g_assert_not_reached();
}
+ s->pc_save = pc_save;
}
static inline void gen_jmp(DisasContext *s, target_long diff)
@@ -9326,7 +9348,7 @@ static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
dc->isar = &cpu->isar;
dc->condjmp = 0;
-
+ dc->pc_save = dc->base.pc_first;
dc->aarch64 = false;
dc->thumb = EX_TBFLAG_AM32(tb_flags, THUMB);
dc->be_data = EX_TBFLAG_ANY(tb_flags, BE_DATA) ? MO_BE : MO_LE;
@@ -9481,13 +9503,17 @@ static void arm_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
* fields here.
*/
uint32_t condexec_bits;
+ target_ulong pc_arg = dc->base.pc_next;
+ if (TARGET_TB_PCREL) {
+ pc_arg &= ~TARGET_PAGE_MASK;
+ }
if (dc->eci) {
condexec_bits = dc->eci << 4;
} else {
condexec_bits = (dc->condexec_cond << 4) | (dc->condexec_mask >> 1);
}
- tcg_gen_insn_start(dc->base.pc_next, condexec_bits, 0);
+ tcg_gen_insn_start(pc_arg, condexec_bits, 0);
dc->insn_start = tcg_last_op();
}
@@ -9530,7 +9556,10 @@ static bool arm_check_ss_active(DisasContext *dc)
static void arm_post_translate_insn(DisasContext *dc)
{
- if (dc->condjmp && !dc->base.is_jmp) {
+ if (dc->condjmp && dc->base.is_jmp == DISAS_NEXT) {
+ if (dc->pc_save != dc->pc_cond_save) {
+ gen_update_pc(dc, dc->pc_cond_save - dc->pc_save);
+ }
gen_set_label(dc->condlabel);
dc->condjmp = 0;
}
@@ -9860,6 +9889,7 @@ static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
if (dc->condjmp) {
/* "Condition failed" instruction codepath for the branch/trap insn */
+ dc->pc_save = dc->pc_cond_save;
gen_set_label(dc->condlabel);
gen_set_condexec(dc);
if (unlikely(dc->ss_active)) {
@@ -9922,11 +9952,19 @@ void restore_state_to_opc(CPUARMState *env, TranslationBlock *tb,
target_ulong *data)
{
if (is_a64(env)) {
- env->pc = data[0];
+ if (TARGET_TB_PCREL) {
+ env->pc = (env->pc & TARGET_PAGE_MASK) | data[0];
+ } else {
+ env->pc = data[0];
+ }
env->condexec_bits = 0;
env->exception.syndrome = data[2] << ARM_INSN_START_WORD2_SHIFT;
} else {
- env->regs[15] = data[0];
+ if (TARGET_TB_PCREL) {
+ env->regs[15] = (env->regs[15] & TARGET_PAGE_MASK) | data[0];
+ } else {
+ env->regs[15] = data[0];
+ }
env->condexec_bits = data[1];
env->exception.syndrome = data[2] << ARM_INSN_START_WORD2_SHIFT;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL
2022-09-30 22:03 ` [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL Richard Henderson
@ 2022-10-04 16:23 ` Peter Maydell
2022-10-04 19:27 ` Richard Henderson
0 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2022-10-04 16:23 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm
On Fri, 30 Sept 2022 at 23:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/cpu-param.h | 1 +
> target/arm/translate.h | 19 ++++++++++++
> target/arm/cpu.c | 23 +++++++-------
> target/arm/translate-a64.c | 37 ++++++++++++++++++-----
> target/arm/translate.c | 62 ++++++++++++++++++++++++++++++--------
> 5 files changed, 112 insertions(+), 30 deletions(-)
>
> diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
> index 68ffb12427..29c5fc4241 100644
> --- a/target/arm/cpu-param.h
> +++ b/target/arm/cpu-param.h
> @@ -30,6 +30,7 @@
> */
> # define TARGET_PAGE_BITS_VARY
> # define TARGET_PAGE_BITS_MIN 10
> +# define TARGET_TB_PCREL 1
> #endif
>
> #define NB_MMU_MODES 15
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index 4aa239e23c..41d14cc067 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -12,6 +12,25 @@ typedef struct DisasContext {
>
> /* The address of the current instruction being translated. */
> target_ulong pc_curr;
> + /*
> + * For TARGET_TB_PCREL, the full value of cpu_pc is not known
> + * (although the page offset is known). For convenience, the
> + * translation loop uses the full virtual address that triggered
> + * the translation is used, from base.pc_start through pc_curr.
s/ is used//
> + * For efficiency, we do not update cpu_pc for every instruction.
> + * Instead, pc_save has the value of pc_curr at the time of the
> + * last update to cpu_pc, which allows us to compute the addend
> + * needed to bring cpu_pc current: pc_curr - pc_save.
> + * If cpu_pc now contains the destiation of an indirect branch,
"destination"
> + * pc_save contains -1 to indicate that relative updates are no
> + * longer possible.
> + */
> + target_ulong pc_save;
> + /*
> + * Similarly, pc_cond_save contains the value of pc_save at the
> + * beginning of an AArch32 conditional instruction.
> + */
> + target_ulong pc_cond_save;
> target_ulong page_start;
> uint32_t insn;
> /* Nonzero if this instruction has been conditionally skipped. */
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 94ca6f163f..0bc5e9b125 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -76,17 +76,18 @@ static vaddr arm_cpu_get_pc(CPUState *cs)
> void arm_cpu_synchronize_from_tb(CPUState *cs,
> const TranslationBlock *tb)
> {
> - ARMCPU *cpu = ARM_CPU(cs);
> - CPUARMState *env = &cpu->env;
> -
> - /*
> - * It's OK to look at env for the current mode here, because it's
> - * never possible for an AArch64 TB to chain to an AArch32 TB.
> - */
> - if (is_a64(env)) {
> - env->pc = tb_pc(tb);
> - } else {
> - env->regs[15] = tb_pc(tb);
> + /* The program counter is always up to date with TARGET_TB_PCREL. */
I was confused for a bit about this, but it works because
although the synchronize_from_tb hook has a name that implies
it's comparatively general purpose, in fact we use it only
in the special case of "we abandoned execution at the start of
this TB without executing any of it".
> + if (!TARGET_TB_PCREL) {
> + CPUARMState *env = cs->env_ptr;
> + /*
> + * It's OK to look at env for the current mode here, because it's
> + * never possible for an AArch64 TB to chain to an AArch32 TB.
> + */
> + if (is_a64(env)) {
> + env->pc = tb_pc(tb);
> + } else {
> + env->regs[15] = tb_pc(tb);
> + }
> }
> }
> #endif /* CONFIG_TCG */
> @@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)
>
> static void gen_exception_internal_insn(DisasContext *s, int excp)
> {
> + target_ulong pc_save = s->pc_save;
> +
> gen_a64_update_pc(s, 0);
> gen_exception_internal(excp);
> s->base.is_jmp = DISAS_NORETURN;
> + s->pc_save = pc_save;
What is trashing s->pc_save that we have to work around like this,
here and in the other similar changes ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL
2022-10-04 16:23 ` Peter Maydell
@ 2022-10-04 19:27 ` Richard Henderson
2022-10-04 21:09 ` Richard Henderson
2022-10-14 17:49 ` Peter Maydell
0 siblings, 2 replies; 24+ messages in thread
From: Richard Henderson @ 2022-10-04 19:27 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, qemu-arm
On 10/4/22 09:23, Peter Maydell wrote:
>> void arm_cpu_synchronize_from_tb(CPUState *cs,
>> const TranslationBlock *tb)
>> {
>> - ARMCPU *cpu = ARM_CPU(cs);
>> - CPUARMState *env = &cpu->env;
>> -
>> - /*
>> - * It's OK to look at env for the current mode here, because it's
>> - * never possible for an AArch64 TB to chain to an AArch32 TB.
>> - */
>> - if (is_a64(env)) {
>> - env->pc = tb_pc(tb);
>> - } else {
>> - env->regs[15] = tb_pc(tb);
>> + /* The program counter is always up to date with TARGET_TB_PCREL. */
>
> I was confused for a bit about this, but it works because
> although the synchronize_from_tb hook has a name that implies
> it's comparatively general purpose, in fact we use it only
> in the special case of "we abandoned execution at the start of
> this TB without executing any of it".
Correct.
>> @@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)
>>
>> static void gen_exception_internal_insn(DisasContext *s, int excp)
>> {
>> + target_ulong pc_save = s->pc_save;
>> +
>> gen_a64_update_pc(s, 0);
>> gen_exception_internal(excp);
>> s->base.is_jmp = DISAS_NORETURN;
>> + s->pc_save = pc_save;
>
> What is trashing s->pc_save that we have to work around like this,
> here and in the other similar changes ?
gen_a64_update_pc trashes pc_save.
Off of the top of my head, I can't remember what conditionally uses exceptions (single
step?). But the usage pattern that is interesting is
brcond(x, y, L1)
update_pc(disp1);
exit-or-exception.
L1:
update_pc(disp2);
exit-or-exception.
where at L1 we should have the same pc_save value as we did at the brcond. Saving and
restoring around (at least some of) the DISAS_NORETURN points achieves that.
r~
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL
2022-10-04 19:27 ` Richard Henderson
@ 2022-10-04 21:09 ` Richard Henderson
2022-10-14 17:49 ` Peter Maydell
1 sibling, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-10-04 21:09 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, qemu-arm
On 10/4/22 12:27, Richard Henderson wrote:
> On 10/4/22 09:23, Peter Maydell wrote:
>>> void arm_cpu_synchronize_from_tb(CPUState *cs,
>>> const TranslationBlock *tb)
>>> {
>>> - ARMCPU *cpu = ARM_CPU(cs);
>>> - CPUARMState *env = &cpu->env;
>>> -
>>> - /*
>>> - * It's OK to look at env for the current mode here, because it's
>>> - * never possible for an AArch64 TB to chain to an AArch32 TB.
>>> - */
>>> - if (is_a64(env)) {
>>> - env->pc = tb_pc(tb);
>>> - } else {
>>> - env->regs[15] = tb_pc(tb);
>>> + /* The program counter is always up to date with TARGET_TB_PCREL. */
>>
>> I was confused for a bit about this, but it works because
>> although the synchronize_from_tb hook has a name that implies
>> it's comparatively general purpose, in fact we use it only
>> in the special case of "we abandoned execution at the start of
>> this TB without executing any of it".
>
> Correct.
>
>>> @@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)
>>>
>>> static void gen_exception_internal_insn(DisasContext *s, int excp)
>>> {
>>> + target_ulong pc_save = s->pc_save;
>>> +
>>> gen_a64_update_pc(s, 0);
>>> gen_exception_internal(excp);
>>> s->base.is_jmp = DISAS_NORETURN;
>>> + s->pc_save = pc_save;
>>
>> What is trashing s->pc_save that we have to work around like this,
>> here and in the other similar changes ?
>
> gen_a64_update_pc trashes pc_save.
>
> Off of the top of my head, I can't remember what conditionally uses exceptions (single
> step?).
Oh, duh, any conditional a32 instruction.
To some extent this instance duplicates s->pc_cond_save, but the usage pattern there is
brcond(..., s->condlabel);
s->pc_cond_save = s->pc_save;
gen_update_pc(s, 0); /* pc_save = pc_curr */
raise_exception;
if (s->pc_cond_save != s->pc_save) {
gen_update_pc(s->pc_save - s->pc_cond_save);
}
/* s->pc_save now matches the state at brcond */
condlabel:
So, we have exited the TB via exception, and the second gen_update_pc would be deleted as
dead code, it's just as easy to keep s->pc_save unchanged so that the second gen_update_pc
is not emitted. We certainly *must* update s->pc_save around indirect branches, so that
we don't wind up with an assert on s->pc_save != -1.
r~
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL
2022-10-04 19:27 ` Richard Henderson
2022-10-04 21:09 ` Richard Henderson
@ 2022-10-14 17:49 ` Peter Maydell
2022-10-14 19:01 ` Richard Henderson
1 sibling, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2022-10-14 17:49 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm
On Tue, 4 Oct 2022 at 20:27, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/4/22 09:23, Peter Maydell wrote:
> >> @@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)
> >>
> >> static void gen_exception_internal_insn(DisasContext *s, int excp)
> >> {
> >> + target_ulong pc_save = s->pc_save;
> >> +
> >> gen_a64_update_pc(s, 0);
> >> gen_exception_internal(excp);
> >> s->base.is_jmp = DISAS_NORETURN;
> >> + s->pc_save = pc_save;
> >
> > What is trashing s->pc_save that we have to work around like this,
> > here and in the other similar changes ?
>
> gen_a64_update_pc trashes pc_save.
>
> Off of the top of my head, I can't remember what conditionally uses exceptions (single
> step?). But the usage pattern that is interesting is
>
> brcond(x, y, L1)
> update_pc(disp1);
> exit-or-exception.
> L1:
> update_pc(disp2);
> exit-or-exception.
>
> where at L1 we should have the same pc_save value as we did at the brcond. Saving and
> restoring around (at least some of) the DISAS_NORETURN points achieves that.
(I figured it would be easiest to continue this conversation
in this thread rather than breaking it up to reply to the v6
equivalent patch.)
I guess it does, but it feels like a weird place to be doing that.
If what we want is "at the label L1 we know the pc_save value
needs to be some specific thing", then shouldn't we
achieve that by setting pc_save to a specific value at that
point? e.g. wrapping gen_set_label() with a function that
does "set the label here, guest PC value is going to be this".
Which should generally be the save_pc value at the point
where you emit the brcond() to this label, right? Maybe you
could even have a little struct that wraps up "TCGLabel*
and the pc_save value associated with a branch to it".
But anyway, I think the less confusing way to handle this is
that the changes to pc_save happen at the label, because that's
where the runtime flow-of-control is already being dealt with.
Also, I think that you need to do something for the case
in translate.c where we call tcg_remove_ops_after() --
that now needs to fix pc_save back up to the value it had
when we called tcg_last_op(). (There is also one of those
in target/i386, I haven't checked whether the i386 pcrel
handling series took care of that.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL
2022-10-14 17:49 ` Peter Maydell
@ 2022-10-14 19:01 ` Richard Henderson
0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-10-14 19:01 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, qemu-arm
On 10/15/22 04:49, Peter Maydell wrote:
> On Tue, 4 Oct 2022 at 20:27, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 10/4/22 09:23, Peter Maydell wrote:
>>>> @@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)
>>>>
>>>> static void gen_exception_internal_insn(DisasContext *s, int excp)
>>>> {
>>>> + target_ulong pc_save = s->pc_save;
>>>> +
>>>> gen_a64_update_pc(s, 0);
>>>> gen_exception_internal(excp);
>>>> s->base.is_jmp = DISAS_NORETURN;
>>>> + s->pc_save = pc_save;
>>>
>>> What is trashing s->pc_save that we have to work around like this,
>>> here and in the other similar changes ?
>>
>> gen_a64_update_pc trashes pc_save.
>>
>> Off of the top of my head, I can't remember what conditionally uses exceptions (single
>> step?). But the usage pattern that is interesting is
>>
>> brcond(x, y, L1)
>> update_pc(disp1);
>> exit-or-exception.
>> L1:
>> update_pc(disp2);
>> exit-or-exception.
>>
>> where at L1 we should have the same pc_save value as we did at the brcond. Saving and
>> restoring around (at least some of) the DISAS_NORETURN points achieves that.
>
> (I figured it would be easiest to continue this conversation
> in this thread rather than breaking it up to reply to the v6
> equivalent patch.)
>
> I guess it does, but it feels like a weird place to be doing that.
> If what we want is "at the label L1 we know the pc_save value
> needs to be some specific thing", then shouldn't we
> achieve that by setting pc_save to a specific value at that
> point? e.g. wrapping gen_set_label() with a function that
> does "set the label here, guest PC value is going to be this".
> Which should generally be the save_pc value at the point
> where you emit the brcond() to this label, right? Maybe you
> could even have a little struct that wraps up "TCGLabel*
> and the pc_save value associated with a branch to it".
> But anyway, I think the less confusing way to handle this is
> that the changes to pc_save happen at the label, because that's
> where the runtime flow-of-control is already being dealt with.
Ok, I'll re-work this.
> Also, I think that you need to do something for the case
> in translate.c where we call tcg_remove_ops_after() --
> that now needs to fix pc_save back up to the value it had
> when we called tcg_last_op().
Yes.
> (There is also one of those
> in target/i386, I haven't checked whether the i386 pcrel
> handling series took care of that.)
I'll double-check that case too, thanks.
r~
^ permalink raw reply [flat|nested] 24+ messages in thread