[Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Pavel Dovgalyuk]

This patch fixes exception handling in MIPS.
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 in MIPS 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.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 target-mips/cpu.h        |   28 +++++++++++++++++++++++++
 target-mips/msa_helper.c |    5 +++-
 target-mips/op_helper.c  |   52 +++++++++++-----------------------------------
 target-mips/translate.c  |    2 ++
 4 files changed, 45 insertions(+), 42 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/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..1b798a2 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -30,34 +30,6 @@ 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)
-{
-    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);
-}
-
 void helper_raise_exception_err(CPUMIPSState *env, uint32_t exception,
                                 int error_code)
 {
@@ -309,7 +281,7 @@ target_ulong helper_##name(CPUMIPSState *env, target_ulong arg, int mem_idx)  \
 {                                                                             \
     if (arg & almask) {                                                       \
         env->CP0_BadVAddr = arg;                                              \
-        helper_raise_exception(env, EXCP_AdEL);                               \
+        do_raise_exception(env, EXCP_AdEL, GETPC());                          \
     }                                                                         \
     env->lladdr = do_translate_address(env, arg, 0);                          \
     env->llval = do_##insn(env, arg, mem_idx);                                \
@@ -329,7 +301,7 @@ target_ulong helper_##name(CPUMIPSState *env, target_ulong arg1,              \
                                                                               \
     if (arg2 & almask) {                                                      \
         env->CP0_BadVAddr = arg2;                                             \
-        helper_raise_exception(env, EXCP_AdES);                               \
+        do_raise_exception(env, EXCP_AdES, GETPC());                          \
     }                                                                         \
     if (do_translate_address(env, arg2, 1) == env->lladdr) {                  \
         tmp = do_##ld_insn(env, arg2, mem_idx);                               \
@@ -1787,13 +1759,13 @@ target_ulong helper_yield(CPUMIPSState *env, target_ulong arg)
                 env->active_tc.CP0_TCStatus & (1 << CP0TCSt_DT)) {
                 env->CP0_VPEControl &= ~(0x7 << CP0VPECo_EXCPT);
                 env->CP0_VPEControl |= 4 << CP0VPECo_EXCPT;
-                helper_raise_exception(env, EXCP_THREAD);
+                do_raise_exception(env, EXCP_THREAD, GETPC());
             }
         }
     } else if (arg1 == 0) {
         if (0 /* TODO: TC underflow */) {
             env->CP0_VPEControl &= ~(0x7 << CP0VPECo_EXCPT);
-            helper_raise_exception(env, EXCP_THREAD);
+            do_raise_exception(env, EXCP_THREAD, GETPC());
         } else {
             // TODO: Deallocate TC
         }
@@ -1801,7 +1773,7 @@ target_ulong helper_yield(CPUMIPSState *env, target_ulong arg)
         /* Yield qualifier inputs not implemented. */
         env->CP0_VPEControl &= ~(0x7 << CP0VPECo_EXCPT);
         env->CP0_VPEControl |= 2 << CP0VPECo_EXCPT;
-        helper_raise_exception(env, EXCP_THREAD);
+        do_raise_exception(env, EXCP_THREAD, GETPC());
     }
     return env->CP0_YQMask;
 }
@@ -2131,7 +2103,7 @@ target_ulong helper_rdhwr_cpunum(CPUMIPSState *env)
         (env->CP0_HWREna & (1 << 0)))
         return env->CP0_EBase & 0x3ff;
     else
-        helper_raise_exception(env, EXCP_RI);
+        do_raise_exception(env, EXCP_RI, GETPC());
 
     return 0;
 }
@@ -2142,7 +2114,7 @@ target_ulong helper_rdhwr_synci_step(CPUMIPSState *env)
         (env->CP0_HWREna & (1 << 1)))
         return env->SYNCI_Step;
     else
-        helper_raise_exception(env, EXCP_RI);
+        do_raise_exception(env, EXCP_RI, GETPC());
 
     return 0;
 }
@@ -2153,7 +2125,7 @@ target_ulong helper_rdhwr_cc(CPUMIPSState *env)
         (env->CP0_HWREna & (1 << 2)))
         return env->CP0_Count;
     else
