Re: [qemu-s390x] [PATCH 3/3] target/s390x: convert to TranslatorOps

[David Hildenbrand]

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 <address@hidden>
> ---
>  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