On 17.02.2018 00:40, Emilio G. Cota wrote:
Can you keep DisasContext named dc instead of s? Avoids unnecessary
changes. (e.g. s390x_tr_tb_stop()). And also matches what e.g. i386 does
in their code.
> Signed-off-by: Emilio G. Cota <c...@braap.org>
> ---
> target/s390x/translate.c | 170
> ++++++++++++++++++++++++-----------------------
> 1 file changed, 86 insertions(+), 84 deletions(-)
>
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index dd504a1..2b27a69 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -55,10 +55,12 @@ struct DisasContext {
> DisasContextBase base;
> const DisasInsn *insn;
> DisasFields *fields;
> + uint64_t next_page_start;
> uint64_t ex_value;
> uint64_t pc, next_pc;
> uint32_t ilen;
> enum cc_op cc_op;
> + bool do_debug;
> };
>
> /* Information carried about a condition to be evaluated. */
> @@ -6108,101 +6110,92 @@ static DisasJumpType translate_one(CPUS390XState
> *env, DisasContext *s)
> return ret;
> }
>
> -void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
> +static int s390x_tr_init_disas_context(DisasContextBase *dcbase,
> + CPUState *cs, int max_insns)
> {
> - CPUS390XState *env = cs->env_ptr;
> - DisasContext dc;
> - uint64_t next_page_start;
> - int num_insns, max_insns;
> - DisasJumpType status;
> - bool do_debug;
> + DisasContext *s = container_of(dcbase, DisasContext, base);
>
> - dc.base.pc_first = tb->pc;
> /* 31-bit mode */
> - if (!(tb->flags & FLAG_MASK_64)) {
> - dc.base.pc_first &= 0x7fffffff;
> + if (!(s->base.tb->flags & FLAG_MASK_64)) {
> + s->base.pc_first &= 0x7fffffff;
> + s->base.pc_next = s->base.pc_first;
> }
> - dc.base.pc_next = dc.base.pc_first;
> - dc.base.tb = tb;
> - dc.base.singlestep_enabled = cs->singlestep_enabled;
>
> - dc.pc = dc.base.pc_first;
> - dc.cc_op = CC_OP_DYNAMIC;
> - dc.ex_value = dc.base.tb->cs_base;
> - do_debug = cs->singlestep_enabled;
> + s->pc = s->base.pc_first;
> + s->cc_op = CC_OP_DYNAMIC;
> + s->ex_value = s->base.tb->cs_base;
> + s->do_debug = s->base.singlestep_enabled;
> + s->next_page_start = (s->base.pc_first & TARGET_PAGE_MASK) +
> + TARGET_PAGE_SIZE;
Does it really make sense to store this pre calculation? Can't you
simply do that in ops->translate_insn?
> +static bool s390x_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
> + const CPUBreakpoint *bp)
> +{
> + DisasContext *s = container_of(dcbase, DisasContext, base);
>
> - if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
> - gen_io_start();
> - }
> + s->base.is_jmp = DISAS_PC_STALE;
> + s->do_debug = true;
Can we drop s->do_debug and instead use DISAS_TOO_MANY like the other
architectures? (if I understood that part correctly :) )
> + /* 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 increment the PC here so that
> + the logic setting tb->size below does the right thing. */
> + s->pc += 2;
> + s->base.pc_next += 2;
> + return true;
> +}
>
--
Thanks,
David / dhildenb
