Richard Henderson <richard.hender...@linaro.org> writes:
> On 10/11/21 8:57 AM, Alex Bennée wrote:
>> We use INDEX_op_insn_start to make the start of instruction
>> boundaries. If we don't do it in the .insn_start hook things get
>> confused especially now plugins want to use that marking to identify
>> the start of instructions and will bomb out if it sees instrumented
>> ops before the first instruction boundary.
>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>> ---
>> target/s390x/tcg/translate.c | 25 ++++++++++++++++++++++---
>> 1 file changed, 22 insertions(+), 3 deletions(-)
>> diff --git a/target/s390x/tcg/translate.c
>> b/target/s390x/tcg/translate.c
>> index f284870cd2..fe145ff2eb 100644
>> --- a/target/s390x/tcg/translate.c
>> +++ b/target/s390x/tcg/translate.c
>> @@ -6380,9 +6380,6 @@ static DisasJumpType translate_one(CPUS390XState *env,
>> DisasContext *s)
>> /* Search for the insn in the table. */
>> insn = extract_insn(env, s);
>> - /* Emit insn_start now that we know the ILEN. */
>> - tcg_gen_insn_start(s->base.pc_next, s->cc_op, s->ilen);
>> -
>> /* Not found means unimplemented/illegal opcode. */
>> if (insn == NULL) {
>> qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n",
>> @@ -6550,8 +6547,30 @@ static void s390x_tr_tb_start(DisasContextBase *db,
>> CPUState *cs)
>> {
>> }
>> +/*
>> + * We just enough partial instruction decoding here to calculate the
>> + * length of the instruction so we can drop the INDEX_op_insn_start
>> + * before anything else is emitted in the TCGOp stream.
>> + *
>> + * See extract_insn for the full decode.
>> + */
>> static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>> {
>> + CPUS390XState *env = cs->env_ptr;
>> + DisasContext *s = container_of(dcbase, DisasContext, base);
>> + uint64_t insn, pc = s->base.pc_next;
>> + int op, ilen;
>> +
>> + if (unlikely(s->ex_value)) {
>> + ilen = s->ex_value & 0xf;
>> + } else {
>> + insn = ld_code2(env, s, pc); /* FIXME: don't reload same pc twice
>> */
>
> I think we might as well delay the set of ilen until after the normal
> load, rather than introduce a fixme.
I'd got as far as this before thinking it was getting messy:
--8<---------------cut here---------------start------------->8---
squash! target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start
1 file changed, 16 insertions(+), 19 deletions(-)
target/s390x/tcg/translate.c | 35 ++++++++++++++++-------------------
modified target/s390x/tcg/translate.c
@@ -147,6 +147,7 @@ struct DisasContext {
*/
uint64_t pc_tmp;
uint32_t ilen;
+ uint16_t start_of_insn; /* collected at s390x_tr_insn_start */
enum cc_op cc_op;
bool do_debug;
};
@@ -388,10 +389,10 @@ static void update_cc_op(DisasContext *s)
}
}
-static inline uint64_t ld_code2(CPUS390XState *env, DisasContext *s,
+static inline uint16_t ld_code2(CPUS390XState *env, DisasContext *s,
uint64_t pc)
{
- return (uint64_t)translator_lduw(env, &s->base, pc);
+ return translator_lduw(env, &s->base, pc);
}
static inline uint64_t ld_code4(CPUS390XState *env, DisasContext *s,
@@ -6261,7 +6262,7 @@ static void extract_field(DisasFields *o, const
DisasField *f, uint64_t insn)
static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
{
uint64_t insn, pc = s->base.pc_next;
- int op, op2, ilen;
+ int op, op2;
const DisasInsn *info;
if (unlikely(s->ex_value)) {
@@ -6272,28 +6273,25 @@ static const DisasInsn *extract_insn(CPUS390XState
*env, DisasContext *s)
/* Extract the values saved by EXECUTE. */
insn = s->ex_value & 0xffffffffffff0000ull;
- ilen = s->ex_value & 0xf;
+ s->ilen = s->ex_value & 0xf;
op = insn >> 56;
} else {
- insn = ld_code2(env, s, pc);
- op = (insn >> 8) & 0xff;
- ilen = get_ilen(op);
- switch (ilen) {
+ op = extract32(s->start_of_insn, 8, 8);
+ insn = deposit64(0, 48, 16, s->start_of_insn);
+ switch (s->ilen) {
case 2:
- insn = insn << 48;
break;
case 4:
- insn = ld_code4(env, s, pc) << 32;
+ insn = deposit64(insn, 32, 16, ld_code2(env, s, pc + 2));
break;
case 6:
- insn = (insn << 48) | (ld_code4(env, s, pc + 2) << 16);
+ insn = deposit64(insn, 16, 32, ld_code4(env, s, pc + 2));
break;
default:
g_assert_not_reached();
}
}
- s->pc_tmp = s->base.pc_next + ilen;
- s->ilen = ilen;
+ s->pc_tmp = s->base.pc_next + s->ilen;
/* We can't actually determine the insn format until we've looked up
the full insn opcode. Which we can't do without locating the
@@ -6558,18 +6556,17 @@ static void s390x_tr_insn_start(DisasContextBase
*dcbase, CPUState *cs)
{
CPUS390XState *env = cs->env_ptr;
DisasContext *s = container_of(dcbase, DisasContext, base);
- uint64_t insn, pc = s->base.pc_next;
- int op, ilen;
+ uint64_t pc = s->base.pc_next;
+ int ilen;
if (unlikely(s->ex_value)) {
ilen = s->ex_value & 0xf;
} else {
- insn = ld_code2(env, s, pc); /* FIXME: don't reload same pc twice */
- op = (insn >> 8) & 0xff;
- ilen = get_ilen(op);
+ s->start_of_insn = ld_code2(env, s, pc);
+ ilen = get_ilen(extract64(s->start_of_insn, 8, 8));
}
- /* Emit insn_start now that we know the ILEN. */
+ s->ilen = ilen;
tcg_gen_insn_start(s->base.pc_next, s->cc_op, ilen);
}
--8<---------------cut here---------------end--------------->8---
--
Alex Bennée
