From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36259) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAL0q-0001AH-CC for qemu-devel@nongnu.org; Wed, 01 Jul 2015 12:37:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAL0p-0000dJ-1R for qemu-devel@nongnu.org; Wed, 01 Jul 2015 12:37:28 -0400 Received: from hall.aurel32.net ([2001:bc8:30d7:100::1]:48086) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAL0o-0000ch-Pg for qemu-devel@nongnu.org; Wed, 01 Jul 2015 12:37:26 -0400 Date: Wed, 1 Jul 2015 18:37:22 +0200 From: Aurelien Jarno Message-ID: <20150701163722.GF11361@aurel32.net> References: <20150618132816.9200.29413.stgit@PASHA-ISP.def.inno> <20150618132828.9200.48645.stgit@PASHA-ISP.def.inno> <20150623233241.GB11474@aurel32.net> <000801d0b238$e02072c0$a0615840$@Dovgaluk@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <000801d0b238$e02072c0$a0615840$@Dovgaluk@ispras.ru> Subject: Re: [Qemu-devel] [PATCH v3 2/3] target-mips: improve exceptions handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Dovgaluk Cc: pbonzini@redhat.com, rth7680@gmail.com, leon.alrae@imgtec.com, qemu-devel@nongnu.org On 2015-06-29 09:57, Pavel Dovgaluk wrote: > > From: Aurelien Jarno [mailto:aurelien@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 > > > --- > > > 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 aurelien@aurel32.net http://www.aurel32.net