-        helper_raise_exception(env, EXCP_RI);
+        do_raise_exception(env, EXCP_RI, GETPC());
 
     return 0;
 }
@@ -2164,7 +2136,7 @@ target_ulong helper_rdhwr_ccres(CPUMIPSState *env)
         (env->CP0_HWREna & (1 << 3)))
         return env->CCRes;
     else
-        helper_raise_exception(env, EXCP_RI);
+        do_raise_exception(env, EXCP_RI, GETPC());
 
     return 0;
 }
@@ -2299,7 +2271,7 @@ target_ulong helper_cfc1(CPUMIPSState *env, uint32_t reg)
                 arg1 = (int32_t)
                        ((env->CP0_Status & (1  << CP0St_FR)) >> CP0St_FR);
             } else {
-                helper_raise_exception(env, EXCP_RI);
+                do_raise_exception(env, EXCP_RI, GETPC());
             }
         }
         break;
@@ -2332,7 +2304,7 @@ void helper_ctc1(CPUMIPSState *env, target_ulong arg1, uint32_t fs, uint32_t rt)
             env->CP0_Status &= ~(1 << CP0St_FR);
             compute_hflags(env);
         } else {
-            helper_raise_exception(env, EXCP_RI);
+            do_raise_exception(env, EXCP_RI, GETPC());
         }
         break;
     case 4:
@@ -2344,7 +2316,7 @@ void helper_ctc1(CPUMIPSState *env, target_ulong arg1, uint32_t fs, uint32_t rt)
             env->CP0_Status |= (1 << CP0St_FR);
             compute_hflags(env);
         } else {
-            helper_raise_exception(env, EXCP_RI);
+            do_raise_exception(env, EXCP_RI, GETPC());
         }
         break;
     case 25:
diff --git a/target-mips/translate.c b/target-mips/translate.c
index fd063a2..9c2ff7c 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -1675,6 +1675,7 @@ generate_exception_err (DisasContext *ctx, int excp, int err)
     TCGv_i32 terr = tcg_const_i32(err);
     save_cpu_state(ctx, 1);
     gen_helper_raise_exception_err(cpu_env, texcp, terr);
+    ctx->bstate = BS_STOP;
     tcg_temp_free_i32(terr);
     tcg_temp_free_i32(texcp);
 }
@@ -1684,6 +1685,7 @@ generate_exception (DisasContext *ctx, int excp)
 {
     save_cpu_state(ctx, 1);
     gen_helper_0e0i(raise_exception, excp);
+    ctx->bstate = BS_STOP;
 }
 
 /* Addresses computation */

Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Aurelien Jarno]

On 2015-06-10 11:33, Pavel Dovgalyuk wrote:
> This patch fixes exception handling in MIPS.
> 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 in MIPS 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.

I don't think it is correct. There is no real point of always doing
retranslation for an exception triggered from the helpers, especially
when the CPU state has been saved before anyway?

> This patch passes pointer to the translation block internals to the exception
> handler. It allows correct restoring of the icount value.

Your patch doesn't do that for all the helpers, for example all the
memory access helpers. It probably improves the situation but therefore
doesn't fix it.

From my point of view, it looks like the problem is actually elsewhere
in the common icount code. Do we know if it works correctly on other
emulated architectures? Also do you have a quick example to reproduce
the issue?


> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  target-mips/cpu.h        |   28 +++++++++++++++++++++++++
>  target-mips/msa_helper.c |    5 +++-
>  target-mips/op_helper.c  |   52 +++++++++++-----------------------------------
>  target-mips/translate.c  |    2 ++
>  4 files changed, 45 insertions(+), 42 deletions(-)

[ snip ]

> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index fd063a2..9c2ff7c 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -1675,6 +1675,7 @@ generate_exception_err (DisasContext *ctx, int excp, int err)
>      TCGv_i32 terr = tcg_const_i32(err);
>      save_cpu_state(ctx, 1);
>      gen_helper_raise_exception_err(cpu_env, texcp, terr);
> +    ctx->bstate = BS_STOP;
>      tcg_temp_free_i32(terr);
>      tcg_temp_free_i32(texcp);
>  }
> @@ -1684,6 +1685,7 @@ generate_exception (DisasContext *ctx, int excp)
>  {
>      save_cpu_state(ctx, 1);
>      gen_helper_0e0i(raise_exception, excp);
> +    ctx->bstate = BS_STOP;
>  }
>  

Why do we need to stop the translation here? The exception might be
conditional (for example for ADDU or SUBU).

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

Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Pavel Dovgaluk]

> From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> On 2015-06-10 11:33, Pavel Dovgalyuk wrote:
> > This patch fixes exception handling in MIPS.
> > 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 in MIPS 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.
> 
> I don't think it is correct. There is no real point of always doing
> retranslation for an exception triggered from the helpers, especially
> when the CPU state has been saved before anyway?

As you know, icount is processed as follows:

TB:
if icount < n then exit
icount -= n
instr1
instr2
...
instrn
exit

When one of the instructions initiates an exception, then icount should be restored
and adjusted number of instructions should be subtracted instead of initial n.

E.g., tlb_fill function passes retaddr to raise_exception, which allows restoring
current instructions in TB and correct icount calculation.

When exception triggered with other function (e.g. by embedding call to 
helper_raise_exception_err into TB), then PC is not passed as retaddr and
correct icount is not recovered.

This behavior leads to incorrect values of timers and non-deterministic execution 
of the code.

> > This patch passes pointer to the translation block internals to the exception
> > handler. It allows correct restoring of the icount value.
> 
> Your patch doesn't do that for all the helpers, for example all the
> memory access helpers. It probably improves the situation but therefore
> doesn't fix it.

Right, I missed these helpers. I'll add them in the next version.

> From my point of view, it looks like the problem is actually elsewhere
> in the common icount code. Do we know if it works correctly on other
> emulated architectures?

This problem can be solved only if someone passes PC to cpu_restore_state function.
It cannot be done without altering exceptions processing of the targets.

Other architectures have bugs in icount processing in case of the exceptions.
I will fix them and submit as separate patches.

> Also do you have a quick example to reproduce
> the issue?

I have no quick example, because I've got this situation in the middle 
of booting Linux process. It is hard to catch, because in most cases 
this bug does not affect the guest behavior.

> 
> 
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  target-mips/cpu.h        |   28 +++++++++++++++++++++++++
> >  target-mips/msa_helper.c |    5 +++-
> >  target-mips/op_helper.c  |   52 +++++++++++-----------------------------------
> >  target-mips/translate.c  |    2 ++
> >  4 files changed, 45 insertions(+), 42 deletions(-)
> 
> [ snip ]
> 
> > diff --git a/target-mips/translate.c b/target-mips/translate.c
> > index fd063a2..9c2ff7c 100644
> > --- a/target-mips/translate.c
> > +++ b/target-mips/translate.c
> > @@ -1675,6 +1675,7 @@ generate_exception_err (DisasContext *ctx, int excp, int err)
> >      TCGv_i32 terr = tcg_const_i32(err);
> >      save_cpu_state(ctx, 1);
> >      gen_helper_raise_exception_err(cpu_env, texcp, terr);
> > +    ctx->bstate = BS_STOP;
> >      tcg_temp_free_i32(terr);
> >      tcg_temp_free_i32(texcp);
> >  }
> > @@ -1684,6 +1685,7 @@ generate_exception (DisasContext *ctx, int excp)
> >  {
> >      save_cpu_state(ctx, 1);
> >      gen_helper_0e0i(raise_exception, excp);
> > +    ctx->bstate = BS_STOP;
> >  }
> >
> 
> Why do we need to stop the translation here? The exception might be
> conditional (for example for ADDU or SUBU).

Right, it would be better to add another helper, which recovers the PC.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Aurelien Jarno]

