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

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

2015-07-01 Thread Aurelien Jarno

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 
> > > ---
> > >  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

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

2015-06-28 Thread Pavel Dovgaluk

> 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 
> > ---
> >  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

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

2015-06-23 Thread Aurelien Jarno

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?

> +}
> +
> +static inline void QEMU_NORETURN do_raise_exception(CPUMIPSState *env,
> +uint32_t exception,
> +uintptr_t pc)
> +{
> +do_raise_exception_err(env, exception, 0, pc);
> +}
> +
>  #endif /* !defined (__MIPS_CPU_H__) */
> diff --git a/target-mips/helper.h b/target-mips/helper.h
> index 3bd0b02..50c699e 100644
> --- a/target-mips/helper.h
> +++ b/target-mips/helper.h
> @@ -1,5 +1,6 @@
>  DEF_HELPER_3(raise_exception_err, noreturn, env, i32, int)
>  DEF_HELPER_2(raise_exception, noreturn, env, i32)
> +DEF_HELPER_1(raise_exception_debug, noreturn, env)
>  
>  #ifdef TARGET_MIPS64
>  DEF_HELPER_4(sdl, void, env, tl, tl, int)
> diff --git a/target-mips/msa_helper.c b/target-mips/msa_helper.c
> index 26ffdc7..f7bc710 100644
> --- a/target-mips/msa_helper.c
> +++ b/target-mips/msa_helper.c
> @@ -1352,7 +1352,7 @@ void helper_msa_ctcmsa(CPUMIPSState *env, target_ulong 
> elm, uint32_t cd)
>  /* check exception */
>  if ((GET_FP_ENABLE(env->active_tc.msacsr) | FP_UNIMPLEMENTED)
>  & GET_FP_CAUSE(env->active_tc.msacsr)) {
> -helper_raise_exception(env, EXCP_MSAFPE);
> +do_raise_exception(env, EXCP_MSAFPE, GETPC());
>  }
>  break;
>  }
> @@ -1512,7 +1512,8 @@ static inline void check_msacsr_cause(CPUMIPSState *env)
>  UPDATE_FP_FLAGS(env->active_tc.msacsr,
>  GET_FP_CAUSE(env->active_tc.msacsr));
>  } else {
> -helper_raise_exception(env, EXCP_MSAFPE);
> +/* Will work only when check_msacsr_cause is actually inlined */
> +do_raise_exception(env, EXCP_MSAFPE, GETPC());

Then that might not work. Instead of adding this comment, please change
the function to always_inline, or (probably better) change it to take
the return address in argument.

>  }
>  }
>  
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 73a8e45..077ea94 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -30,41 +30,23 @@ static inline void cpu_mips_tlb_flush (CPUMIPSState *env, 
> int flush_global);
>  
> /*/
>  /* Exceptions processing helpers */
>  
> -static inline void QEMU_NORETURN do_raise_exception_err(CPUMIPSState *env,
> -uint32_t exception,
> -int error_code,
> -uintptr_t pc)
> +void helper_raise_exception_err(CPUMIPSState *env, uint32_t exception,
> +int error_code)
>  {

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

2015-06-18 Thread Pavel Dovgalyuk

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);
+}
+
+static inline void QEMU_NORETURN do_raise_exception(CPUMIPSState *env,
+uint32_t exception,
+uintptr_t pc)
+{
+do_raise_exception_err(env, exception, 0, pc);
+}
+
 #endif /* !defined (__MIPS_CPU_H__) */
diff --git a/target-mips/helper.h b/target-mips/helper.h
index 3bd0b02..50c699e 100644
--- a/target-mips/helper.h
+++ b/target-mips/helper.h
@@ -1,5 +1,6 @@
 DEF_HELPER_3(raise_exception_err, noreturn, env, i32, int)
 DEF_HELPER_2(raise_exception, noreturn, env, i32)
+DEF_HELPER_1(raise_exception_debug, noreturn, env)
 
 #ifdef TARGET_MIPS64
 DEF_HELPER_4(sdl, void, env, tl, tl, int)
diff --git a/target-mips/msa_helper.c b/target-mips/msa_helper.c
index 26ffdc7..f7bc710 100644
--- a/target-mips/msa_helper.c
+++ b/target-mips/msa_helper.c
@@ -1352,7 +1352,7 @@ void helper_msa_ctcmsa(CPUMIPSState *env, target_ulong 
elm, uint32_t cd)
 /* check exception */
 if ((GET_FP_ENABLE(env->active_tc.msacsr) | FP_UNIMPLEMENTED)
 & GET_FP_CAUSE(env->active_tc.msacsr)) {
-helper_raise_exception(env, EXCP_MSAFPE);
+do_raise_exception(env, EXCP_MSAFPE, GETPC());
 }
 break;
 }
@@ -1512,7 +1512,8 @@ static inline void check_msacsr_cause(CPUMIPSState *env)
 UPDATE_FP_FLAGS(env->active_tc.msacsr,
 GET_FP_CAUSE(env->active_tc.msacsr));
 } else {
-helper_raise_exception(env, EXCP_MSAFPE);
+/* Will work only when check_msacsr_cause is actually inlined */
+do_raise_exception(env, EXCP_MSAFPE, GETPC());
 }
 }
 
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 73a8e45..077ea94 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -30,41 +30,23 @@ static inline void cpu_mips_tlb_flush (CPUMIPSState *env, 
int flush_global);
 /*/
 /* Exceptions processing helpers */
 
-static inline void QEMU_NORETURN do_raise_exception_err(CPUMIPSState *env,
-uint32_t exception,
-int error_code,
-uintptr_t pc)
+void helper_raise_exception_err(CPUMIPSState *env, uint32_t exception,
+int error_code)
 {
-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);
+do_raise_exception_err(env, exception, error_code, 0);
 }
 
-static inline void QEMU_NORETURN do_raise_exception(CPUMIPSState *env,
-uint32_t exception,
-

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information