Re: [Qemu-devel] [PATCH v3 2/3] target-mips: improve exceptions handling

[Pavel Dovgaluk]

> From: Aurelien Jarno [mailto:address@hidden
> On 2015-06-18 16:28, Pavel Dovgalyuk wrote:
> > This patch improves exception handling in MIPS.
> > Instructions generate several types of exceptions.
> > When exception is generated, it breaks the execution of the current 
> > translation
> > block. Implementation of the exceptions handling does not correctly
> > restore icount for the instruction which caused the exception. In most cases
> > icount will be decreased by the value equal to the size of TB.
> > This patch passes pointer to the translation block internals to the 
> > exception
> > handler. It allows correct restoring of the icount value.
> >
> > v3 changes:
> > This patch stops translation when instruction which always generates 
> > exception
> > is translated. This improves the performance of the patched version compared
> > to original one.
> >
> > Signed-off-by: Pavel Dovgalyuk <address@hidden>
> > ---
> >  target-mips/cpu.h        |   28 +++
> >  target-mips/helper.h     |    1
> >  target-mips/msa_helper.c |    5 -
> >  target-mips/op_helper.c  |  183 ++++++++++------------
> >  target-mips/translate.c  |  379 
> > ++++++++++++++++++++++------------------------
> >  5 files changed, 302 insertions(+), 294 deletions(-)
> >
> > diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> > index f9d2b4c..70ba39a 100644
> > --- a/target-mips/cpu.h
> > +++ b/target-mips/cpu.h
> > @@ -1015,4 +1015,32 @@ static inline void cpu_mips_store_cause(CPUMIPSState 
> > *env,
> target_ulong val)
> >  }
> >  #endif
> >
> > +static inline void QEMU_NORETURN do_raise_exception_err(CPUMIPSState *env,
> > +                                                        uint32_t exception,
> > +                                                        int error_code,
> > +                                                        uintptr_t pc)
> > +{
> > +    CPUState *cs = CPU(mips_env_get_cpu(env));
> > +
> > +    if (exception < EXCP_SC) {
> > +        qemu_log("%s: %d %d\n", __func__, exception, error_code);
> > +    }
> > +    cs->exception_index = exception;
> > +    env->error_code = error_code;
> > +
> > +    if (pc) {
> > +        /* now we have a real cpu fault */
> > +        cpu_restore_state(cs, pc);
> > +    }
> > +
> > +    cpu_loop_exit(cs);
> 
> What about creating a cpu_loop_exit_restore(cs, pc) (maybe with a better
> name?) in the common code, if we now have to repeat this pattern for
> every target?

Ok.

> > diff --git a/target-mips/translate.c b/target-mips/translate.c
> > index fd063a2..f87d5ac 100644
> > --- a/target-mips/translate.c
> > +++ b/target-mips/translate.c
> > @@ -1677,15 +1677,21 @@ generate_exception_err (DisasContext *ctx, int 
> > excp, int err)
> >      gen_helper_raise_exception_err(cpu_env, texcp, terr);
> >      tcg_temp_free_i32(terr);
> >      tcg_temp_free_i32(texcp);
> > +    ctx->bstate = BS_STOP;
> >  }
> >
> >  static inline void
> >  generate_exception (DisasContext *ctx, int excp)
> >  {
> > -    save_cpu_state(ctx, 1);
> >      gen_helper_0e0i(raise_exception, excp);
> >  }
> >
> > +static inline void
> > +generate_exception_end(DisasContext *ctx, int excp)
> > +{
> > +    generate_exception_err(ctx, excp, 0);
> > +}
> > +
> 
> This sets error_code to 0, which is different than leaving it unchanged.
> This might be ok, but have you checked there is no side effect?

Previous version called do_raise_exception, which passes 0 as error_code.

> 
> >  /* Addresses computation */
> >  static inline void gen_op_addr_add (DisasContext *ctx, TCGv ret, TCGv 
> > arg0, TCGv arg1)
> >  {
> > @@ -1731,7 +1737,7 @@ static inline void check_cp1_enabled(DisasContext 
> > *ctx)
> >  static inline void check_cop1x(DisasContext *ctx)
> >  {
> >      if (unlikely(!(ctx->hflags & MIPS_HFLAG_COP1X)))
> > -        generate_exception(ctx, EXCP_RI);
> > +        generate_exception_end(ctx, EXCP_RI);
> 
> I don't think it is correct. Before triggering such an exception, we
> were saving the CPU state, and not going through retranslation. With
> this change, we don't save the CPU state, but we don't go through
> retranslation either.
> 
> The rule is to either go through retranslation, or to save the CPU state
> before a possible exception.

generate_exception_end saves CPU state and stops the translation
through calling of generate_exception_err function.


Pavel Dovgalyuk