On 2015-06-15 07:53, Pavel Dovgaluk wrote:
> > From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> > On 2015-06-10 11:33, Pavel Dovgalyuk wrote:
> > > This patch fixes exception handling in MIPS.
> > > 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 in MIPS 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.
> > 
> > I don't think it is correct. There is no real point of always doing
> > retranslation for an exception triggered from the helpers, especially
> > when the CPU state has been saved before anyway?
> 
> As you know, icount is processed as follows:
> 
> TB:
> if icount < n then exit
> icount -= n
> instr1
> instr2
> ...
> instrn
> exit
> 
> When one of the instructions initiates an exception, then icount should be restored
> and adjusted number of instructions should be subtracted instead of initial n.
> 
> E.g., tlb_fill function passes retaddr to raise_exception, which allows restoring
> current instructions in TB and correct icount calculation.
> 
> When exception triggered with other function (e.g. by embedding call to 
> helper_raise_exception_err into TB), then PC is not passed as retaddr and
> correct icount is not recovered.
> 
> This behavior leads to incorrect values of timers and non-deterministic execution 
> of the code.

Ok, this therefore doesn't looks something MIPS specific, but rather a
flaw in the icount design. Instead of fixing blindly one target, we
should try to fix it globally, or if not possible at least agree on a
way to fix that for all target and provide the infrastructure for that
(for example provide load/store functions which accept a return
address). Paolo any opinion on that?

Also retranslation has a cost (actually on MIPS we spend more time in
*retranslation* than in translation due to the way the MMU works), we
should avoid it if we already have to save the CPU state for another
reason. At least your patch should remove the code saving the CPU state
when possible if the helper uses retranslation instead.

> > > This patch passes pointer to the translation block internals to the exception
> > > handler. It allows correct restoring of the icount value.
> > 
> > Your patch doesn't do that for all the helpers, for example all the
> > memory access helpers. It probably improves the situation but therefore
> > doesn't fix it.
> 
> Right, I missed these helpers. I'll add them in the next version.

Except we currently don't provide a way in the softmmu code to provide a
return address...

Aurelien

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

Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Pavel Dovgaluk]

> From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> On 2015-06-15 07:53, Pavel Dovgaluk wrote:
> > > From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> > > On 2015-06-10 11:33, Pavel Dovgalyuk wrote:
> > > > This patch fixes exception handling in MIPS.
> > > > 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 in MIPS 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.
> > >
> > > I don't think it is correct. There is no real point of always doing
> > > retranslation for an exception triggered from the helpers, especially
> > > when the CPU state has been saved before anyway?
> >
> > As you know, icount is processed as follows:
> >
> > TB:
> > if icount < n then exit
> > icount -= n
> > instr1
> > instr2
> > ...
> > instrn
> > exit
> >
> > When one of the instructions initiates an exception, then icount should be restored
> > and adjusted number of instructions should be subtracted instead of initial n.
> >
> > E.g., tlb_fill function passes retaddr to raise_exception, which allows restoring
> > current instructions in TB and correct icount calculation.
> >
> > When exception triggered with other function (e.g. by embedding call to
> > helper_raise_exception_err into TB), then PC is not passed as retaddr and
> > correct icount is not recovered.
> >
> > This behavior leads to incorrect values of timers and non-deterministic execution
> > of the code.
> 
> Ok, this therefore doesn't looks something MIPS specific, but rather a
> flaw in the icount design. Instead of fixing blindly one target, we
> should try to fix it globally, or if not possible at least agree on a
> way to fix that for all target and provide the infrastructure for that
> (for example provide load/store functions which accept a return
> address). Paolo any opinion on that?
> 
> Also retranslation has a cost (actually on MIPS we spend more time in
> *retranslation* than in translation due to the way the MMU works), we
> should avoid it if we already have to save the CPU state for another
> reason. At least your patch should remove the code saving the CPU state
> when possible if the helper uses retranslation instead.

Ok, I'll remove CPU saving where it is not actually used because of the exception.

> > > > This patch passes pointer to the translation block internals to the exception
> > > > handler. It allows correct restoring of the icount value.
> > >
> > > Your patch doesn't do that for all the helpers, for example all the
> > > memory access helpers. It probably improves the situation but therefore
> > > doesn't fix it.
> >
> > Right, I missed these helpers. I'll add them in the next version.
> 
> Except we currently don't provide a way in the softmmu code to provide a
> return address...

