* [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start @ 2021-10-11 15:57 Alex Bennée 2021-10-11 18:34 ` Richard Henderson 0 siblings, 1 reply; 3+ messages in thread From: Alex Bennée @ 2021-10-11 15:57 UTC (permalink / raw) To: qemu-devel Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, richard.henderson, open list:S390 TCG CPUs, Alex Bennée 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.bennee@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 */ + op = (insn >> 8) & 0xff; + ilen = get_ilen(op); + } + + /* Emit insn_start now that we know the ILEN. */ + tcg_gen_insn_start(s->base.pc_next, s->cc_op, ilen); } static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs) -- 2.30.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start 2021-10-11 15:57 [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start Alex Bennée @ 2021-10-11 18:34 ` Richard Henderson 2021-10-11 21:31 ` Alex Bennée 0 siblings, 1 reply; 3+ messages in thread From: Richard Henderson @ 2021-10-11 18:34 UTC (permalink / raw) To: Alex Bennée, qemu-devel Cc: Thomas Huth, Cornelia Huck, open list:S390 TCG CPUs, David Hildenbrand 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.bennee@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. r~ > + op = (insn >> 8) & 0xff; > + ilen = get_ilen(op); > + } > + > + /* Emit insn_start now that we know the ILEN. */ > + tcg_gen_insn_start(s->base.pc_next, s->cc_op, ilen); > } > > static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs) > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start 2021-10-11 18:34 ` Richard Henderson @ 2021-10-11 21:31 ` Alex Bennée 0 siblings, 0 replies; 3+ messages in thread From: Alex Bennée @ 2021-10-11 21:31 UTC (permalink / raw) To: Richard Henderson Cc: open list:S390 TCG CPUs, Thomas Huth, Cornelia Huck, qemu-devel, David Hildenbrand Richard Henderson <richard.henderson@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.bennee@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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-11 21:35 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-11 15:57 [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start Alex Bennée 2021-10-11 18:34 ` Richard Henderson 2021-10-11 21:31 ` Alex Bennée
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).