On 2015-06-29 09:57, Pavel Dovgaluk wrote: > > From: Aurelien Jarno [mailto:aurel...@aurel32.net] > > 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 <pavel.dovga...@ispras.ru> > > > --- > > > 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 aurel...@aurel32.net http://www.aurel32.net