Softmmu passes return address into tlb_fill function, which passes it to raise_exception.
I also fixed HELPER_LD_ATOMIC and HELPER_ST_ATOMIC to recover PC value.
Do you mean something different?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Pavel Dovgaluk]

> From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> On 2015-06-15 07:53, Pavel Dovgaluk wrote:
> > > From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> > > On 2015-06-10 11:33, Pavel Dovgalyuk wrote:
> > > > This patch fixes exception handling in MIPS.
> > > > 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 in MIPS 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.
> > >
> > > I don't think it is correct. There is no real point of always doing
> > > retranslation for an exception triggered from the helpers, especially
> > > when the CPU state has been saved before anyway?
> >
> > As you know, icount is processed as follows:
> >
> > TB:
> > if icount < n then exit
> > icount -= n
> > instr1
> > instr2
> > ...
> > instrn
> > exit
> >
> > When one of the instructions initiates an exception, then icount should be restored
> > and adjusted number of instructions should be subtracted instead of initial n.
> >
> > E.g., tlb_fill function passes retaddr to raise_exception, which allows restoring
> > current instructions in TB and correct icount calculation.
> >
> > When exception triggered with other function (e.g. by embedding call to
> > helper_raise_exception_err into TB), then PC is not passed as retaddr and
> > correct icount is not recovered.
> >
> > This behavior leads to incorrect values of timers and non-deterministic execution
> > of the code.
> 
> Ok, this therefore doesn't looks something MIPS specific, but rather a
> flaw in the icount design. Instead of fixing blindly one target, we
> should try to fix it globally, or if not possible at least agree on a
> way to fix that for all target and provide the infrastructure for that
> (for example provide load/store functions which accept a return
> address). Paolo any opinion on that?

Recovering from is a tricky mechanism. It can break the correct execution
if used inaccurately even when icount is disabled.
I already posted a patch for maskmov instruction in i386:
http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg02960.html

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Aurelien Jarno]

On 2015-06-15 10:39, Pavel Dovgaluk wrote:
> > From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> > On 2015-06-15 07:53, Pavel Dovgaluk wrote:
> > > > From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> > > > On 2015-06-10 11:33, Pavel Dovgalyuk wrote:
> > > > > This patch fixes exception handling in MIPS.
> > > > > 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 in MIPS 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.
> > > >
> > > > I don't think it is correct. There is no real point of always doing
> > > > retranslation for an exception triggered from the helpers, especially
> > > > when the CPU state has been saved before anyway?
> > >
> > > As you know, icount is processed as follows:
> > >
> > > TB:
> > > if icount < n then exit
> > > icount -= n
> > > instr1
> > > instr2
> > > ...
> > > instrn
> > > exit
> > >
> > > When one of the instructions initiates an exception, then icount should be restored
> > > and adjusted number of instructions should be subtracted instead of initial n.
> > >
> > > E.g., tlb_fill function passes retaddr to raise_exception, which allows restoring
> > > current instructions in TB and correct icount calculation.
> > >
> > > When exception triggered with other function (e.g. by embedding call to
> > > helper_raise_exception_err into TB), then PC is not passed as retaddr and
> > > correct icount is not recovered.
> > >
> > > This behavior leads to incorrect values of timers and non-deterministic execution
> > > of the code.
> > 
> > Ok, this therefore doesn't looks something MIPS specific, but rather a
> > flaw in the icount design. Instead of fixing blindly one target, we
> > should try to fix it globally, or if not possible at least agree on a
> > way to fix that for all target and provide the infrastructure for that
> > (for example provide load/store functions which accept a return
> > address). Paolo any opinion on that?
> > 
> > Also retranslation has a cost (actually on MIPS we spend more time in
> > *retranslation* than in translation due to the way the MMU works), we
> > should avoid it if we already have to save the CPU state for another
> > reason. At least your patch should remove the code saving the CPU state
> > when possible if the helper uses retranslation instead.
> 
> Ok, I'll remove CPU saving where it is not actually used because of the exception.
> 
> > > > > This patch passes pointer to the translation block internals to the exception
> > > > > handler. It allows correct restoring of the icount value.
> > > >
> > > > Your patch doesn't do that for all the helpers, for example all the
> > > > memory access helpers. It probably improves the situation but therefore
> > > > doesn't fix it.
> > >
> > > Right, I missed these helpers. I'll add them in the next version.
> > 
> > Except we currently don't provide a way in the softmmu code to provide a
> > return address...
> 
> Softmmu passes return address into tlb_fill function, which passes it to raise_exception.
> I also fixed HELPER_LD_ATOMIC and HELPER_ST_ATOMIC to recover PC value.
> Do you mean something different?

