* [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches
@ 2019-08-07 4:53 Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 01/11] target/arm: Pass in pc to thumb_insn_is_16bit Richard Henderson
` (11 more replies)
0 siblings, 12 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
These are split out of my decodetree conversion of the
aarch32 general instructions. With one exception, these
are all related to cleaning up how we refer to "PC".
r~
Richard Henderson (11):
target/arm: Pass in pc to thumb_insn_is_16bit
target/arm: Introduce pc_curr
target/arm: Introduce read_pc
target/arm: Introduce add_reg_for_lit
target/arm: Remove redundant s->pc & ~1
target/arm: Replace s->pc with s->base.pc_next
target/arm: Replace offset with pc in gen_exception_insn
target/arm: Replace offset with pc in gen_exception_internal_insn
target/arm: Remove offset argument to gen_exception_bkpt_insn
target/arm: Use unallocated_encoding for aarch32
target/arm: Remove helper_double_saturate
target/arm/helper.h | 1 -
target/arm/translate-a64.h | 4 +-
target/arm/translate.h | 5 +-
target/arm/op_helper.c | 15 --
target/arm/translate-a64.c | 109 +++++----
target/arm/translate-vfp.inc.c | 45 +---
target/arm/translate.c | 397 +++++++++++++++------------------
7 files changed, 249 insertions(+), 327 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 01/11] target/arm: Pass in pc to thumb_insn_is_16bit
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-08 5:47 ` Philippe Mathieu-Daudé
2019-08-07 4:53 ` [Qemu-devel] [PATCH 02/11] target/arm: Introduce pc_curr Richard Henderson
` (10 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
This function is used in two different contexts, and it will be
clearer if the function is given the address to which it applies.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 7853462b21..1f15f14022 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9261,11 +9261,11 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
}
}
-static bool thumb_insn_is_16bit(DisasContext *s, uint32_t insn)
+static bool thumb_insn_is_16bit(DisasContext *s, uint32_t pc, uint32_t insn)
{
- /* Return true if this is a 16 bit instruction. We must be precise
- * about this (matching the decode). We assume that s->pc still
- * points to the first 16 bits of the insn.
+ /*
+ * Return true if this is a 16 bit instruction. We must be precise
+ * about this (matching the decode).
*/
if ((insn >> 11) < 0x1d) {
/* Definitely a 16-bit instruction */
@@ -9285,7 +9285,7 @@ static bool thumb_insn_is_16bit(DisasContext *s, uint32_t insn)
return false;
}
- if ((insn >> 11) == 0x1e && s->pc - s->page_start < TARGET_PAGE_SIZE - 3) {
+ if ((insn >> 11) == 0x1e && pc - s->page_start < TARGET_PAGE_SIZE - 3) {
/* 0b1111_0xxx_xxxx_xxxx : BL/BLX prefix, and the suffix
* is not on the next page; we merge this into a 32-bit
* insn.
@@ -11824,7 +11824,7 @@ static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
*/
uint16_t insn = arm_lduw_code(env, s->pc, s->sctlr_b);
- return !thumb_insn_is_16bit(s, insn);
+ return !thumb_insn_is_16bit(s, s->pc, insn);
}
static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
@@ -12122,7 +12122,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
}
insn = arm_lduw_code(env, dc->pc, dc->sctlr_b);
- is_16bit = thumb_insn_is_16bit(dc, insn);
+ is_16bit = thumb_insn_is_16bit(dc, dc->pc, insn);
dc->pc += 2;
if (!is_16bit) {
uint32_t insn2 = arm_lduw_code(env, dc->pc, dc->sctlr_b);
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 02/11] target/arm: Introduce pc_curr
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 01/11] target/arm: Pass in pc to thumb_insn_is_16bit Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc Richard Henderson
` (9 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
Add a new field to retain the address of the instruction currently
being translated. The 32-bit uses are all within subroutines used
by a32 and t32. This will become less obvious when t16 support is
merged with a32+t32, and having a clear definition will help.
Convert aarch64 as well for consistency. Note that there is one
instance of a pre-assert fprintf that used the wrong value for the
address of the current instruction.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a64.h | 2 +-
target/arm/translate.h | 2 ++
target/arm/translate-a64.c | 21 +++++++++++----------
target/arm/translate.c | 14 ++++++++------
4 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
index 9ab40872d8..9cd2b3d238 100644
--- a/target/arm/translate-a64.h
+++ b/target/arm/translate-a64.h
@@ -25,7 +25,7 @@ void unallocated_encoding(DisasContext *s);
qemu_log_mask(LOG_UNIMP, \
"%s:%d: unsupported instruction encoding 0x%08x " \
"at pc=%016" PRIx64 "\n", \
- __FILE__, __LINE__, insn, s->pc - 4); \
+ __FILE__, __LINE__, insn, s->pc_curr); \
unallocated_encoding(s); \
} while (0)
diff --git a/target/arm/translate.h b/target/arm/translate.h
index a20f6e2056..525603eb30 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -10,6 +10,8 @@ typedef struct DisasContext {
const ARMISARegisters *isar;
target_ulong pc;
+ /* The address of the current instruction being translated. */
+ target_ulong pc_curr;
target_ulong page_start;
uint32_t insn;
/* Nonzero if this instruction has been conditionally skipped. */
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index d3231477a2..da447eedcc 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1248,7 +1248,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 + sextract32(insn, 0, 26) * 4 - 4;
+ uint64_t addr = s->pc_curr + sextract32(insn, 0, 26) * 4;
if (insn & (1U << 31)) {
/* BL Branch with link */
@@ -1276,7 +1276,7 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t insn)
sf = extract32(insn, 31, 1);
op = extract32(insn, 24, 1); /* 0: CBZ; 1: CBNZ */
rt = extract32(insn, 0, 5);
- addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
+ addr = s->pc_curr + sextract32(insn, 5, 19) * 4;
tcg_cmp = read_cpu_reg(s, rt, sf);
label_match = gen_new_label();
@@ -1305,7 +1305,7 @@ static void disas_test_b_imm(DisasContext *s, uint32_t insn)
bit_pos = (extract32(insn, 31, 1) << 5) | extract32(insn, 19, 5);
op = extract32(insn, 24, 1); /* 0: TBZ; 1: TBNZ */
- addr = s->pc + sextract32(insn, 5, 14) * 4 - 4;
+ addr = s->pc_curr + sextract32(insn, 5, 14) * 4;
rt = extract32(insn, 0, 5);
tcg_cmp = tcg_temp_new_i64();
@@ -1336,7 +1336,7 @@ static void disas_cond_b_imm(DisasContext *s, uint32_t insn)
unallocated_encoding(s);
return;
}
- addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
+ addr = s->pc_curr + sextract32(insn, 5, 19) * 4;
cond = extract32(insn, 0, 4);
reset_btype(s);
@@ -1720,7 +1720,7 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
TCGv_i32 tcg_syn, tcg_isread;
uint32_t syndrome;
- gen_a64_set_pc_im(s->pc - 4);
+ gen_a64_set_pc_im(s->pc_curr);
tmpptr = tcg_const_ptr(ri);
syndrome = syn_aa64_sysregtrap(op0, op1, op2, crn, crm, rt, isread);
tcg_syn = tcg_const_i32(syndrome);
@@ -1884,7 +1884,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 - 4);
+ gen_a64_set_pc_im(s->pc_curr);
gen_helper_pre_hvc(cpu_env);
gen_ss_advance(s);
gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
@@ -1894,7 +1894,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
unallocated_encoding(s);
break;
}
- gen_a64_set_pc_im(s->pc - 4);
+ gen_a64_set_pc_im(s->pc_curr);
tmp = tcg_const_i32(syn_aa64_smc(imm16));
gen_helper_pre_smc(cpu_env, tmp);
tcg_temp_free_i32(tmp);
@@ -2615,7 +2615,7 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn)
tcg_rt = cpu_reg(s, rt);
- clean_addr = tcg_const_i64((s->pc - 4) + imm);
+ clean_addr = tcg_const_i64(s->pc_curr + imm);
if (is_vector) {
do_fp_ld(s, rt, clean_addr, size);
} else {
@@ -3594,7 +3594,7 @@ static void disas_pc_rel_adr(DisasContext *s, uint32_t insn)
offset = sextract64(insn, 5, 19);
offset = offset << 2 | extract32(insn, 29, 2);
rd = extract32(insn, 0, 5);
- base = s->pc - 4;
+ base = s->pc_curr;
if (page) {
/* ADRP (page based) */
@@ -11533,7 +11533,7 @@ static void disas_simd_three_reg_same_fp16(DisasContext *s, uint32_t insn)
break;
default:
fprintf(stderr, "%s: insn %#04x, fpop %#2x @ %#" PRIx64 "\n",
- __func__, insn, fpopcode, s->pc);
+ __func__, insn, fpopcode, s->pc_curr);
g_assert_not_reached();
}
@@ -14044,6 +14044,7 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
{
uint32_t insn;
+ s->pc_curr = s->pc;
insn = arm_ldl_code(env, s->pc, s->sctlr_b);
s->insn = insn;
s->pc += 4;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 1f15f14022..59e35aafbf 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1212,7 +1212,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 - 4);
+ gen_set_pc_im(s, s->pc_curr);
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
@@ -1231,7 +1231,7 @@ static inline void gen_smc(DisasContext *s)
*/
TCGv_i32 tmp;
- gen_set_pc_im(s, s->pc - 4);
+ gen_set_pc_im(s, s->pc_curr);
tmp = tcg_const_i32(syn_aa32_smc());
gen_helper_pre_smc(cpu_env, tmp);
tcg_temp_free_i32(tmp);
@@ -3190,7 +3190,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 - 4);
+ gen_set_pc_im(s, s->pc_curr);
tcg_reg = load_reg(s, rn);
tcg_tgtmode = tcg_const_i32(tgtmode);
tcg_regno = tcg_const_i32(regno);
@@ -3212,7 +3212,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 - 4);
+ gen_set_pc_im(s, s->pc_curr);
tcg_reg = tcg_temp_new_i32();
tcg_tgtmode = tcg_const_i32(tgtmode);
tcg_regno = tcg_const_i32(regno);
@@ -7219,7 +7219,7 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
}
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc - 4);
+ gen_set_pc_im(s, s->pc_curr);
tmpptr = tcg_const_ptr(ri);
tcg_syn = tcg_const_i32(syndrome);
tcg_isread = tcg_const_i32(isread);
@@ -7629,7 +7629,7 @@ static void gen_srs(DisasContext *s,
tmp = tcg_const_i32(mode);
/* get_r13_banked() will raise an exception if called from System mode */
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc - 4);
+ gen_set_pc_im(s, s->pc_curr);
gen_helper_get_r13_banked(addr, cpu_env, tmp);
tcg_temp_free_i32(tmp);
switch (amode) {
@@ -12053,6 +12053,7 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
return;
}
+ dc->pc_curr = dc->pc;
insn = arm_ldl_code(env, dc->pc, dc->sctlr_b);
dc->insn = insn;
dc->pc += 4;
@@ -12121,6 +12122,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
return;
}
+ dc->pc_curr = dc->pc;
insn = arm_lduw_code(env, dc->pc, dc->sctlr_b);
is_16bit = thumb_insn_is_16bit(dc, dc->pc, insn);
dc->pc += 2;
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 01/11] target/arm: Pass in pc to thumb_insn_is_16bit Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 02/11] target/arm: Introduce pc_curr Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-07 17:27 ` Peter Maydell
2019-08-07 4:53 ` [Qemu-devel] [PATCH 04/11] target/arm: Introduce add_reg_for_lit Richard Henderson
` (8 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
We currently have 3 different ways of computing the architectural
value of "PC" as seen in the ARM ARM.
The value of s->pc has been incremented past the current insn,
but that is all. Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;
for t16, PC = s->pc + 2. These differing computations make it
impossible at present to unify the various code paths.
With the newly introduced s->pc_curr, we can compute the correct
value for all cases, using the formula given in the ARM ARM.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate.c | 59 ++++++++++++++++--------------------------
1 file changed, 23 insertions(+), 36 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 59e35aafbf..61933865d5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -196,17 +196,17 @@ static inline void store_cpu_offset(TCGv_i32 var, int offset)
#define store_cpu_field(var, name) \
store_cpu_offset(var, offsetof(CPUARMState, name))
+/* The architectural value of PC. */
+static uint32_t read_pc(DisasContext *s)
+{
+ return s->pc_curr + (s->thumb ? 4 : 8);
+}
+
/* Set a variable to the value of a CPU register. */
static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
{
if (reg == 15) {
- uint32_t addr;
- /* normally, since we updated PC, we need only to add one insn */
- if (s->thumb)
- addr = (long)s->pc + 2;
- else
- addr = (long)s->pc + 4;
- tcg_gen_movi_i32(var, addr);
+ tcg_gen_movi_i32(var, read_pc(s));
} else {
tcg_gen_mov_i32(var, cpu_R[reg]);
}
@@ -7868,16 +7868,14 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
/* branch link and change to thumb (blx <offset>) */
int32_t offset;
- val = (uint32_t)s->pc;
tmp = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp, val);
+ tcg_gen_movi_i32(tmp, s->pc);
store_reg(s, 14, tmp);
/* Sign-extend the 24-bit offset */
offset = (((int32_t)insn) << 8) >> 8;
+ val = read_pc(s);
/* offset * 4 + bit24 * 2 + (thumb bit) */
val += (offset << 2) | ((insn >> 23) & 2) | 1;
- /* pipeline offset */
- val += 4;
/* protected by ARCH(5); above, near the start of uncond block */
gen_bx_im(s, val);
return;
@@ -9153,10 +9151,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
} else {
/* store */
if (i == 15) {
- /* special case: r15 = PC + 8 */
- val = (long)s->pc + 4;
tmp = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp, val);
+ tcg_gen_movi_i32(tmp, read_pc(s));
} else if (user) {
tmp = tcg_temp_new_i32();
tmp2 = tcg_const_i32(i);
@@ -9222,15 +9218,13 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
int32_t offset;
/* branch (and link) */
- val = (int32_t)s->pc;
if (insn & (1 << 24)) {
tmp = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp, val);
+ tcg_gen_movi_i32(tmp, s->pc);
store_reg(s, 14, tmp);
}
offset = sextract32(insn << 2, 0, 26);
- val += offset + 4;
- gen_jmp(s, val);
+ gen_jmp(s, read_pc(s) + offset);
}
break;
case 0xc:
@@ -9588,12 +9582,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
tcg_temp_free_i32(addr);
} else if ((insn & (7 << 5)) == 0) {
/* Table Branch. */
- if (rn == 15) {
- addr = tcg_temp_new_i32();
- tcg_gen_movi_i32(addr, s->pc);
- } else {
- addr = load_reg(s, rn);
- }
+ addr = load_reg(s, rn);
tmp = load_reg(s, rm);
tcg_gen_add_i32(addr, addr, tmp);
if (insn & (1 << 4)) {
@@ -9609,7 +9598,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
}
tcg_temp_free_i32(addr);
tcg_gen_shli_i32(tmp, tmp, 1);
- tcg_gen_addi_i32(tmp, tmp, s->pc);
+ tcg_gen_addi_i32(tmp, tmp, read_pc(s));
store_reg(s, 15, tmp);
} else {
bool is_lasr = false;
@@ -10342,7 +10331,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
tcg_gen_movi_i32(cpu_R[14], s->pc | 1);
}
- offset += s->pc;
+ offset += read_pc(s);
if (insn & (1 << 12)) {
/* b/bl */
gen_jmp(s, offset);
@@ -10583,7 +10572,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
offset |= (insn & (1 << 11)) << 8;
/* jump to the offset */
- gen_jmp(s, s->pc + offset);
+ gen_jmp(s, read_pc(s) + offset);
}
} else {
/*
@@ -11077,7 +11066,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
if (insn & (1 << 11)) {
rd = (insn >> 8) & 7;
/* load pc-relative. Bit 1 of PC is ignored. */
- val = s->pc + 2 + ((insn & 0xff) * 4);
+ val = read_pc(s) + ((insn & 0xff) * 4);
val &= ~(uint32_t)2;
addr = tcg_temp_new_i32();
tcg_gen_movi_i32(addr, val);
@@ -11464,7 +11453,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
} else {
/* PC. bit 1 is ignored. */
tmp = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp, (s->pc + 2) & ~(uint32_t)2);
+ tcg_gen_movi_i32(tmp, read_pc(s) & ~(uint32_t)2);
}
val = (insn & 0xff) * 4;
tcg_gen_addi_i32(tmp, tmp, val);
@@ -11584,9 +11573,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
tcg_gen_brcondi_i32(TCG_COND_NE, tmp, 0, s->condlabel);
tcg_temp_free_i32(tmp);
offset = ((insn & 0xf8) >> 2) | (insn & 0x200) >> 3;
- val = (uint32_t)s->pc + 2;
- val += offset;
- gen_jmp(s, val);
+ gen_jmp(s, read_pc(s) + offset);
break;
case 15: /* IT, nop-hint. */
@@ -11750,7 +11737,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
arm_skip_unless(s, cond);
/* jump to the offset */
- val = (uint32_t)s->pc + 2;
+ val = read_pc(s);
offset = ((int32_t)insn << 24) >> 24;
val += offset << 1;
gen_jmp(s, val);
@@ -11776,9 +11763,9 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
break;
}
/* unconditional branch */
- val = (uint32_t)s->pc;
+ val = read_pc(s);
offset = ((int32_t)insn << 21) >> 21;
- val += (offset << 1) + 2;
+ val += offset << 1;
gen_jmp(s, val);
break;
@@ -11802,7 +11789,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
/* 0b1111_0xxx_xxxx_xxxx : BL/BLX prefix */
uint32_t uoffset = ((int32_t)insn << 21) >> 9;
- tcg_gen_movi_i32(cpu_R[14], s->pc + 2 + uoffset);
+ tcg_gen_movi_i32(cpu_R[14], read_pc(s) + uoffset);
}
break;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 04/11] target/arm: Introduce add_reg_for_lit
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
` (2 preceding siblings ...)
2019-08-07 4:53 ` [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-08 5:43 ` Philippe Mathieu-Daudé
2019-08-07 4:53 ` [Qemu-devel] [PATCH 05/11] target/arm: Remove redundant s->pc & ~1 Richard Henderson
` (7 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
Provide a common routine for the places that require ALIGN(PC, 4)
as the base address as opposed to plain PC. The two are always
the same for A32, but the difference is meaningful for thumb mode.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Note: This patch is easier to read with -b, as there are several
large-ish indentation changes as the if statements fold together.
---
target/arm/translate-vfp.inc.c | 38 ++------
target/arm/translate.c | 166 +++++++++++++++------------------
2 files changed, 82 insertions(+), 122 deletions(-)
diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
index 092eb5ec53..262d4177e5 100644
--- a/target/arm/translate-vfp.inc.c
+++ b/target/arm/translate-vfp.inc.c
@@ -941,14 +941,8 @@ static bool trans_VLDR_VSTR_sp(DisasContext *s, arg_VLDR_VSTR_sp *a)
offset = -offset;
}
- if (s->thumb && a->rn == 15) {
- /* This is actually UNPREDICTABLE */
- addr = tcg_temp_new_i32();
- tcg_gen_movi_i32(addr, s->pc & ~2);
- } else {
- addr = load_reg(s, a->rn);
- }
- tcg_gen_addi_i32(addr, addr, offset);
+ /* For thumb, use of PC is UNPREDICTABLE. */
+ addr = add_reg_for_lit(s, a->rn, offset);
tmp = tcg_temp_new_i32();
if (a->l) {
gen_aa32_ld32u(s, tmp, addr, get_mem_index(s));
@@ -983,14 +977,8 @@ static bool trans_VLDR_VSTR_dp(DisasContext *s, arg_VLDR_VSTR_dp *a)
offset = -offset;
}
- if (s->thumb && a->rn == 15) {
- /* This is actually UNPREDICTABLE */
- addr = tcg_temp_new_i32();
- tcg_gen_movi_i32(addr, s->pc & ~2);
- } else {
- addr = load_reg(s, a->rn);
- }
- tcg_gen_addi_i32(addr, addr, offset);
+ /* For thumb, use of PC is UNPREDICTABLE. */
+ addr = add_reg_for_lit(s, a->rn, offset);
tmp = tcg_temp_new_i64();
if (a->l) {
gen_aa32_ld64(s, tmp, addr, get_mem_index(s));
@@ -1029,13 +1017,8 @@ static bool trans_VLDM_VSTM_sp(DisasContext *s, arg_VLDM_VSTM_sp *a)
return true;
}
- if (s->thumb && a->rn == 15) {
- /* This is actually UNPREDICTABLE */
- addr = tcg_temp_new_i32();
- tcg_gen_movi_i32(addr, s->pc & ~2);
- } else {
- addr = load_reg(s, a->rn);
- }
+ /* For thumb, use of PC is UNPREDICTABLE. */
+ addr = add_reg_for_lit(s, a->rn, 0);
if (a->p) {
/* pre-decrement */
tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
@@ -1112,13 +1095,8 @@ static bool trans_VLDM_VSTM_dp(DisasContext *s, arg_VLDM_VSTM_dp *a)
return true;
}
- if (s->thumb && a->rn == 15) {
- /* This is actually UNPREDICTABLE */
- addr = tcg_temp_new_i32();
- tcg_gen_movi_i32(addr, s->pc & ~2);
- } else {
- addr = load_reg(s, a->rn);
- }
+ /* For thumb, use of PC is UNPREDICTABLE. */
+ addr = add_reg_for_lit(s, a->rn, 0);
if (a->p) {
/* pre-decrement */
tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 61933865d5..71d94c053b 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -220,6 +220,23 @@ static inline TCGv_i32 load_reg(DisasContext *s, int reg)
return tmp;
}
+/*
+ * Create a new temp, REG + OFS, except PC is ALIGN(PC, 4).
+ * This is used for load/store for which use of PC implies (literal),
+ * or ADD that implies ADR.
+ */
+static 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);
+ } else {
+ tcg_gen_addi_i32(tmp, cpu_R[reg], ofs);
+ }
+ return tmp;
+}
+
/* Set a CPU register. The source must be a temporary and will be
marked as dead. */
static void store_reg(DisasContext *s, int reg, TCGv_i32 var)
@@ -9472,16 +9489,12 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
*/
bool wback = extract32(insn, 21, 1);
- if (rn == 15) {
- if (insn & (1 << 21)) {
- /* UNPREDICTABLE */
- goto illegal_op;
- }
- addr = tcg_temp_new_i32();
- tcg_gen_movi_i32(addr, s->pc & ~3);
- } else {
- addr = load_reg(s, rn);
+ if (rn == 15 && (insn & (1 << 21))) {
+ /* UNPREDICTABLE */
+ goto illegal_op;
}
+
+ addr = add_reg_for_lit(s, rn, 0);
offset = (insn & 0xff) * 4;
if ((insn & (1 << 23)) == 0) {
offset = -offset;
@@ -10682,27 +10695,15 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
store_reg(s, rd, tmp);
} else {
/* Add/sub 12-bit immediate. */
- if (rn == 15) {
- offset = s->pc & ~(uint32_t)3;
- if (insn & (1 << 23))
- offset -= imm;
- else
- offset += imm;
- tmp = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp, offset);
- store_reg(s, rd, tmp);
+ if (insn & (1 << 23)) {
+ imm = -imm;
+ }
+ tmp = add_reg_for_lit(s, rn, imm);
+ if (rn == 13 && rd == 13) {
+ /* ADD SP, SP, imm or SUB SP, SP, imm */
+ store_sp_checked(s, tmp);
} else {
- tmp = load_reg(s, rn);
- if (insn & (1 << 23))
- tcg_gen_subi_i32(tmp, tmp, imm);
- else
- tcg_gen_addi_i32(tmp, tmp, imm);
- if (rn == 13 && rd == 13) {
- /* ADD SP, SP, imm or SUB SP, SP, imm */
- store_sp_checked(s, tmp);
- } else {
- store_reg(s, rd, tmp);
- }
+ store_reg(s, rd, tmp);
}
}
}
@@ -10816,61 +10817,53 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
}
}
memidx = get_mem_index(s);
- if (rn == 15) {
- addr = tcg_temp_new_i32();
- /* PC relative. */
- /* s->pc has already been incremented by 4. */
- imm = s->pc & 0xfffffffc;
- if (insn & (1 << 23))
- imm += insn & 0xfff;
- else
- imm -= insn & 0xfff;
- tcg_gen_movi_i32(addr, imm);
+ imm = insn & 0xfff;
+ if (insn & (1 << 23)) {
+ /* PC relative or Positive offset. */
+ addr = add_reg_for_lit(s, rn, imm);
+ } else if (rn == 15) {
+ /* PC relative with negative offset. */
+ addr = add_reg_for_lit(s, rn, -imm);
} else {
addr = load_reg(s, rn);
- if (insn & (1 << 23)) {
- /* Positive offset. */
- imm = insn & 0xfff;
- tcg_gen_addi_i32(addr, addr, imm);
- } else {
- imm = insn & 0xff;
- switch ((insn >> 8) & 0xf) {
- case 0x0: /* Shifted Register. */
- shift = (insn >> 4) & 0xf;
- if (shift > 3) {
- tcg_temp_free_i32(addr);
- goto illegal_op;
- }
- tmp = load_reg(s, rm);
- if (shift)
- tcg_gen_shli_i32(tmp, tmp, shift);
- tcg_gen_add_i32(addr, addr, tmp);
- tcg_temp_free_i32(tmp);
- break;
- case 0xc: /* Negative offset. */
- tcg_gen_addi_i32(addr, addr, -imm);
- break;
- case 0xe: /* User privilege. */
- tcg_gen_addi_i32(addr, addr, imm);
- memidx = get_a32_user_mem_index(s);
- break;
- case 0x9: /* Post-decrement. */
- imm = -imm;
- /* Fall through. */
- case 0xb: /* Post-increment. */
- postinc = 1;
- writeback = 1;
- break;
- case 0xd: /* Pre-decrement. */
- imm = -imm;
- /* Fall through. */
- case 0xf: /* Pre-increment. */
- writeback = 1;
- break;
- default:
+ imm = insn & 0xff;
+ switch ((insn >> 8) & 0xf) {
+ case 0x0: /* Shifted Register. */
+ shift = (insn >> 4) & 0xf;
+ if (shift > 3) {
tcg_temp_free_i32(addr);
goto illegal_op;
}
+ tmp = load_reg(s, rm);
+ if (shift) {
+ tcg_gen_shli_i32(tmp, tmp, shift);
+ }
+ tcg_gen_add_i32(addr, addr, tmp);
+ tcg_temp_free_i32(tmp);
+ break;
+ case 0xc: /* Negative offset. */
+ tcg_gen_addi_i32(addr, addr, -imm);
+ break;
+ case 0xe: /* User privilege. */
+ tcg_gen_addi_i32(addr, addr, imm);
+ memidx = get_a32_user_mem_index(s);
+ break;
+ case 0x9: /* Post-decrement. */
+ imm = -imm;
+ /* Fall through. */
+ case 0xb: /* Post-increment. */
+ postinc = 1;
+ writeback = 1;
+ break;
+ case 0xd: /* Pre-decrement. */
+ imm = -imm;
+ /* Fall through. */
+ case 0xf: /* Pre-increment. */
+ writeback = 1;
+ break;
+ default:
+ tcg_temp_free_i32(addr);
+ goto illegal_op;
}
}
@@ -11066,10 +11059,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
if (insn & (1 << 11)) {
rd = (insn >> 8) & 7;
/* load pc-relative. Bit 1 of PC is ignored. */
- val = read_pc(s) + ((insn & 0xff) * 4);
- val &= ~(uint32_t)2;
- addr = tcg_temp_new_i32();
- tcg_gen_movi_i32(addr, val);
+ addr = add_reg_for_lit(s, 15, (insn & 0xff) * 4);
tmp = tcg_temp_new_i32();
gen_aa32_ld32u_iss(s, tmp, addr, get_mem_index(s),
rd | ISSIs16Bit);
@@ -11447,16 +11437,8 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
* - Add PC/SP (immediate)
*/
rd = (insn >> 8) & 7;
- if (insn & (1 << 11)) {
- /* SP */
- tmp = load_reg(s, 13);
- } else {
- /* PC. bit 1 is ignored. */
- tmp = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp, read_pc(s) & ~(uint32_t)2);
- }
val = (insn & 0xff) * 4;
- tcg_gen_addi_i32(tmp, tmp, val);
+ tmp = add_reg_for_lit(s, insn & (1 << 11) ? 13 : 15, val);
store_reg(s, rd, tmp);
break;
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 05/11] target/arm: Remove redundant s->pc & ~1
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
` (3 preceding siblings ...)
2019-08-07 4:53 ` [Qemu-devel] [PATCH 04/11] target/arm: Introduce add_reg_for_lit Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 06/11] target/arm: Replace s->pc with s->base.pc_next Richard Henderson
` (6 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
The thumb bit has already been removed from s->pc, and is always even.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 71d94c053b..100f958e33 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1288,7 +1288,7 @@ static void gen_exception_bkpt_insn(DisasContext *s, int offset, uint32_t syn)
/* Force a TB lookup after an instruction that changes the CPU state. */
static inline void gen_lookup_tb(DisasContext *s)
{
- tcg_gen_movi_i32(cpu_R[15], s->pc & ~1);
+ tcg_gen_movi_i32(cpu_R[15], s->pc);
s->base.is_jmp = DISAS_EXIT;
}
@@ -7819,7 +7819,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
* self-modifying code correctly and also to take
* any pending interrupts immediately.
*/
- gen_goto_tb(s, 0, s->pc & ~1);
+ gen_goto_tb(s, 0, s->pc);
return;
case 7: /* sb */
if ((insn & 0xf) || !dc_isar_feature(aa32_sb, s)) {
@@ -7830,7 +7830,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
* for TCG; MB and end the TB instead.
*/
tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
- gen_goto_tb(s, 0, s->pc & ~1);
+ gen_goto_tb(s, 0, s->pc);
return;
default:
goto illegal_op;
@@ -10464,7 +10464,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
* and also to take any pending interrupts
* immediately.
*/
- gen_goto_tb(s, 0, s->pc & ~1);
+ gen_goto_tb(s, 0, s->pc);
break;
case 7: /* sb */
if ((insn & 0xf) || !dc_isar_feature(aa32_sb, s)) {
@@ -10475,7 +10475,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
* for TCG; MB and end the TB instead.
*/
tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
- gen_goto_tb(s, 0, s->pc & ~1);
+ gen_goto_tb(s, 0, s->pc);
break;
default:
goto illegal_op;
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 06/11] target/arm: Replace s->pc with s->base.pc_next
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
` (4 preceding siblings ...)
2019-08-07 4:53 ` [Qemu-devel] [PATCH 05/11] target/arm: Remove redundant s->pc & ~1 Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 07/11] target/arm: Replace offset with pc in gen_exception_insn Richard Henderson
` (5 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
We must update s->base.pc_next when we return from the translate_insn
hook to the main translator loop. By incrementing s->base.pc_next
immediately after reading the insn word, "pc_next" contains the address
of the next instruction throughout translation.
All remaining uses of s->pc are referencing the address of the next insn,
so this is now a simple global replacement. Remove the "s->pc" field.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate.h | 1 -
target/arm/translate-a64.c | 51 +++++++++---------
target/arm/translate.c | 103 ++++++++++++++++++-------------------
3 files changed, 72 insertions(+), 83 deletions(-)
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 525603eb30..de600073d8 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -9,7 +9,6 @@ typedef struct DisasContext {
DisasContextBase base;
const ARMISARegisters *isar;
- target_ulong pc;
/* The address of the current instruction being translated. */
target_ulong pc_curr;
target_ulong page_start;
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index da447eedcc..e640e116b0 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -268,7 +268,7 @@ static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
{
- gen_a64_set_pc_im(s->pc - offset);
+ gen_a64_set_pc_im(s->base.pc_next - offset);
gen_exception_internal(excp);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -276,7 +276,7 @@ static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
static void gen_exception_insn(DisasContext *s, int offset, int excp,
uint32_t syndrome, uint32_t target_el)
{
- gen_a64_set_pc_im(s->pc - offset);
+ gen_a64_set_pc_im(s->base.pc_next - offset);
gen_exception(excp, syndrome, target_el);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -286,7 +286,7 @@ static void gen_exception_bkpt_insn(DisasContext *s, int offset,
{
TCGv_i32 tcg_syn;
- gen_a64_set_pc_im(s->pc - offset);
+ gen_a64_set_pc_im(s->base.pc_next - offset);
tcg_syn = tcg_const_i32(syndrome);
gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
tcg_temp_free_i32(tcg_syn);
@@ -1252,7 +1252,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->pc);
+ tcg_gen_movi_i64(cpu_reg(s, 30), s->base.pc_next);
}
/* B Branch / BL Branch with link */
@@ -1285,7 +1285,7 @@ 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->pc);
+ gen_goto_tb(s, 0, s->base.pc_next);
gen_set_label(label_match);
gen_goto_tb(s, 1, addr);
}
@@ -1316,7 +1316,7 @@ 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->pc);
+ gen_goto_tb(s, 0, s->base.pc_next);
gen_set_label(label_match);
gen_goto_tb(s, 1, addr);
}
@@ -1344,7 +1344,7 @@ 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->pc);
+ gen_goto_tb(s, 0, s->base.pc_next);
gen_set_label(label_match);
gen_goto_tb(s, 1, addr);
} else {
@@ -1505,7 +1505,7 @@ static void handle_sync(DisasContext *s, uint32_t insn,
* any pending interrupts immediately.
*/
reset_btype(s);
- gen_goto_tb(s, 0, s->pc);
+ gen_goto_tb(s, 0, s->base.pc_next);
return;
case 7: /* SB */
@@ -1517,7 +1517,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->pc);
+ gen_goto_tb(s, 0, s->base.pc_next);
return;
default:
@@ -2029,7 +2029,7 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
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->pc);
+ tcg_gen_movi_i64(cpu_reg(s, 30), s->base.pc_next);
}
break;
@@ -2056,7 +2056,7 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
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->pc);
+ tcg_gen_movi_i64(cpu_reg(s, 30), s->base.pc_next);
}
break;
@@ -14044,10 +14044,10 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
{
uint32_t insn;
- s->pc_curr = s->pc;
- insn = arm_ldl_code(env, s->pc, s->sctlr_b);
+ s->pc_curr = s->base.pc_next;
+ insn = arm_ldl_code(env, s->base.pc_next, s->sctlr_b);
s->insn = insn;
- s->pc += 4;
+ s->base.pc_next += 4;
s->fp_access_checked = false;
@@ -14144,7 +14144,6 @@ static void aarch64_tr_init_disas_context(DisasContextBase *dcbase,
int bound, core_mmu_idx;
dc->isar = &arm_cpu->isar;
- dc->pc = dc->base.pc_first;
dc->condjmp = 0;
dc->aarch64 = 1;
@@ -14217,7 +14216,7 @@ static void aarch64_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
{
DisasContext *dc = container_of(dcbase, DisasContext, base);
- tcg_gen_insn_start(dc->pc, 0, 0);
+ tcg_gen_insn_start(dc->base.pc_next, 0, 0);
dc->insn_start = tcg_last_op();
}
@@ -14227,7 +14226,7 @@ static bool aarch64_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
DisasContext *dc = container_of(dcbase, DisasContext, base);
if (bp->flags & BP_CPU) {
- gen_a64_set_pc_im(dc->pc);
+ gen_a64_set_pc_im(dc->base.pc_next);
gen_helper_check_breakpoints(cpu_env);
/* End the TB early; it likely won't be executed */
dc->base.is_jmp = DISAS_TOO_MANY;
@@ -14238,7 +14237,7 @@ static bool aarch64_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
to for it to be properly cleared -- thus we
increment the PC here so that the logic setting
tb->size below does the right thing. */
- dc->pc += 4;
+ dc->base.pc_next += 4;
dc->base.is_jmp = DISAS_NORETURN;
}
@@ -14269,7 +14268,6 @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
disas_a64_insn(env, dc);
}
- dc->base.pc_next = dc->pc;
translator_loop_temp_check(&dc->base);
}
@@ -14285,7 +14283,7 @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
*/
switch (dc->base.is_jmp) {
default:
- gen_a64_set_pc_im(dc->pc);
+ gen_a64_set_pc_im(dc->base.pc_next);
/* fall through */
case DISAS_EXIT:
case DISAS_JUMP:
@@ -14302,11 +14300,11 @@ 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->pc);
+ gen_goto_tb(dc, 1, dc->base.pc_next);
break;
default:
case DISAS_UPDATE:
- gen_a64_set_pc_im(dc->pc);
+ gen_a64_set_pc_im(dc->base.pc_next);
/* fall through */
case DISAS_EXIT:
tcg_gen_exit_tb(NULL, 0);
@@ -14318,11 +14316,11 @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
case DISAS_SWI:
break;
case DISAS_WFE:
- gen_a64_set_pc_im(dc->pc);
+ gen_a64_set_pc_im(dc->base.pc_next);
gen_helper_wfe(cpu_env);
break;
case DISAS_YIELD:
- gen_a64_set_pc_im(dc->pc);
+ gen_a64_set_pc_im(dc->base.pc_next);
gen_helper_yield(cpu_env);
break;
case DISAS_WFI:
@@ -14332,7 +14330,7 @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
*/
TCGv_i32 tmp = tcg_const_i32(4);
- gen_a64_set_pc_im(dc->pc);
+ gen_a64_set_pc_im(dc->base.pc_next);
gen_helper_wfi(cpu_env, tmp);
tcg_temp_free_i32(tmp);
/* The helper doesn't necessarily throw an exception, but we
@@ -14343,9 +14341,6 @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
}
}
}
-
- /* Functions above can change dc->pc, so re-align db->pc_next */
- dc->base.pc_next = dc->pc;
}
static void aarch64_tr_disas_log(const DisasContextBase *dcbase,
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 100f958e33..dfbaa592ab 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1051,7 +1051,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->pc);
+ gen_set_pc_im(s, s->base.pc_next);
gen_helper_v7m_blxns(cpu_env, var);
tcg_temp_free_i32(var);
s->base.is_jmp = DISAS_EXIT;
@@ -1237,7 +1237,7 @@ static inline void gen_hvc(DisasContext *s, int imm16)
* for single stepping.)
*/
s->svc_imm = imm16;
- gen_set_pc_im(s, s->pc);
+ gen_set_pc_im(s, s->base.pc_next);
s->base.is_jmp = DISAS_HVC;
}
@@ -1252,14 +1252,14 @@ static inline void gen_smc(DisasContext *s)
tmp = tcg_const_i32(syn_aa32_smc());
gen_helper_pre_smc(cpu_env, tmp);
tcg_temp_free_i32(tmp);
- gen_set_pc_im(s, s->pc);
+ gen_set_pc_im(s, s->base.pc_next);
s->base.is_jmp = DISAS_SMC;
}
static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
{
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc - offset);
+ gen_set_pc_im(s, s->base.pc_next - offset);
gen_exception_internal(excp);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -1268,7 +1268,7 @@ static void gen_exception_insn(DisasContext *s, int offset, int excp,
int syn, uint32_t target_el)
{
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc - offset);
+ gen_set_pc_im(s, s->base.pc_next - offset);
gen_exception(excp, syn, target_el);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -1278,7 +1278,7 @@ static void gen_exception_bkpt_insn(DisasContext *s, int offset, uint32_t syn)
TCGv_i32 tcg_syn;
gen_set_condexec(s);
- gen_set_pc_im(s, s->pc - offset);
+ gen_set_pc_im(s, s->base.pc_next - offset);
tcg_syn = tcg_const_i32(syn);
gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
tcg_temp_free_i32(tcg_syn);
@@ -1288,7 +1288,7 @@ static void gen_exception_bkpt_insn(DisasContext *s, int offset, uint32_t syn)
/* Force a TB lookup after an instruction that changes the CPU state. */
static inline void gen_lookup_tb(DisasContext *s)
{
- tcg_gen_movi_i32(cpu_R[15], s->pc);
+ tcg_gen_movi_i32(cpu_R[15], s->base.pc_next);
s->base.is_jmp = DISAS_EXIT;
}
@@ -2924,7 +2924,7 @@ static inline bool use_goto_tb(DisasContext *s, target_ulong dest)
{
#ifndef CONFIG_USER_ONLY
return (s->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) ||
- ((s->pc - 1) & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
+ ((s->base.pc_next - 1) & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
#else
return true;
#endif
@@ -3294,17 +3294,17 @@ static void gen_nop_hint(DisasContext *s, int val)
*/
case 1: /* yield */
if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
- gen_set_pc_im(s, s->pc);
+ gen_set_pc_im(s, s->base.pc_next);
s->base.is_jmp = DISAS_YIELD;
}
break;
case 3: /* wfi */
- gen_set_pc_im(s, s->pc);
+ gen_set_pc_im(s, s->base.pc_next);
s->base.is_jmp = DISAS_WFI;
break;
case 2: /* wfe */
if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
- gen_set_pc_im(s, s->pc);
+ gen_set_pc_im(s, s->base.pc_next);
s->base.is_jmp = DISAS_WFE;
}
break;
@@ -7255,7 +7255,7 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
if (isread) {
return 1;
}
- gen_set_pc_im(s, s->pc);
+ gen_set_pc_im(s, s->base.pc_next);
s->base.is_jmp = DISAS_WFI;
return 0;
default:
@@ -7819,7 +7819,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
* self-modifying code correctly and also to take
* any pending interrupts immediately.
*/
- gen_goto_tb(s, 0, s->pc);
+ gen_goto_tb(s, 0, s->base.pc_next);
return;
case 7: /* sb */
if ((insn & 0xf) || !dc_isar_feature(aa32_sb, s)) {
@@ -7830,7 +7830,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
* for TCG; MB and end the TB instead.
*/
tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
- gen_goto_tb(s, 0, s->pc);
+ gen_goto_tb(s, 0, s->base.pc_next);
return;
default:
goto illegal_op;
@@ -7886,7 +7886,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
int32_t offset;
tmp = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp, s->pc);
+ tcg_gen_movi_i32(tmp, s->base.pc_next);
store_reg(s, 14, tmp);
/* Sign-extend the 24-bit offset */
offset = (((int32_t)insn) << 8) >> 8;
@@ -8071,7 +8071,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
/* branch link/exchange thumb (blx) */
tmp = load_reg(s, rm);
tmp2 = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp2, s->pc);
+ tcg_gen_movi_i32(tmp2, s->base.pc_next);
store_reg(s, 14, tmp2);
gen_bx(s, tmp);
break;
@@ -9237,7 +9237,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
/* branch (and link) */
if (insn & (1 << 24)) {
tmp = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp, s->pc);
+ tcg_gen_movi_i32(tmp, s->base.pc_next);
store_reg(s, 14, tmp);
}
offset = sextract32(insn << 2, 0, 26);
@@ -9259,7 +9259,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
break;
case 0xf:
/* swi */
- gen_set_pc_im(s, s->pc);
+ gen_set_pc_im(s, s->base.pc_next);
s->svc_imm = extract32(insn, 0, 24);
s->base.is_jmp = DISAS_SWI;
break;
@@ -10341,7 +10341,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
if (insn & (1 << 14)) {
/* Branch and link. */
- tcg_gen_movi_i32(cpu_R[14], s->pc | 1);
+ tcg_gen_movi_i32(cpu_R[14], s->base.pc_next | 1);
}
offset += read_pc(s);
@@ -10464,7 +10464,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
* and also to take any pending interrupts
* immediately.
*/
- gen_goto_tb(s, 0, s->pc);
+ gen_goto_tb(s, 0, s->base.pc_next);
break;
case 7: /* sb */
if ((insn & 0xf) || !dc_isar_feature(aa32_sb, s)) {
@@ -10475,7 +10475,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
* for TCG; MB and end the TB instead.
*/
tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
- gen_goto_tb(s, 0, s->pc);
+ gen_goto_tb(s, 0, s->base.pc_next);
break;
default:
goto illegal_op;
@@ -11136,7 +11136,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
/* BLX/BX */
tmp = load_reg(s, rm);
if (link) {
- val = (uint32_t)s->pc | 1;
+ val = (uint32_t)s->base.pc_next | 1;
tmp2 = tcg_temp_new_i32();
tcg_gen_movi_i32(tmp2, val);
store_reg(s, 14, tmp2);
@@ -11710,7 +11710,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
if (cond == 0xf) {
/* swi */
- gen_set_pc_im(s, s->pc);
+ gen_set_pc_im(s, s->base.pc_next);
s->svc_imm = extract32(insn, 0, 8);
s->base.is_jmp = DISAS_SWI;
break;
@@ -11739,7 +11739,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
tcg_gen_andi_i32(tmp, tmp, 0xfffffffc);
tmp2 = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp2, s->pc | 1);
+ tcg_gen_movi_i32(tmp2, s->base.pc_next | 1);
store_reg(s, 14, tmp2);
gen_bx(s, tmp);
break;
@@ -11764,7 +11764,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
tcg_gen_addi_i32(tmp, tmp, offset);
tmp2 = tcg_temp_new_i32();
- tcg_gen_movi_i32(tmp2, s->pc | 1);
+ tcg_gen_movi_i32(tmp2, s->base.pc_next | 1);
store_reg(s, 14, tmp2);
gen_bx(s, tmp);
} else {
@@ -11784,16 +11784,16 @@ undef:
static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
{
- /* Return true if the insn at dc->pc might cross a page boundary.
+ /* Return true if the insn at dc->base.pc_next might cross a page boundary.
* (False positives are OK, false negatives are not.)
* We know this is a Thumb insn, and our caller ensures we are
- * only called if dc->pc is less than 4 bytes from the page
+ * only called if dc->base.pc_next is less than 4 bytes from the page
* boundary, so we cross the page if the first 16 bits indicate
* that this is a 32 bit insn.
*/
- uint16_t insn = arm_lduw_code(env, s->pc, s->sctlr_b);
+ uint16_t insn = arm_lduw_code(env, s->base.pc_next, s->sctlr_b);
- return !thumb_insn_is_16bit(s, s->pc, insn);
+ return !thumb_insn_is_16bit(s, s->base.pc_next, insn);
}
static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
@@ -11805,7 +11805,6 @@ static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
uint32_t condexec, core_mmu_idx;
dc->isar = &cpu->isar;
- dc->pc = dc->base.pc_first;
dc->condjmp = 0;
dc->aarch64 = 0;
@@ -11935,7 +11934,7 @@ static void arm_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
{
DisasContext *dc = container_of(dcbase, DisasContext, base);
- tcg_gen_insn_start(dc->pc,
+ tcg_gen_insn_start(dc->base.pc_next,
(dc->condexec_cond << 4) | (dc->condexec_mask >> 1),
0);
dc->insn_start = tcg_last_op();
@@ -11948,7 +11947,7 @@ static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
if (bp->flags & BP_CPU) {
gen_set_condexec(dc);
- gen_set_pc_im(dc, dc->pc);
+ gen_set_pc_im(dc, dc->base.pc_next);
gen_helper_check_breakpoints(cpu_env);
/* End the TB early; it's likely not going to be executed */
dc->base.is_jmp = DISAS_TOO_MANY;
@@ -11961,7 +11960,7 @@ static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
tb->size below does the right thing. */
/* TODO: Advance PC by correct instruction length to
* avoid disassembler error messages */
- dc->pc += 2;
+ dc->base.pc_next += 2;
dc->base.is_jmp = DISAS_NORETURN;
}
@@ -11972,7 +11971,7 @@ static bool arm_pre_translate_insn(DisasContext *dc)
{
#ifdef CONFIG_USER_ONLY
/* Intercept jump to the magic kernel page. */
- if (dc->pc >= 0xffff0000) {
+ if (dc->base.pc_next >= 0xffff0000) {
/* We always get here via a jump, so know we are not in a
conditional execution block. */
gen_exception_internal(EXCP_KERNEL_TRAP);
@@ -12008,7 +12007,6 @@ static void arm_post_translate_insn(DisasContext *dc)
gen_set_label(dc->condlabel);
dc->condjmp = 0;
}
- dc->base.pc_next = dc->pc;
translator_loop_temp_check(&dc->base);
}
@@ -12022,10 +12020,10 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
return;
}
- dc->pc_curr = dc->pc;
- insn = arm_ldl_code(env, dc->pc, dc->sctlr_b);
+ dc->pc_curr = dc->base.pc_next;
+ insn = arm_ldl_code(env, dc->base.pc_next, dc->sctlr_b);
dc->insn = insn;
- dc->pc += 4;
+ dc->base.pc_next += 4;
disas_arm_insn(dc, insn);
arm_post_translate_insn(dc);
@@ -12091,15 +12089,15 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
return;
}
- dc->pc_curr = dc->pc;
- insn = arm_lduw_code(env, dc->pc, dc->sctlr_b);
- is_16bit = thumb_insn_is_16bit(dc, dc->pc, insn);
- dc->pc += 2;
+ dc->pc_curr = dc->base.pc_next;
+ insn = arm_lduw_code(env, dc->base.pc_next, dc->sctlr_b);
+ is_16bit = thumb_insn_is_16bit(dc, dc->base.pc_next, insn);
+ dc->base.pc_next += 2;
if (!is_16bit) {
- uint32_t insn2 = arm_lduw_code(env, dc->pc, dc->sctlr_b);
+ uint32_t insn2 = arm_lduw_code(env, dc->base.pc_next, dc->sctlr_b);
insn = insn << 16 | insn2;
- dc->pc += 2;
+ dc->base.pc_next += 2;
}
dc->insn = insn;
@@ -12147,8 +12145,8 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
* but isn't very efficient).
*/
if (dc->base.is_jmp == DISAS_NEXT
- && (dc->pc - dc->page_start >= TARGET_PAGE_SIZE
- || (dc->pc - dc->page_start >= TARGET_PAGE_SIZE - 3
+ && (dc->base.pc_next - dc->page_start >= TARGET_PAGE_SIZE
+ || (dc->base.pc_next - dc->page_start >= TARGET_PAGE_SIZE - 3
&& insn_crosses_page(env, dc)))) {
dc->base.is_jmp = DISAS_TOO_MANY;
}
@@ -12193,7 +12191,7 @@ static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
case DISAS_NEXT:
case DISAS_TOO_MANY:
case DISAS_UPDATE:
- gen_set_pc_im(dc, dc->pc);
+ gen_set_pc_im(dc, dc->base.pc_next);
/* fall through */
default:
/* FIXME: Single stepping a WFI insn will not halt the CPU. */
@@ -12214,13 +12212,13 @@ 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->pc);
+ gen_goto_tb(dc, 1, dc->base.pc_next);
break;
case DISAS_JUMP:
gen_goto_ptr();
break;
case DISAS_UPDATE:
- gen_set_pc_im(dc, dc->pc);
+ gen_set_pc_im(dc, dc->base.pc_next);
/* fall through */
default:
/* indicate that the hash table must be used to find the next TB */
@@ -12266,15 +12264,12 @@ static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
gen_set_label(dc->condlabel);
gen_set_condexec(dc);
if (unlikely(is_singlestepping(dc))) {
- gen_set_pc_im(dc, dc->pc);
+ gen_set_pc_im(dc, dc->base.pc_next);
gen_singlestep_exception(dc);
} else {
- gen_goto_tb(dc, 1, dc->pc);
+ gen_goto_tb(dc, 1, dc->base.pc_next);
}
}
-
- /* Functions above can change dc->pc, so re-align db->pc_next */
- dc->base.pc_next = dc->pc;
}
static void arm_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu)
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 07/11] target/arm: Replace offset with pc in gen_exception_insn
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
` (5 preceding siblings ...)
2019-08-07 4:53 ` [Qemu-devel] [PATCH 06/11] target/arm: Replace s->pc with s->base.pc_next Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 08/11] target/arm: Replace offset with pc in gen_exception_internal_insn Richard Henderson
` (4 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
The offset is variable depending on the instruction set, whereas
we have stored values for the current pc and the next pc. Passing
in the actual value is clearer in intent.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a64.c | 25 ++++++++++++++-----------
target/arm/translate-vfp.inc.c | 6 +++---
target/arm/translate.c | 31 ++++++++++++++++---------------
3 files changed, 33 insertions(+), 29 deletions(-)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index e640e116b0..92aa6db12e 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -273,10 +273,10 @@ static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
s->base.is_jmp = DISAS_NORETURN;
}
-static void gen_exception_insn(DisasContext *s, int offset, int excp,
+static void gen_exception_insn(DisasContext *s, uint64_t pc, int excp,
uint32_t syndrome, uint32_t target_el)
{
- gen_a64_set_pc_im(s->base.pc_next - offset);
+ gen_a64_set_pc_im(pc);
gen_exception(excp, syndrome, target_el);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -356,7 +356,7 @@ static inline void gen_goto_tb(DisasContext *s, int n, uint64_t dest)
void unallocated_encoding(DisasContext *s)
{
/* Unallocated and reserved encodings are uncategorized */
- gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
}
@@ -1128,8 +1128,8 @@ static inline bool fp_access_check(DisasContext *s)
return true;
}
- gen_exception_insn(s, 4, EXCP_UDEF, syn_fp_access_trap(1, 0xe, false),
- s->fp_excp_el);
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ syn_fp_access_trap(1, 0xe, false), s->fp_excp_el);
return false;
}
@@ -1139,7 +1139,7 @@ static inline bool fp_access_check(DisasContext *s)
bool sve_access_check(DisasContext *s)
{
if (s->sve_excp_el) {
- gen_exception_insn(s, 4, EXCP_UDEF, syn_sve_access_trap(),
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_sve_access_trap(),
s->sve_excp_el);
return false;
}
@@ -1873,8 +1873,8 @@ static void disas_exc(DisasContext *s, uint32_t insn)
switch (op2_ll) {
case 1: /* SVC */
gen_ss_advance(s);
- gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
- default_exception_el(s));
+ gen_exception_insn(s, s->base.pc_next, EXCP_SWI,
+ syn_aa64_svc(imm16), default_exception_el(s));
break;
case 2: /* HVC */
if (s->current_el == 0) {
@@ -1887,7 +1887,8 @@ static void disas_exc(DisasContext *s, uint32_t insn)
gen_a64_set_pc_im(s->pc_curr);
gen_helper_pre_hvc(cpu_env);
gen_ss_advance(s);
- gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
+ gen_exception_insn(s, s->base.pc_next, EXCP_HVC,
+ syn_aa64_hvc(imm16), 2);
break;
case 3: /* SMC */
if (s->current_el == 0) {
@@ -1899,7 +1900,8 @@ static void disas_exc(DisasContext *s, uint32_t insn)
gen_helper_pre_smc(cpu_env, tmp);
tcg_temp_free_i32(tmp);
gen_ss_advance(s);
- gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16), 3);
+ gen_exception_insn(s, s->base.pc_next, EXCP_SMC,
+ syn_aa64_smc(imm16), 3);
break;
default:
unallocated_encoding(s);
@@ -14078,7 +14080,8 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
if (s->btype != 0
&& s->guarded_page
&& !btype_destination_ok(insn, s->bt, s->btype)) {
- gen_exception_insn(s, 4, EXCP_UDEF, syn_btitrap(s->btype),
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ syn_btitrap(s->btype),
default_exception_el(s));
return;
}
diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
index 262d4177e5..5065d4524c 100644
--- a/target/arm/translate-vfp.inc.c
+++ b/target/arm/translate-vfp.inc.c
@@ -96,10 +96,10 @@ static bool full_vfp_access_check(DisasContext *s, bool ignore_vfp_enabled)
{
if (s->fp_excp_el) {
if (arm_dc_feature(s, ARM_FEATURE_M)) {
- gen_exception_insn(s, 4, EXCP_NOCP, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_NOCP, syn_uncategorized(),
s->fp_excp_el);
} else {
- gen_exception_insn(s, 4, EXCP_UDEF,
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
syn_fp_access_trap(1, 0xe, false),
s->fp_excp_el);
}
@@ -108,7 +108,7 @@ static bool full_vfp_access_check(DisasContext *s, bool ignore_vfp_enabled)
if (!s->vfp_enabled && !ignore_vfp_enabled) {
assert(!arm_dc_feature(s, ARM_FEATURE_M));
- gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
return false;
}
diff --git a/target/arm/translate.c b/target/arm/translate.c
index dfbaa592ab..7a05ecae87 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1264,11 +1264,11 @@ static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
s->base.is_jmp = DISAS_NORETURN;
}
-static void gen_exception_insn(DisasContext *s, int offset, int excp,
+static void gen_exception_insn(DisasContext *s, uint32_t pc, int excp,
int syn, uint32_t target_el)
{
gen_set_condexec(s);
- gen_set_pc_im(s, s->base.pc_next - offset);
+ gen_set_pc_im(s, pc);
gen_exception(excp, syn, target_el);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -1315,7 +1315,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
return;
}
- gen_exception_insn(s, s->thumb ? 2 : 4, EXCP_UDEF, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
}
@@ -3192,7 +3192,8 @@ 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, 4, EXCP_UDEF, syn_uncategorized(), exc_target);
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ syn_uncategorized(), exc_target);
return false;
}
@@ -3586,7 +3587,7 @@ static int disas_neon_ls_insn(DisasContext *s, uint32_t insn)
* for attempts to execute invalid vfp/neon encodings with FP disabled.
*/
if (s->fp_excp_el) {
- gen_exception_insn(s, 4, EXCP_UDEF,
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
syn_simd_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -4857,7 +4858,7 @@ static int disas_neon_data_insn(DisasContext *s, uint32_t insn)
* for attempts to execute invalid vfp/neon encodings with FP disabled.
*/
if (s->fp_excp_el) {
- gen_exception_insn(s, 4, EXCP_UDEF,
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
syn_simd_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -6985,7 +6986,7 @@ static int disas_neon_insn_3same_ext(DisasContext *s, uint32_t insn)
}
if (s->fp_excp_el) {
- gen_exception_insn(s, 4, EXCP_UDEF,
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
syn_simd_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -7108,7 +7109,7 @@ static int disas_neon_insn_2reg_scalar_ext(DisasContext *s, uint32_t insn)
off_rm = vfp_reg_offset(0, rm);
}
if (s->fp_excp_el) {
- gen_exception_insn(s, 4, EXCP_UDEF,
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
syn_simd_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -7601,7 +7602,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(s, 4, EXCP_UDEF, syn_uncategorized(), 3);
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(), 3);
return;
}
@@ -7637,7 +7638,7 @@ static void gen_srs(DisasContext *s,
}
if (undef) {
- gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
return;
}
@@ -7728,7 +7729,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, 4, EXCP_INVSTATE, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_INVSTATE, syn_uncategorized(),
default_exception_el(s));
return;
}
@@ -9265,7 +9266,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
break;
default:
illegal_op:
- gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
break;
}
@@ -10290,7 +10291,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
}
/* All other insns: NOCP */
- gen_exception_insn(s, 4, EXCP_NOCP, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_NOCP, syn_uncategorized(),
default_exception_el(s));
break;
}
@@ -10954,7 +10955,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
}
return;
illegal_op:
- gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
}
@@ -11778,7 +11779,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
return;
illegal_op:
undef:
- gen_exception_insn(s, 2, EXCP_UDEF, syn_uncategorized(),
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
}
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 08/11] target/arm: Replace offset with pc in gen_exception_internal_insn
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
` (6 preceding siblings ...)
2019-08-07 4:53 ` [Qemu-devel] [PATCH 07/11] target/arm: Replace offset with pc in gen_exception_insn Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 09/11] target/arm: Remove offset argument to gen_exception_bkpt_insn Richard Henderson
` (3 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
The offset is variable depending on the instruction set.
Passing in the actual value is clearer in intent.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a64.c | 8 ++++----
target/arm/translate.c | 8 ++++----
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 92aa6db12e..c8504d221a 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -266,9 +266,9 @@ static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
tcg_temp_free_i32(tcg_excp);
}
-static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
+static void gen_exception_internal_insn(DisasContext *s, uint64_t pc, int excp)
{
- gen_a64_set_pc_im(s->base.pc_next - offset);
+ gen_a64_set_pc_im(pc);
gen_exception_internal(excp);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -1938,7 +1938,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
break;
}
#endif
- gen_exception_internal_insn(s, 0, EXCP_SEMIHOST);
+ gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
} else {
unsupported_encoding(s, insn);
}
@@ -14234,7 +14234,7 @@ static bool aarch64_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
/* End the TB early; it likely won't be executed */
dc->base.is_jmp = DISAS_TOO_MANY;
} else {
- gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
+ gen_exception_internal_insn(dc, dc->base.pc_next, EXCP_DEBUG);
/* The address covered by the breakpoint must be
included in [tb->pc, tb->pc + tb->size) in order
to for it to be properly cleared -- thus we
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 7a05ecae87..e6b18ecdaf 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1256,10 +1256,10 @@ static inline void gen_smc(DisasContext *s)
s->base.is_jmp = DISAS_SMC;
}
-static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
+static void gen_exception_internal_insn(DisasContext *s, uint32_t pc, int excp)
{
gen_set_condexec(s);
- gen_set_pc_im(s, s->base.pc_next - offset);
+ gen_set_pc_im(s, pc);
gen_exception_internal(excp);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -1311,7 +1311,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
s->current_el != 0 &&
#endif
(imm == (s->thumb ? 0x3c : 0xf000))) {
- gen_exception_internal_insn(s, 0, EXCP_SEMIHOST);
+ gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
return;
}
@@ -11953,7 +11953,7 @@ static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
/* End the TB early; it's likely not going to be executed */
dc->base.is_jmp = DISAS_TOO_MANY;
} else {
- gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
+ gen_exception_internal_insn(dc, dc->base.pc_next, EXCP_DEBUG);
/* The address covered by the breakpoint must be
included in [tb->pc, tb->pc + tb->size) in order
to for it to be properly cleared -- thus we
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 09/11] target/arm: Remove offset argument to gen_exception_bkpt_insn
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
` (7 preceding siblings ...)
2019-08-07 4:53 ` [Qemu-devel] [PATCH 08/11] target/arm: Replace offset with pc in gen_exception_internal_insn Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32 Richard Henderson
` (2 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
Unlike the other more generic gen_exception{,_internal}_insn
interfaces, breakpoints always refer to the current instruction.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a64.c | 7 +++----
target/arm/translate.c | 8 ++++----
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index c8504d221a..d68bfc66d3 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -281,12 +281,11 @@ static void gen_exception_insn(DisasContext *s, uint64_t pc, int excp,
s->base.is_jmp = DISAS_NORETURN;
}
-static void gen_exception_bkpt_insn(DisasContext *s, int offset,
- uint32_t syndrome)
+static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syndrome)
{
TCGv_i32 tcg_syn;
- gen_a64_set_pc_im(s->base.pc_next - offset);
+ gen_a64_set_pc_im(s->pc_curr);
tcg_syn = tcg_const_i32(syndrome);
gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
tcg_temp_free_i32(tcg_syn);
@@ -1914,7 +1913,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
break;
}
/* BRK */
- gen_exception_bkpt_insn(s, 4, syn_aa64_bkpt(imm16));
+ gen_exception_bkpt_insn(s, syn_aa64_bkpt(imm16));
break;
case 2:
if (op2_ll != 0) {
diff --git a/target/arm/translate.c b/target/arm/translate.c
index e6b18ecdaf..d6b0ab7247 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1273,12 +1273,12 @@ static void gen_exception_insn(DisasContext *s, uint32_t pc, int excp,
s->base.is_jmp = DISAS_NORETURN;
}
-static void gen_exception_bkpt_insn(DisasContext *s, int offset, uint32_t syn)
+static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syn)
{
TCGv_i32 tcg_syn;
gen_set_condexec(s);
- gen_set_pc_im(s, s->base.pc_next - offset);
+ gen_set_pc_im(s, s->pc_curr);
tcg_syn = tcg_const_i32(syn);
gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
tcg_temp_free_i32(tcg_syn);
@@ -8155,7 +8155,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
case 1:
/* bkpt */
ARCH(5);
- gen_exception_bkpt_insn(s, 4, syn_aa32_bkpt(imm16, false));
+ gen_exception_bkpt_insn(s, syn_aa32_bkpt(imm16, false));
break;
case 2:
/* Hypervisor call (v7) */
@@ -11581,7 +11581,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
{
int imm8 = extract32(insn, 0, 8);
ARCH(5);
- gen_exception_bkpt_insn(s, 2, syn_aa32_bkpt(imm8, true));
+ gen_exception_bkpt_insn(s, syn_aa32_bkpt(imm8, true));
break;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
` (8 preceding siblings ...)
2019-08-07 4:53 ` [Qemu-devel] [PATCH 09/11] target/arm: Remove offset argument to gen_exception_bkpt_insn Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-07 17:35 ` Philippe Mathieu-Daudé
2019-08-26 8:45 ` [Qemu-devel] [Qemu-arm] " Laurent Desnogues
2019-08-07 4:53 ` [Qemu-devel] [PATCH 11/11] target/arm: Remove helper_double_saturate Richard Henderson
2019-08-07 17:52 ` [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Peter Maydell
11 siblings, 2 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
Promote this function from aarch64 to fully general use.
Use it to unify the code sequences for generating illegal
opcode exceptions.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a64.h | 2 --
target/arm/translate.h | 2 ++
target/arm/translate-a64.c | 7 -------
target/arm/translate-vfp.inc.c | 3 +--
target/arm/translate.c | 22 ++++++++++++----------
5 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
index 9cd2b3d238..12ad8ac6ed 100644
--- a/target/arm/translate-a64.h
+++ b/target/arm/translate-a64.h
@@ -18,8 +18,6 @@
#ifndef TARGET_ARM_TRANSLATE_A64_H
#define TARGET_ARM_TRANSLATE_A64_H
-void unallocated_encoding(DisasContext *s);
-
#define unsupported_encoding(s, insn) \
do { \
qemu_log_mask(LOG_UNIMP, \
diff --git a/target/arm/translate.h b/target/arm/translate.h
index de600073d8..6a65df0b27 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -98,6 +98,8 @@ typedef struct DisasCompare {
bool value_global;
} DisasCompare;
+void unallocated_encoding(DisasContext *s);
+
/* Share the TCG temporaries common between 32 and 64 bit modes. */
extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF;
extern TCGv_i64 cpu_exclusive_addr;
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index d68bfc66d3..9e1ffe9cfb 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -352,13 +352,6 @@ static inline void gen_goto_tb(DisasContext *s, int n, uint64_t dest)
}
}
-void unallocated_encoding(DisasContext *s)
-{
- /* Unallocated and reserved encodings are uncategorized */
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
- default_exception_el(s));
-}
-
static void init_tmp_a64_array(DisasContext *s)
{
#ifdef CONFIG_DEBUG_TCG
diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
index 5065d4524c..3e8ea80493 100644
--- a/target/arm/translate-vfp.inc.c
+++ b/target/arm/translate-vfp.inc.c
@@ -108,8 +108,7 @@ static bool full_vfp_access_check(DisasContext *s, bool ignore_vfp_enabled)
if (!s->vfp_enabled && !ignore_vfp_enabled) {
assert(!arm_dc_feature(s, ARM_FEATURE_M));
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
- default_exception_el(s));
+ unallocated_encoding(s);
return false;
}
diff --git a/target/arm/translate.c b/target/arm/translate.c
index d6b0ab7247..2d447d4b90 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1285,6 +1285,13 @@ static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syn)
s->base.is_jmp = DISAS_NORETURN;
}
+void unallocated_encoding(DisasContext *s)
+{
+ /* Unallocated and reserved encodings are uncategorized */
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
+ default_exception_el(s));
+}
+
/* Force a TB lookup after an instruction that changes the CPU state. */
static inline void gen_lookup_tb(DisasContext *s)
{
@@ -1315,8 +1322,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
return;
}
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
- default_exception_el(s));
+ unallocated_encoding(s);
}
static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
@@ -7638,8 +7644,7 @@ static void gen_srs(DisasContext *s,
}
if (undef) {
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
- default_exception_el(s));
+ unallocated_encoding(s);
return;
}
@@ -9266,8 +9271,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
break;
default:
illegal_op:
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
- default_exception_el(s));
+ unallocated_encoding(s);
break;
}
}
@@ -10955,8 +10959,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
}
return;
illegal_op:
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
- default_exception_el(s));
+ unallocated_encoding(s);
}
static void disas_thumb_insn(DisasContext *s, uint32_t insn)
@@ -11779,8 +11782,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
return;
illegal_op:
undef:
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
- default_exception_el(s));
+ unallocated_encoding(s);
}
static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 11/11] target/arm: Remove helper_double_saturate
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
` (9 preceding siblings ...)
2019-08-07 4:53 ` [Qemu-devel] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32 Richard Henderson
@ 2019-08-07 4:53 ` Richard Henderson
2019-08-07 17:52 ` [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Peter Maydell
11 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 4:53 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm
Replace x = double_saturate(y) with x = add_saturate(y, y).
There is no need for a separate more specialized helper.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/helper.h | 1 -
target/arm/op_helper.c | 15 ---------------
target/arm/translate.c | 4 ++--
3 files changed, 2 insertions(+), 18 deletions(-)
diff --git a/target/arm/helper.h b/target/arm/helper.h
index 132aa1682e..1fb2cb5a77 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -6,7 +6,6 @@ DEF_HELPER_3(add_saturate, i32, env, i32, i32)
DEF_HELPER_3(sub_saturate, i32, env, i32, i32)
DEF_HELPER_3(add_usaturate, i32, env, i32, i32)
DEF_HELPER_3(sub_usaturate, i32, env, i32, i32)
-DEF_HELPER_2(double_saturate, i32, env, s32)
DEF_HELPER_FLAGS_2(sdiv, TCG_CALL_NO_RWG_SE, s32, s32, s32)
DEF_HELPER_FLAGS_2(udiv, TCG_CALL_NO_RWG_SE, i32, i32, i32)
DEF_HELPER_FLAGS_1(rbit, TCG_CALL_NO_RWG_SE, i32, i32)
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 5e1625a1c8..0fd4bd0238 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -135,21 +135,6 @@ uint32_t HELPER(sub_saturate)(CPUARMState *env, uint32_t a, uint32_t b)
return res;
}
-uint32_t HELPER(double_saturate)(CPUARMState *env, int32_t val)
-{
- uint32_t res;
- if (val >= 0x40000000) {
- res = ~SIGNBIT;
- env->QF = 1;
- } else if (val <= (int32_t)0xc0000000) {
- res = SIGNBIT;
- env->QF = 1;
- } else {
- res = val << 1;
- }
- return res;
-}
-
uint32_t HELPER(add_usaturate)(CPUARMState *env, uint32_t a, uint32_t b)
{
uint32_t res = a + b;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 2d447d4b90..846052acea 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8122,7 +8122,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
tmp = load_reg(s, rm);
tmp2 = load_reg(s, rn);
if (op1 & 2)
- gen_helper_double_saturate(tmp2, cpu_env, tmp2);
+ gen_helper_add_saturate(tmp2, cpu_env, tmp2, tmp2);
if (op1 & 1)
gen_helper_sub_saturate(tmp, cpu_env, tmp, tmp2);
else
@@ -9965,7 +9965,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
tmp = load_reg(s, rn);
tmp2 = load_reg(s, rm);
if (op & 1)
- gen_helper_double_saturate(tmp, cpu_env, tmp);
+ gen_helper_add_saturate(tmp, cpu_env, tmp, tmp);
if (op & 2)
gen_helper_sub_saturate(tmp, cpu_env, tmp2, tmp);
else
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc
2019-08-07 4:53 ` [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc Richard Henderson
@ 2019-08-07 17:27 ` Peter Maydell
2019-08-07 18:04 ` Richard Henderson
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2019-08-07 17:27 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Wed, 7 Aug 2019 at 05:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We currently have 3 different ways of computing the architectural
> value of "PC" as seen in the ARM ARM.
>
> The value of s->pc has been incremented past the current insn,
> but that is all. Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;
> for t16, PC = s->pc + 2. These differing computations make it
> impossible at present to unify the various code paths.
>
> With the newly introduced s->pc_curr, we can compute the correct
> value for all cases, using the formula given in the ARM ARM.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/translate.c | 59 ++++++++++++++++--------------------------
> 1 file changed, 23 insertions(+), 36 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 59e35aafbf..61933865d5 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -196,17 +196,17 @@ static inline void store_cpu_offset(TCGv_i32 var, int offset)
> #define store_cpu_field(var, name) \
> store_cpu_offset(var, offsetof(CPUARMState, name))
>
> +/* The architectural value of PC. */
> +static uint32_t read_pc(DisasContext *s)
> +{
> + return s->pc_curr + (s->thumb ? 4 : 8);
> +}
> +
> /* Set a variable to the value of a CPU register. */
> static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
> {
> if (reg == 15) {
> - uint32_t addr;
> - /* normally, since we updated PC, we need only to add one insn */
> - if (s->thumb)
> - addr = (long)s->pc + 2;
> - else
> - addr = (long)s->pc + 4;
> - tcg_gen_movi_i32(var, addr);
> + tcg_gen_movi_i32(var, read_pc(s));
So previously:
* for A32 we would return s->pc + 4, which is the same as s->pc_curr + 8
* for T16 we would return s->pc + 2, which is the same as s->pc_curr + 4
* for T32 we would return s->pc + 2 -- but that's not the same as
s->pc_curr + 4, it's s->pc_curr + 6...
Since s->pc_curr + 4 is the right architectural answer, are we
fixing a bug here? Or are all the places where T32 code calls
this function UNPREDICTABLE for the reg == 15 case ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32
2019-08-07 4:53 ` [Qemu-devel] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32 Richard Henderson
@ 2019-08-07 17:35 ` Philippe Mathieu-Daudé
2019-08-26 8:45 ` [Qemu-devel] [Qemu-arm] " Laurent Desnogues
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-07 17:35 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: peter.maydell, qemu-arm
On 8/7/19 6:53 AM, Richard Henderson wrote:
> Promote this function from aarch64 to fully general use.
> Use it to unify the code sequences for generating illegal
> opcode exceptions.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/translate-a64.h | 2 --
> target/arm/translate.h | 2 ++
> target/arm/translate-a64.c | 7 -------
> target/arm/translate-vfp.inc.c | 3 +--
> target/arm/translate.c | 22 ++++++++++++----------
> 5 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
> index 9cd2b3d238..12ad8ac6ed 100644
> --- a/target/arm/translate-a64.h
> +++ b/target/arm/translate-a64.h
> @@ -18,8 +18,6 @@
> #ifndef TARGET_ARM_TRANSLATE_A64_H
> #define TARGET_ARM_TRANSLATE_A64_H
>
> -void unallocated_encoding(DisasContext *s);
> -
> #define unsupported_encoding(s, insn) \
> do { \
> qemu_log_mask(LOG_UNIMP, \
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index de600073d8..6a65df0b27 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -98,6 +98,8 @@ typedef struct DisasCompare {
> bool value_global;
> } DisasCompare;
>
> +void unallocated_encoding(DisasContext *s);
> +
> /* Share the TCG temporaries common between 32 and 64 bit modes. */
> extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF;
> extern TCGv_i64 cpu_exclusive_addr;
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index d68bfc66d3..9e1ffe9cfb 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -352,13 +352,6 @@ static inline void gen_goto_tb(DisasContext *s, int n, uint64_t dest)
> }
> }
>
> -void unallocated_encoding(DisasContext *s)
> -{
> - /* Unallocated and reserved encodings are uncategorized */
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> -}
> -
> static void init_tmp_a64_array(DisasContext *s)
> {
> #ifdef CONFIG_DEBUG_TCG
> diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
> index 5065d4524c..3e8ea80493 100644
> --- a/target/arm/translate-vfp.inc.c
> +++ b/target/arm/translate-vfp.inc.c
> @@ -108,8 +108,7 @@ static bool full_vfp_access_check(DisasContext *s, bool ignore_vfp_enabled)
>
> if (!s->vfp_enabled && !ignore_vfp_enabled) {
> assert(!arm_dc_feature(s, ARM_FEATURE_M));
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> return false;
> }
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index d6b0ab7247..2d447d4b90 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1285,6 +1285,13 @@ static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syn)
> s->base.is_jmp = DISAS_NORETURN;
> }
>
> +void unallocated_encoding(DisasContext *s)
> +{
> + /* Unallocated and reserved encodings are uncategorized */
> + gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> + default_exception_el(s));
> +}
> +
> /* Force a TB lookup after an instruction that changes the CPU state. */
> static inline void gen_lookup_tb(DisasContext *s)
> {
> @@ -1315,8 +1322,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
> return;
> }
>
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> }
>
> static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
> @@ -7638,8 +7644,7 @@ static void gen_srs(DisasContext *s,
> }
>
> if (undef) {
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> return;
> }
>
> @@ -9266,8 +9271,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
> break;
> default:
> illegal_op:
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> break;
> }
> }
> @@ -10955,8 +10959,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
> }
> return;
> illegal_op:
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> }
>
> static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> @@ -11779,8 +11782,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> return;
> illegal_op:
> undef:
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> }
>
> static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
>
I wanted to suggest naming it gen_unallocated_encoding_exception() but
it is already used ~250 times, so no :)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
` (10 preceding siblings ...)
2019-08-07 4:53 ` [Qemu-devel] [PATCH 11/11] target/arm: Remove helper_double_saturate Richard Henderson
@ 2019-08-07 17:52 ` Peter Maydell
2019-08-09 12:49 ` Peter Maydell
11 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2019-08-07 17:52 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Wed, 7 Aug 2019 at 05:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> These are split out of my decodetree conversion of the
> aarch32 general instructions. With one exception, these
> are all related to cleaning up how we refer to "PC".
>
>
> r~
>
>
> Richard Henderson (11):
> target/arm: Pass in pc to thumb_insn_is_16bit
> target/arm: Introduce pc_curr
> target/arm: Introduce read_pc
> target/arm: Introduce add_reg_for_lit
> target/arm: Remove redundant s->pc & ~1
> target/arm: Replace s->pc with s->base.pc_next
> target/arm: Replace offset with pc in gen_exception_insn
> target/arm: Replace offset with pc in gen_exception_internal_insn
> target/arm: Remove offset argument to gen_exception_bkpt_insn
> target/arm: Use unallocated_encoding for aarch32
> target/arm: Remove helper_double_saturate
Whole series
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
I have one query on 3/11 but the change itself is clearly
correct so it's just a question of if we need to tweak the
commit message. Once we've figured that out I'll apply the
series to target-arm.next.
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc
2019-08-07 17:27 ` Peter Maydell
@ 2019-08-07 18:04 ` Richard Henderson
2019-08-07 18:16 ` Peter Maydell
0 siblings, 1 reply; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 18:04 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers
On 8/7/19 10:27 AM, Peter Maydell wrote:
>> +/* The architectural value of PC. */
>> +static uint32_t read_pc(DisasContext *s)
>> +{
>> + return s->pc_curr + (s->thumb ? 4 : 8);
>> +}
>> +
>> /* Set a variable to the value of a CPU register. */
>> static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
>> {
>> if (reg == 15) {
>> - uint32_t addr;
>> - /* normally, since we updated PC, we need only to add one insn */
>> - if (s->thumb)
>> - addr = (long)s->pc + 2;
>> - else
>> - addr = (long)s->pc + 4;
>> - tcg_gen_movi_i32(var, addr);
>> + tcg_gen_movi_i32(var, read_pc(s));
>
> So previously:
> * for A32 we would return s->pc + 4, which is the same as s->pc_curr + 8
> * for T16 we would return s->pc + 2, which is the same as s->pc_curr + 4
> * for T32 we would return s->pc + 2 -- but that's not the same as
> s->pc_curr + 4, it's s->pc_curr + 6...
>
> Since s->pc_curr + 4 is the right architectural answer, are we
> fixing a bug here? Or are all the places where T32 code calls
> this function UNPREDICTABLE for the reg == 15 case ?
I believe that this is UNPREDICTABLE.
The T32 cases that reference the PC that are not UNPREDICTABLE, literal memory
references and adr, are all of the form (s->pc & ~3) and do not come through
load_reg_var(). Those will be unified by add_reg_for_lit() in the next patch.
r~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc
2019-08-07 18:04 ` Richard Henderson
@ 2019-08-07 18:16 ` Peter Maydell
2019-08-07 18:25 ` Richard Henderson
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2019-08-07 18:16 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Wed, 7 Aug 2019 at 19:04, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/7/19 10:27 AM, Peter Maydell wrote:
> >> +/* The architectural value of PC. */
> >> +static uint32_t read_pc(DisasContext *s)
> >> +{
> >> + return s->pc_curr + (s->thumb ? 4 : 8);
> >> +}
> >> +
> >> /* Set a variable to the value of a CPU register. */
> >> static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
> >> {
> >> if (reg == 15) {
> >> - uint32_t addr;
> >> - /* normally, since we updated PC, we need only to add one insn */
> >> - if (s->thumb)
> >> - addr = (long)s->pc + 2;
> >> - else
> >> - addr = (long)s->pc + 4;
> >> - tcg_gen_movi_i32(var, addr);
> >> + tcg_gen_movi_i32(var, read_pc(s));
> >
> > So previously:
> > * for A32 we would return s->pc + 4, which is the same as s->pc_curr + 8
> > * for T16 we would return s->pc + 2, which is the same as s->pc_curr + 4
> > * for T32 we would return s->pc + 2 -- but that's not the same as
> > s->pc_curr + 4, it's s->pc_curr + 6...
> >
> > Since s->pc_curr + 4 is the right architectural answer, are we
> > fixing a bug here? Or are all the places where T32 code calls
> > this function UNPREDICTABLE for the reg == 15 case ?
>
> I believe that this is UNPREDICTABLE.
>
> The T32 cases that reference the PC that are not UNPREDICTABLE, literal memory
> references and adr, are all of the form (s->pc & ~3) and do not come through
> load_reg_var(). Those will be unified by add_reg_for_lit() in the next patch.
Yeah, that was my assumption -- at some previous point rather
than making load_reg/load_reg_var do the right thing for 32-bit
Thumb insns we just fixed up all the callsites to specialcase 15...
How about we add this to the commit message?
This changes the behaviour for load_reg() and load_reg_var()
when called with reg==15 from a 32-bit Thumb instruction:
previously they would have returned the incorrect value
of pc_curr + 6, and now they will return the architecturally
correct value of PC, which is pc_curr + 4. This will not
affect well-behaved guest software, because all of the places
we call these functions from T32 code are instructions where
using r15 is UNPREDICTABLE. Using the architectural PC value
here is more consistent with the T16 and A32 behaviour.
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc
2019-08-07 18:16 ` Peter Maydell
@ 2019-08-07 18:25 ` Richard Henderson
0 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2019-08-07 18:25 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers
On 8/7/19 11:16 AM, Peter Maydell wrote:
> How about we add this to the commit message?
>
> This changes the behaviour for load_reg() and load_reg_var()
> when called with reg==15 from a 32-bit Thumb instruction:
> previously they would have returned the incorrect value
> of pc_curr + 6, and now they will return the architecturally
> correct value of PC, which is pc_curr + 4. This will not
> affect well-behaved guest software, because all of the places
> we call these functions from T32 code are instructions where
> using r15 is UNPREDICTABLE. Using the architectural PC value
> here is more consistent with the T16 and A32 behaviour.
Looks good to me.
r~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 04/11] target/arm: Introduce add_reg_for_lit
2019-08-07 4:53 ` [Qemu-devel] [PATCH 04/11] target/arm: Introduce add_reg_for_lit Richard Henderson
@ 2019-08-08 5:43 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-08 5:43 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: peter.maydell, qemu-arm
On 8/7/19 6:53 AM, Richard Henderson wrote:
> Provide a common routine for the places that require ALIGN(PC, 4)
> as the base address as opposed to plain PC. The two are always
> the same for A32, but the difference is meaningful for thumb mode.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Note: This patch is easier to read with -b, as there are several
> large-ish indentation changes as the if statements fold together.
> ---
> target/arm/translate-vfp.inc.c | 38 ++------
> target/arm/translate.c | 166 +++++++++++++++------------------
> 2 files changed, 82 insertions(+), 122 deletions(-)
>
> diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
> index 092eb5ec53..262d4177e5 100644
> --- a/target/arm/translate-vfp.inc.c
> +++ b/target/arm/translate-vfp.inc.c
> @@ -941,14 +941,8 @@ static bool trans_VLDR_VSTR_sp(DisasContext *s, arg_VLDR_VSTR_sp *a)
> offset = -offset;
> }
>
> - if (s->thumb && a->rn == 15) {
> - /* This is actually UNPREDICTABLE */
> - addr = tcg_temp_new_i32();
> - tcg_gen_movi_i32(addr, s->pc & ~2);
> - } else {
> - addr = load_reg(s, a->rn);
> - }
> - tcg_gen_addi_i32(addr, addr, offset);
> + /* For thumb, use of PC is UNPREDICTABLE. */
> + addr = add_reg_for_lit(s, a->rn, offset);
> tmp = tcg_temp_new_i32();
> if (a->l) {
> gen_aa32_ld32u(s, tmp, addr, get_mem_index(s));
> @@ -983,14 +977,8 @@ static bool trans_VLDR_VSTR_dp(DisasContext *s, arg_VLDR_VSTR_dp *a)
> offset = -offset;
> }
>
> - if (s->thumb && a->rn == 15) {
> - /* This is actually UNPREDICTABLE */
> - addr = tcg_temp_new_i32();
> - tcg_gen_movi_i32(addr, s->pc & ~2);
> - } else {
> - addr = load_reg(s, a->rn);
> - }
> - tcg_gen_addi_i32(addr, addr, offset);
> + /* For thumb, use of PC is UNPREDICTABLE. */
> + addr = add_reg_for_lit(s, a->rn, offset);
> tmp = tcg_temp_new_i64();
> if (a->l) {
> gen_aa32_ld64(s, tmp, addr, get_mem_index(s));
> @@ -1029,13 +1017,8 @@ static bool trans_VLDM_VSTM_sp(DisasContext *s, arg_VLDM_VSTM_sp *a)
> return true;
> }
>
> - if (s->thumb && a->rn == 15) {
> - /* This is actually UNPREDICTABLE */
> - addr = tcg_temp_new_i32();
> - tcg_gen_movi_i32(addr, s->pc & ~2);
> - } else {
> - addr = load_reg(s, a->rn);
> - }
> + /* For thumb, use of PC is UNPREDICTABLE. */
> + addr = add_reg_for_lit(s, a->rn, 0);
> if (a->p) {
> /* pre-decrement */
> tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
> @@ -1112,13 +1095,8 @@ static bool trans_VLDM_VSTM_dp(DisasContext *s, arg_VLDM_VSTM_dp *a)
> return true;
> }
>
> - if (s->thumb && a->rn == 15) {
> - /* This is actually UNPREDICTABLE */
> - addr = tcg_temp_new_i32();
> - tcg_gen_movi_i32(addr, s->pc & ~2);
> - } else {
> - addr = load_reg(s, a->rn);
> - }
> + /* For thumb, use of PC is UNPREDICTABLE. */
> + addr = add_reg_for_lit(s, a->rn, 0);
> if (a->p) {
> /* pre-decrement */
> tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 61933865d5..71d94c053b 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -220,6 +220,23 @@ static inline TCGv_i32 load_reg(DisasContext *s, int reg)
> return tmp;
> }
>
> +/*
> + * Create a new temp, REG + OFS, except PC is ALIGN(PC, 4).
> + * This is used for load/store for which use of PC implies (literal),
> + * or ADD that implies ADR.
> + */
> +static 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);
> + } else {
> + tcg_gen_addi_i32(tmp, cpu_R[reg], ofs);
> + }
> + return tmp;
> +}
> +
> /* Set a CPU register. The source must be a temporary and will be
> marked as dead. */
> static void store_reg(DisasContext *s, int reg, TCGv_i32 var)
> @@ -9472,16 +9489,12 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
> */
> bool wback = extract32(insn, 21, 1);
>
> - if (rn == 15) {
> - if (insn & (1 << 21)) {
> - /* UNPREDICTABLE */
> - goto illegal_op;
> - }
> - addr = tcg_temp_new_i32();
> - tcg_gen_movi_i32(addr, s->pc & ~3);
> - } else {
> - addr = load_reg(s, rn);
> + if (rn == 15 && (insn & (1 << 21))) {
> + /* UNPREDICTABLE */
> + goto illegal_op;
> }
> +
> + addr = add_reg_for_lit(s, rn, 0);
> offset = (insn & 0xff) * 4;
> if ((insn & (1 << 23)) == 0) {
> offset = -offset;
> @@ -10682,27 +10695,15 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
> store_reg(s, rd, tmp);
> } else {
> /* Add/sub 12-bit immediate. */
> - if (rn == 15) {
> - offset = s->pc & ~(uint32_t)3;
> - if (insn & (1 << 23))
> - offset -= imm;
> - else
> - offset += imm;
> - tmp = tcg_temp_new_i32();
> - tcg_gen_movi_i32(tmp, offset);
> - store_reg(s, rd, tmp);
> + if (insn & (1 << 23)) {
> + imm = -imm;
:)
> + }
> + tmp = add_reg_for_lit(s, rn, imm);
> + if (rn == 13 && rd == 13) {
> + /* ADD SP, SP, imm or SUB SP, SP, imm */
> + store_sp_checked(s, tmp);
> } else {
> - tmp = load_reg(s, rn);
> - if (insn & (1 << 23))
> - tcg_gen_subi_i32(tmp, tmp, imm);
> - else
> - tcg_gen_addi_i32(tmp, tmp, imm);
> - if (rn == 13 && rd == 13) {
> - /* ADD SP, SP, imm or SUB SP, SP, imm */
> - store_sp_checked(s, tmp);
> - } else {
> - store_reg(s, rd, tmp);
> - }
> + store_reg(s, rd, tmp);
> }
> }
> }
> @@ -10816,61 +10817,53 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
> }
> }
> memidx = get_mem_index(s);
> - if (rn == 15) {
> - addr = tcg_temp_new_i32();
> - /* PC relative. */
> - /* s->pc has already been incremented by 4. */
> - imm = s->pc & 0xfffffffc;
> - if (insn & (1 << 23))
> - imm += insn & 0xfff;
> - else
> - imm -= insn & 0xfff;
> - tcg_gen_movi_i32(addr, imm);
> + imm = insn & 0xfff;
> + if (insn & (1 << 23)) {
> + /* PC relative or Positive offset. */
> + addr = add_reg_for_lit(s, rn, imm);
> + } else if (rn == 15) {
> + /* PC relative with negative offset. */
> + addr = add_reg_for_lit(s, rn, -imm);
> } else {
> addr = load_reg(s, rn);
> - if (insn & (1 << 23)) {
> - /* Positive offset. */
> - imm = insn & 0xfff;
> - tcg_gen_addi_i32(addr, addr, imm);
> - } else {
> - imm = insn & 0xff;
> - switch ((insn >> 8) & 0xf) {
> - case 0x0: /* Shifted Register. */
> - shift = (insn >> 4) & 0xf;
> - if (shift > 3) {
> - tcg_temp_free_i32(addr);
> - goto illegal_op;
> - }
> - tmp = load_reg(s, rm);
> - if (shift)
'diff -b' shows me you removed this 'if (shift)' but ...
> - tcg_gen_shli_i32(tmp, tmp, shift);
> - tcg_gen_add_i32(addr, addr, tmp);
> - tcg_temp_free_i32(tmp);
> - break;
> - case 0xc: /* Negative offset. */
> - tcg_gen_addi_i32(addr, addr, -imm);
> - break;
> - case 0xe: /* User privilege. */
> - tcg_gen_addi_i32(addr, addr, imm);
> - memidx = get_a32_user_mem_index(s);
> - break;
> - case 0x9: /* Post-decrement. */
> - imm = -imm;
> - /* Fall through. */
> - case 0xb: /* Post-increment. */
> - postinc = 1;
> - writeback = 1;
> - break;
> - case 0xd: /* Pre-decrement. */
> - imm = -imm;
> - /* Fall through. */
> - case 0xf: /* Pre-increment. */
> - writeback = 1;
> - break;
> - default:
> + imm = insn & 0xff;
> + switch ((insn >> 8) & 0xf) {
> + case 0x0: /* Shifted Register. */
> + shift = (insn >> 4) & 0xf;
> + if (shift > 3) {
> tcg_temp_free_i32(addr);
> goto illegal_op;
> }
> + tmp = load_reg(s, rm);
> + if (shift) {
... I'm seeing it here. Odd.
> + tcg_gen_shli_i32(tmp, tmp, shift);
> + }
> + tcg_gen_add_i32(addr, addr, tmp);
> + tcg_temp_free_i32(tmp);
> + break;
> + case 0xc: /* Negative offset. */
> + tcg_gen_addi_i32(addr, addr, -imm);
> + break;
> + case 0xe: /* User privilege. */
> + tcg_gen_addi_i32(addr, addr, imm);
> + memidx = get_a32_user_mem_index(s);
> + break;
> + case 0x9: /* Post-decrement. */
> + imm = -imm;
> + /* Fall through. */
> + case 0xb: /* Post-increment. */
> + postinc = 1;
> + writeback = 1;
> + break;
> + case 0xd: /* Pre-decrement. */
> + imm = -imm;
> + /* Fall through. */
> + case 0xf: /* Pre-increment. */
> + writeback = 1;
> + break;
> + default:
> + tcg_temp_free_i32(addr);
> + goto illegal_op;
> }
> }
>
> @@ -11066,10 +11059,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> if (insn & (1 << 11)) {
> rd = (insn >> 8) & 7;
> /* load pc-relative. Bit 1 of PC is ignored. */
> - val = read_pc(s) + ((insn & 0xff) * 4);
> - val &= ~(uint32_t)2;
> - addr = tcg_temp_new_i32();
> - tcg_gen_movi_i32(addr, val);
> + addr = add_reg_for_lit(s, 15, (insn & 0xff) * 4);
> tmp = tcg_temp_new_i32();
> gen_aa32_ld32u_iss(s, tmp, addr, get_mem_index(s),
> rd | ISSIs16Bit);
> @@ -11447,16 +11437,8 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> * - Add PC/SP (immediate)
> */
> rd = (insn >> 8) & 7;
> - if (insn & (1 << 11)) {
> - /* SP */
> - tmp = load_reg(s, 13);
> - } else {
> - /* PC. bit 1 is ignored. */
> - tmp = tcg_temp_new_i32();
> - tcg_gen_movi_i32(tmp, read_pc(s) & ~(uint32_t)2);
> - }
> val = (insn & 0xff) * 4;
> - tcg_gen_addi_i32(tmp, tmp, val);
> + tmp = add_reg_for_lit(s, insn & (1 << 11) ? 13 : 15, val);
> store_reg(s, rd, tmp);
> break;
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 01/11] target/arm: Pass in pc to thumb_insn_is_16bit
2019-08-07 4:53 ` [Qemu-devel] [PATCH 01/11] target/arm: Pass in pc to thumb_insn_is_16bit Richard Henderson
@ 2019-08-08 5:47 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-08 5:47 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: peter.maydell, qemu-arm
On 8/7/19 6:53 AM, Richard Henderson wrote:
> This function is used in two different contexts, and it will be
> clearer if the function is given the address to which it applies.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> target/arm/translate.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 7853462b21..1f15f14022 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -9261,11 +9261,11 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
> }
> }
>
> -static bool thumb_insn_is_16bit(DisasContext *s, uint32_t insn)
> +static bool thumb_insn_is_16bit(DisasContext *s, uint32_t pc, uint32_t insn)
> {
> - /* Return true if this is a 16 bit instruction. We must be precise
> - * about this (matching the decode). We assume that s->pc still
> - * points to the first 16 bits of the insn.
> + /*
> + * Return true if this is a 16 bit instruction. We must be precise
> + * about this (matching the decode).
> */
> if ((insn >> 11) < 0x1d) {
> /* Definitely a 16-bit instruction */
> @@ -9285,7 +9285,7 @@ static bool thumb_insn_is_16bit(DisasContext *s, uint32_t insn)
> return false;
> }
>
> - if ((insn >> 11) == 0x1e && s->pc - s->page_start < TARGET_PAGE_SIZE - 3) {
> + if ((insn >> 11) == 0x1e && pc - s->page_start < TARGET_PAGE_SIZE - 3) {
> /* 0b1111_0xxx_xxxx_xxxx : BL/BLX prefix, and the suffix
> * is not on the next page; we merge this into a 32-bit
> * insn.
> @@ -11824,7 +11824,7 @@ static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
> */
> uint16_t insn = arm_lduw_code(env, s->pc, s->sctlr_b);
>
> - return !thumb_insn_is_16bit(s, insn);
> + return !thumb_insn_is_16bit(s, s->pc, insn);
> }
>
> static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> @@ -12122,7 +12122,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
> }
>
> insn = arm_lduw_code(env, dc->pc, dc->sctlr_b);
> - is_16bit = thumb_insn_is_16bit(dc, insn);
> + is_16bit = thumb_insn_is_16bit(dc, dc->pc, insn);
> dc->pc += 2;
> if (!is_16bit) {
> uint32_t insn2 = arm_lduw_code(env, dc->pc, dc->sctlr_b);
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches
2019-08-07 17:52 ` [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Peter Maydell
@ 2019-08-09 12:49 ` Peter Maydell
0 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2019-08-09 12:49 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Wed, 7 Aug 2019 at 18:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 7 Aug 2019 at 05:53, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > These are split out of my decodetree conversion of the
> > aarch32 general instructions. With one exception, these
> > are all related to cleaning up how we refer to "PC".
> >
> >
> > r~
> >
> >
> > Richard Henderson (11):
> > target/arm: Pass in pc to thumb_insn_is_16bit
> > target/arm: Introduce pc_curr
> > target/arm: Introduce read_pc
> > target/arm: Introduce add_reg_for_lit
> > target/arm: Remove redundant s->pc & ~1
> > target/arm: Replace s->pc with s->base.pc_next
> > target/arm: Replace offset with pc in gen_exception_insn
> > target/arm: Replace offset with pc in gen_exception_internal_insn
> > target/arm: Remove offset argument to gen_exception_bkpt_insn
> > target/arm: Use unallocated_encoding for aarch32
> > target/arm: Remove helper_double_saturate
>
> Whole series
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> I have one query on 3/11 but the change itself is clearly
> correct so it's just a question of if we need to tweak the
> commit message. Once we've figured that out I'll apply the
> series to target-arm.next.
Now applied to target-arm.next with the agreed commit tweak.
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32
2019-08-07 4:53 ` [Qemu-devel] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32 Richard Henderson
2019-08-07 17:35 ` Philippe Mathieu-Daudé
@ 2019-08-26 8:45 ` Laurent Desnogues
1 sibling, 0 replies; 22+ messages in thread
From: Laurent Desnogues @ 2019-08-26 8:45 UTC (permalink / raw)
To: Richard Henderson; +Cc: Peter Maydell, qemu-arm, qemu-devel
Hi Richard,
On Wed, Aug 7, 2019 at 6:54 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Promote this function from aarch64 to fully general use.
> Use it to unify the code sequences for generating illegal
> opcode exceptions.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/translate-a64.h | 2 --
> target/arm/translate.h | 2 ++
> target/arm/translate-a64.c | 7 -------
> target/arm/translate-vfp.inc.c | 3 +--
> target/arm/translate.c | 22 ++++++++++++----------
> 5 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
> index 9cd2b3d238..12ad8ac6ed 100644
> --- a/target/arm/translate-a64.h
> +++ b/target/arm/translate-a64.h
> @@ -18,8 +18,6 @@
> #ifndef TARGET_ARM_TRANSLATE_A64_H
> #define TARGET_ARM_TRANSLATE_A64_H
>
> -void unallocated_encoding(DisasContext *s);
> -
> #define unsupported_encoding(s, insn) \
> do { \
> qemu_log_mask(LOG_UNIMP, \
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index de600073d8..6a65df0b27 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -98,6 +98,8 @@ typedef struct DisasCompare {
> bool value_global;
> } DisasCompare;
>
> +void unallocated_encoding(DisasContext *s);
> +
> /* Share the TCG temporaries common between 32 and 64 bit modes. */
> extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF;
> extern TCGv_i64 cpu_exclusive_addr;
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index d68bfc66d3..9e1ffe9cfb 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -352,13 +352,6 @@ static inline void gen_goto_tb(DisasContext *s, int n, uint64_t dest)
> }
> }
>
> -void unallocated_encoding(DisasContext *s)
> -{
> - /* Unallocated and reserved encodings are uncategorized */
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> -}
> -
> static void init_tmp_a64_array(DisasContext *s)
> {
> #ifdef CONFIG_DEBUG_TCG
> diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
> index 5065d4524c..3e8ea80493 100644
> --- a/target/arm/translate-vfp.inc.c
> +++ b/target/arm/translate-vfp.inc.c
> @@ -108,8 +108,7 @@ static bool full_vfp_access_check(DisasContext *s, bool ignore_vfp_enabled)
>
> if (!s->vfp_enabled && !ignore_vfp_enabled) {
> assert(!arm_dc_feature(s, ARM_FEATURE_M));
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> return false;
> }
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index d6b0ab7247..2d447d4b90 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1285,6 +1285,13 @@ static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syn)
> s->base.is_jmp = DISAS_NORETURN;
> }
>
> +void unallocated_encoding(DisasContext *s)
> +{
> + /* Unallocated and reserved encodings are uncategorized */
> + gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> + default_exception_el(s));
> +}
Isn't the move of unallocated_encoding from translate-64.c to
translate.c causing issues since gen_exception_insn isn't the same?
In particular in one case the pc is 64-bit while now it's always
32-bit. Did I miss something?
Thanks,
Laurent
> +
> /* Force a TB lookup after an instruction that changes the CPU state. */
> static inline void gen_lookup_tb(DisasContext *s)
> {
> @@ -1315,8 +1322,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
> return;
> }
>
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> }
>
> static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
> @@ -7638,8 +7644,7 @@ static void gen_srs(DisasContext *s,
> }
>
> if (undef) {
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> return;
> }
>
> @@ -9266,8 +9271,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
> break;
> default:
> illegal_op:
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> break;
> }
> }
> @@ -10955,8 +10959,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
> }
> return;
> illegal_op:
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> }
>
> static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> @@ -11779,8 +11782,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> return;
> illegal_op:
> undef:
> - gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> - default_exception_el(s));
> + unallocated_encoding(s);
> }
>
> static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-08-26 8:47 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-07 4:53 [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 01/11] target/arm: Pass in pc to thumb_insn_is_16bit Richard Henderson
2019-08-08 5:47 ` Philippe Mathieu-Daudé
2019-08-07 4:53 ` [Qemu-devel] [PATCH 02/11] target/arm: Introduce pc_curr Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc Richard Henderson
2019-08-07 17:27 ` Peter Maydell
2019-08-07 18:04 ` Richard Henderson
2019-08-07 18:16 ` Peter Maydell
2019-08-07 18:25 ` Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 04/11] target/arm: Introduce add_reg_for_lit Richard Henderson
2019-08-08 5:43 ` Philippe Mathieu-Daudé
2019-08-07 4:53 ` [Qemu-devel] [PATCH 05/11] target/arm: Remove redundant s->pc & ~1 Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 06/11] target/arm: Replace s->pc with s->base.pc_next Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 07/11] target/arm: Replace offset with pc in gen_exception_insn Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 08/11] target/arm: Replace offset with pc in gen_exception_internal_insn Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 09/11] target/arm: Remove offset argument to gen_exception_bkpt_insn Richard Henderson
2019-08-07 4:53 ` [Qemu-devel] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32 Richard Henderson
2019-08-07 17:35 ` Philippe Mathieu-Daudé
2019-08-26 8:45 ` [Qemu-devel] [Qemu-arm] " Laurent Desnogues
2019-08-07 4:53 ` [Qemu-devel] [PATCH 11/11] target/arm: Remove helper_double_saturate Richard Henderson
2019-08-07 17:52 ` [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches Peter Maydell
2019-08-09 12:49 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).