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

[Aurelien Jarno]

On 2015-06-29 09:57, Pavel Dovgaluk wrote:
> > 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.

Ok, it's all fine then.

> > 
> > >  /* 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.

I missed that. Thanks for pointing me to that. That looks fine then. 

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net