Yes, for example in your patch:

> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 73a8e45..1b798a2 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -309,7 +281,7 @@ target_ulong helper_##name(CPUMIPSState *env, target_ulong arg, int mem_idx)  \
>  {                                                                             \
>      if (arg & almask) {                                                       \
>          env->CP0_BadVAddr = arg;                                              \
> -        helper_raise_exception(env, EXCP_AdEL);                               \
> +        do_raise_exception(env, EXCP_AdEL, GETPC());                          \
>      }                                                                         \
>      env->lladdr = do_translate_address(env, arg, 0);                          \
>      env->llval = do_##insn(env, arg, mem_idx);                                \

do_##insn is actually a load/store instruction, but you doesn't pass the
return address as an argument (and you can't with the current code).
 
-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Aurelien Jarno]

On 2015-06-15 10:48, Pavel Dovgaluk wrote:
> > From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> > On 2015-06-15 07:53, Pavel Dovgaluk wrote:
> > > > From: Aurelien Jarno [mailto:aurelien@aurel32.net]
> > > > On 2015-06-10 11:33, Pavel Dovgalyuk wrote:
> > > > > This patch fixes exception handling in MIPS.
> > > > > 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 in MIPS 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.
> > > >
> > > > I don't think it is correct. There is no real point of always doing
> > > > retranslation for an exception triggered from the helpers, especially
> > > > when the CPU state has been saved before anyway?
> > >
> > > As you know, icount is processed as follows:
> > >
> > > TB:
> > > if icount < n then exit
> > > icount -= n
> > > instr1
> > > instr2
> > > ...
> > > instrn
> > > exit
> > >
> > > When one of the instructions initiates an exception, then icount should be restored
> > > and adjusted number of instructions should be subtracted instead of initial n.
> > >
> > > E.g., tlb_fill function passes retaddr to raise_exception, which allows restoring
> > > current instructions in TB and correct icount calculation.
> > >
> > > When exception triggered with other function (e.g. by embedding call to
> > > helper_raise_exception_err into TB), then PC is not passed as retaddr and
> > > correct icount is not recovered.
> > >
> > > This behavior leads to incorrect values of timers and non-deterministic execution
> > > of the code.
> > 
> > Ok, this therefore doesn't looks something MIPS specific, but rather a
> > flaw in the icount design. Instead of fixing blindly one target, we
> > should try to fix it globally, or if not possible at least agree on a
> > way to fix that for all target and provide the infrastructure for that
> > (for example provide load/store functions which accept a return
> > address). Paolo any opinion on that?
> 
> Recovering from is a tricky mechanism. It can break the correct execution
> if used inaccurately even when icount is disabled.
> I already posted a patch for maskmov instruction in i386:
> http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg02960.html

One solution there is to save the CPU state before calling the maskmov
helper. That would fix the bug, but not the icount problem you reported.

If we decided that we must always be able to do retranslation, we should
go with the second possibility from Richard:

| (2) Add helpers that accept the GETRA value from the top-level helper.  And not
| hidden within a macro or always_inline function.  This helps us see what
| portions of the code have been audited for the new interface.  This will
| involve quite a bit more code churn, but shouldn't been too difficult for any
| single function.

And we should do it a way that it works for both user and softmmu modes,
to avoid too many #ifdef in the targets code, which in general is a
source of bugs.

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