MIPS: exceptions handling in icount mode

PATCH In review Pavel Dovgalyuk 2 participants 8 messages Jun 10, 2015 – Jun 15, 2015

Share & tools

Pavel Dovgalyuk

author

2015-06-10 08:33 UTC #0 lore raw

PATCH 1/1 MIPS: exceptions handling in icount mode

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 <[email protected]>

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

target-mips/cpu.h +28 -0

index f9d2b4c..70ba39a 100644
@@ -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__) */

target-mips/msa_helper.c +3 -2

index 26ffdc7..f7bc710 100644
@@ -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());
     }
 }

target-mips/op_helper.c +12 ~~-40~~

129 lines not on this page · raw message

target-mips/translate.c +2 -0

index fd063a2..9c2ff7c 100644
@@ -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 */

Aurelien Jarno

2015-06-11 22:37 UTC #1 lore raw

Quoted 6 lines · 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.

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?

Quoted 2 lines

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?

Quoted 7 lines

Signed-off-by Pavel Dovgalyuk <[email protected]>

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

Quoted 13 lines

target-mips/translate.c +1 -0

index fd063a2..9c2ff7c 100644
@@ -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);
 }

target-mips/translate.c generate_exception (DisasContext *ctx, int excp) · line 1690

Quoted 6 lines

target-mips/translate.c +1 -0

@@ -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).

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net

Pavel Dovgaluk

author

2015-06-15 04:53 UTC #2 lore raw

Quoted 9 lines

From: Aurelien Jarno [mailto:[email protected]]
Quoted 6 lines · 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.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 26 lines

Quoted 9 lines

Signed-off-by Pavel Dovgalyuk <[email protected]>

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

Quoted 14 lines

target-mips/translate.c +1 -0

index fd063a2..9c2ff7c 100644
@@ -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)
 {

Quoted 8 lines

target-mips/translate.c generate_exception (DisasContext *ctx, int excp) · line 1690
Quoted 5 lines
target-mips/translate.c +1 -0
     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

Aurelien Jarno

2015-06-15 07:26 UTC #3 lore raw

Quoted 30 lines · Pavel Dovgaluk

Quoted 9 lines
From: Aurelien Jarno [mailto:[email protected]]
Quoted 6 lines · 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.
Quoted 3 lines
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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net

Pavel Dovgaluk

author

2015-06-15 07:39 UTC #4 lore raw

Quoted 46 lines

From: Aurelien Jarno [mailto:[email protected]]
Quoted 30 lines · Pavel Dovgaluk
Quoted 9 lines
From: Aurelien Jarno [mailto:[email protected]]
Quoted 6 lines · 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.
Quoted 3 lines
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.
Quoted 6 lines
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?

Quoted 5 lines

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.

Quoted 11 lines

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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

Aurelien Jarno

2015-06-15 08:23 UTC #5 lore raw

Quoted 61 lines · Pavel Dovgaluk

Quoted 46 lines
From: Aurelien Jarno [mailto:[email protected]]
Quoted 30 lines · Pavel Dovgaluk
Quoted 9 lines
From: Aurelien Jarno [mailto:[email protected]]
Quoted 6 lines · 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.
Quoted 3 lines
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.
Quoted 6 lines
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?
Quoted 5 lines
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.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
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:

target-mips/op_helper.c target_ulong helper_##name(CPUMIPSState *env, target_ulong arg, int mem_idx) \ · line 287

Quoted 13 lines

target-mips/op_helper.c +1 -1

index 73a8e45..1b798a2 100644
@@ -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).

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net

Pavel Dovgaluk

author

2015-06-15 07:48 UTC #6 lore raw

In reply to Aurelien Jarno

Quoted 39 lines

From: Aurelien Jarno [mailto:[email protected]]
Quoted 30 lines · Pavel Dovgaluk
Quoted 9 lines
From: Aurelien Jarno [mailto:[email protected]]
Quoted 6 lines · 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.
Quoted 3 lines
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.
Quoted 6 lines
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.

Quoted 6 lines

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

Aurelien Jarno

2015-06-15 08:28 UTC #7 lore raw

Quoted 46 lines · Pavel Dovgaluk

Quoted 39 lines
From: Aurelien Jarno [mailto:[email protected]]
Quoted 30 lines · Pavel Dovgaluk
Quoted 9 lines
From: Aurelien Jarno [mailto:[email protected]]
Quoted 6 lines · 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.
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 6 lines
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?

Quoted 4 lines

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.

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net