Fix exceptions handling for MIPS and i386

This patch introduces several helpers to pass return address which points to the TB. Correct return address allows correct restoring of the guest PC and icount. These functions should be used when helpers embedded into TB invoke memory operations.

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

---
 include/exec/cpu_ldst_template.h |   42 +++++++++++++++++++++++++++++++-------
 include/exec/exec-all.h          |   27 ++++++++++++++++++++++++
 softmmu_template.h               |   18 ++++++++++++++++
 3 files changed, 79 insertions(+), 8 deletions(-)

include/exec/cpu_ldst_template.h +0 -0

index 95ab750..1847816 100644

From: Paolo Bonzini [mailto:[email protected]]

Quoted 19 lines · Pavel Dovgalyuk

This patch introduces several helpers to pass return address which points to the TB. Correct return address allows correct restoring of the guest PC and icount. These functions should be used when helpers embedded into TB invoke memory operations.

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

---
 include/exec/cpu_ldst_template.h |   42 +++++++++++++++++++++++++++++++-------
 include/exec/exec-all.h          |   27 ++++++++++++++++++++++++
 softmmu_template.h               |   18 ++++++++++++++++
 3 files changed, 79 insertions(+), 8 deletions(-)

include/exec/cpu_ldst_template.h +0 -0

index 95ab750..1847816 100644
@@ -62,7 +62,9 @@
 /* generic load/store macros */
 

include/exec/cpu_ldst_template.h line 67

Quoted 5 lines

include/exec/cpu_ldst_template.h +3 -1

 static inline RES_TYPE
-glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
+                                                  target_ulong ptr,
+                                                  uintptr_t retaddr)

Would it make sense to call these helper_cpu_ld##USUFFIX##MEMSUFFIX?

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

include/exec/cpu_ldst_template.h line 67

Quoted 6 lines

static inline RES_TYPE

include/exec/cpu_ldst_template.h +3 -1

-glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
+                                                  target_ulong ptr,
+                                                  uintptr_t retaddr)

Would it make sense to call these helper_cpu_ld##USUFFIX##MEMSUFFIX?

I don't want to use 'helper' prefix, because helper functions are usually called directly from TB.

Quoted 8 lines · Pavel Dovgaluk

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

include/exec/cpu_ldst_template.h line 67

Quoted 6 lines

static inline RES_TYPE

include/exec/cpu_ldst_template.h +3 -1

-glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
+                                                  target_ulong ptr,
+                                                  uintptr_t retaddr)

Would it make sense to call these helper_cpu_ld##USUFFIX##MEMSUFFIX?

Quoted 2 lines

I don't want to use 'helper' prefix, because helper functions are usually called directly from TB.

True, but in the end these have the same functionality as helpers, just they're indirectly called from other helpers.

From: Paolo Bonzini [mailto:[email protected]]

Quoted 17 lines · Pavel Dovgalyuk

This patch introduces several helpers to pass return address which points to the TB. Correct return address allows correct restoring of the guest PC and icount. These functions should be used when helpers embedded into TB invoke memory operations.

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

---
 include/exec/cpu_ldst_template.h |   42 +++++++++++++++++++++++++++++++-------
 include/exec/exec-all.h          |   27 ++++++++++++++++++++++++
 softmmu_template.h               |   18 ++++++++++++++++
 3 files changed, 79 insertions(+), 8 deletions(-)

include/exec/cpu_ldst_template.h +0 -0

index 95ab750..1847816 100644

include/exec/cpu_ldst_template.h line 67

Quoted 7 lines

include/exec/cpu_ldst_template.h +3 -1

@@ -62,7 +62,9 @@
 /* generic load/store macros */
 
 static inline RES_TYPE
-glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
+                                                  target_ulong ptr,
+                                                  uintptr_t retaddr)

Would it make sense to call these helper_cpu_ld##USUFFIX##MEMSUFFIX?

include/exec/exec-all.h struct MemoryRegion *iotlb_to_region(CPUState *cpu, · line 354

Quoted 10 lines

include/exec/exec-all.h +2 -0

index 856e698..b3aefde 100644
@@ -350,6 +350,33 @@ struct MemoryRegion *iotlb_to_region(CPUState *cpu,
 void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int mmu_idx,
               uintptr_t retaddr);
 
+uint8_t helper_call_ldb_cmmu(CPUArchState *env, target_ulong addr,
+                             int mmu_idx, uintptr_t retaddr);

Here we already have helper_ret_ldb_cmmu, so the new function is only needed if DATA_SIZE != 1.

include/exec/exec-all.h struct MemoryRegion *iotlb_to_region(CPUState *cpu, · line 356

Quoted 2 lines

include/exec/exec-all.h +2 -0

+uint16_t helper_call_ldw_cmmu(CPUArchState *env, target_ulong addr,
+                              int mmu_idx, uintptr_t retaddr);

What about helper_ret_ldw_cmmu for consistency with the DATA_SIZE == 1 case?

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+uint16_t helper_call_ldw_cmmu(CPUArchState *env, target_ulong addr,
+                              int mmu_idx, uintptr_t retaddr);

What about helper_ret_ldw_cmmu for consistency with the DATA_SIZE == 1 case?

tcg.h breaks these definitions:

/* Temporary aliases until backends are converted. */ #ifdef TARGET_WORDS_BIGENDIAN

# define helper_ret_ldsw_mmu  helper_be_ldsw_mmu
# define helper_ret_lduw_mmu  helper_be_lduw_mmu
# define helper_ret_ldsl_mmu  helper_be_ldsl_mmu
# define helper_ret_ldul_mmu  helper_be_ldul_mmu
# define helper_ret_ldq_mmu   helper_be_ldq_mmu
# define helper_ret_stw_mmu   helper_be_stw_mmu
# define helper_ret_stl_mmu   helper_be_stl_mmu
# define helper_ret_stq_mmu   helper_be_stq_mmu
#else

From: Paolo Bonzini [mailto:[email protected]]

Quoted 8 lines · Pavel Dovgaluk

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+uint16_t helper_call_ldw_cmmu(CPUArchState *env, target_ulong addr,
+                              int mmu_idx, uintptr_t retaddr);

What about helper_ret_ldw_cmmu for consistency with the DATA_SIZE == 1 case?

tcg.h breaks these definitions:

/* Temporary aliases until backends are converted. */ #ifdef TARGET_WORDS_BIGENDIAN

Quoted 9 lines

# define helper_ret_ldsw_mmu  helper_be_ldsw_mmu
# define helper_ret_lduw_mmu  helper_be_lduw_mmu
# define helper_ret_ldsl_mmu  helper_be_ldsl_mmu
# define helper_ret_ldul_mmu  helper_be_ldul_mmu
# define helper_ret_ldq_mmu   helper_be_ldq_mmu
# define helper_ret_stw_mmu   helper_be_stw_mmu
# define helper_ret_stl_mmu   helper_be_stl_mmu
# define helper_ret_stq_mmu   helper_be_stq_mmu
#else

Isn't this exactly the same as your helper_call_ldw_cmmu?

Quoted 12 lines

Quoted 11 lines

/* Temporary aliases until backends are converted. */ #ifdef TARGET_WORDS_BIGENDIAN

# define helper_ret_ldsw_mmu  helper_be_ldsw_mmu
# define helper_ret_lduw_mmu  helper_be_lduw_mmu
# define helper_ret_ldsl_mmu  helper_be_ldsl_mmu
# define helper_ret_ldul_mmu  helper_be_ldul_mmu
# define helper_ret_ldq_mmu   helper_be_ldq_mmu
# define helper_ret_stw_mmu   helper_be_stw_mmu
# define helper_ret_stl_mmu   helper_be_stl_mmu
# define helper_ret_stq_mmu   helper_be_stq_mmu
#else

Quoted 1 line

Isn't this exactly the same as your helper_call_ldw_cmmu?

Yes, but I can't compile it yet.

This patch fixes 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.

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

---
 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  |   46 ++++++------
 5 files changed, 141 insertions(+), 122 deletions(-)

target-mips/op_helper.c +3 -18

index 73a8e45..2815c60 100644
@@ -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, GETPC());

This patch passes TB return address into softmmu functions that are invoked from target helpers. This allows correct PC and icount recovering while handling MMU faults.

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

---
 target-i386/cc_helper.c   |    2 
 target-i386/cpu.h         |    5 
 target-i386/excp_helper.c |   23 +
 target-i386/fpu_helper.c  |  146 +++++----
 target-i386/helper.c      |    4 
 target-i386/int_helper.c  |   32 +-
 target-i386/mem_helper.c  |   39 +-
 target-i386/misc_helper.c |   12 -
 target-i386/ops_sse.h     |    2 
 target-i386/seg_helper.c  |  712 +++++++++++++++++++++++----------------------
 target-i386/svm_helper.c  |    4 
 target-i386/translate.c   |   25 --
 12 files changed, 506 insertions(+), 500 deletions(-)

target-i386/excp_helper.c +0 -0

index 99fca84..48be348 100644

This set of patches fixes exception handling for MIPS and i386 targets. These targets contain instructions that break correct execution in icount/TCG modes (MIPS) and in regular TCG mode (i386).

Incorrect execution for i386 is causes by exceptions raised by MMU functions. MMU helper functions are called from generated code and other helper functions. In both cases they try to get function's return address for restoring virtual CPU state.

When MMU helper is called from some other helper function (like helper_maskmov_xmm) through cpu_st* function, the return address will point to that helper. That is why CPU state cannot be restored in the case of MMU fault.

This bug can occur when maskmov instruction is located in the middle of the translation block.

Execution sequence for this example:

TB start:
PC1: instr1
instr2
PC2: maskmov <page fault>
<page fault processing>
PC1: instr1
instr2
maskmov

At the start of TB execution guest PC points to instr1. When page fault occurs QEMU tries to restore guest PC (which should be equal to PC2). It reads host PC from the call stack and checks whether it points to TB or not. Bug in ldst helpers implementation provides incorrect host PC, which is not located within the TB. That's why QEMU cannot recover guest PC and it remains the same (PC1). After page fault processing QEMU restarts TB and executes instr1 and instr2 for the second time, because guest PC was not recovered.

Bugs in MIPS helper functions do not break the execution in regular TCG mode, because PC value is updated before calling the functions that can raise an exception. But icount value cannot be updated this way. Therefore exceptions make execution in icount mode non-determinisic.
In icount mode every translation block looks as follows:

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

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

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 exception raising helper into TB), then PC is not passed as retaddr and correct icount is not recovered. In such cases icount will be decreased by the value equal to the size of TB.

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

These patches passes pointer to the translation block code to the exception handler. It allows correct restoring of PC and icount values.

From: Aurelien Jarno [mailto:[email protected]]

Quoted 3 lines · Pavel Dovgalyuk

This set of patches fixes exception handling for MIPS and i386 targets. These targets contain instructions that break correct execution in icount/TCG modes (MIPS) and in regular TCG mode (i386).

Just to be clear, this is not something specific to MIPS and i386. Every target which call cpu_loop_exit without doing a retranslation is affected by the icount bug.

Quoted 24 lines

Incorrect execution for i386 is causes by exceptions raised by MMU functions. MMU helper functions are called from generated code and other helper functions. In both cases they try to get function's return address for restoring virtual CPU state.

When MMU helper is called from some other helper function (like helper_maskmov_xmm) through cpu_st* function, the return address will point to that helper. That is why CPU state cannot be restored in the case of MMU fault.

This bug can occur when maskmov instruction is located in the middle of the translation block.

Execution sequence for this example:

TB start:
PC1: instr1
instr2
PC2: maskmov <page fault>
<page fault processing>
PC1: instr1
instr2
maskmov

Quoted 7 lines

At the start of TB execution guest PC points to instr1. When page fault occurs QEMU tries to restore guest PC (which should be equal to PC2). It reads host PC from the call stack and checks whether it points to TB or not. Bug in ldst helpers implementation provides incorrect host PC, which is not located within the TB. That's why QEMU cannot recover guest PC and it remains the same (PC1). After page fault processing QEMU restarts TB and executes instr1 and instr2 for the second time, because guest PC was not recovered.

Also just to be clear, the way you propose is just one way to fix the issue. The other way (which is used for all similar instructions) is to call just before the helper:

    gen_update_cc_op(s);
    gen_jmp_im(pc_start - s->cs_base);

Quoted 26 lines

Bugs in MIPS helper functions do not break the execution in regular TCG mode, because PC value is updated before calling the functions that can raise an exception. But icount value cannot be updated this way. Therefore exceptions make execution in icount mode non-determinisic.
In icount mode every translation block looks as follows:

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

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

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 exception raising helper into TB), then PC is not passed as retaddr and correct icount is not recovered. In such cases icount will be decreased by the value equal to the size of TB.

Quoted 5 lines

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

These patches passes pointer to the translation block code to the exception handler. It allows correct restoring of PC and icount values.

While I think it's the correct way for load/stores or FPU exceptions, I am not convinced we should do that in all cases. Retranslation has a cost, and when the exception is likely to occur, it's better to save the CPU state instead of going for a retranslation. Actually we had to rollback such a change on SH4, as it has some performances issues. See commit 1012740098d0307ce5d957ebbe9a7f020da7f574.

One way to fix that would be to reduce the cost of retranslation, for example by using a kind of exception table that we generate at the same time of the TB.

In icount mode every translation block looks as follows:

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

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

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 exception raising helper into TB), then PC is not passed as retaddr and correct icount is not recovered. In such cases icount will be decreased by the value equal to the size of TB.

From: Aurelien Jarno [mailto:[email protected]]

Quoted 14 lines · Pavel Dovgalyuk

In icount mode every translation block looks as follows:

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

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

Quoted 7 lines

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 exception raising helper into TB), then PC is not passed as retaddr and correct icount is not recovered. In such cases icount will be decreased by the value equal to the size of TB.

Looking at how icount work, I see it's basically a variable in the CPU state (icount_decr.u16.low), which is already accessed from the TB. Couldn't we adjust it using additional code before generating an exception, when in icount mode.

For example for MIPS, we can add some code before generate_exception which use the value from s->gen_opc_icount[j] to adjust the variable icount_decr.u16.low.

Quoted 24 lines

From: Aurelien Jarno [mailto:[email protected]]

Quoted 14 lines · Pavel Dovgalyuk

In icount mode every translation block looks as follows:

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

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

Quoted 7 lines

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 exception raising helper into TB), then PC is not passed as retaddr and correct icount is not recovered. In such cases icount will be decreased by the value equal to the size of TB.

Quoted 8 lines

Looking at how icount work, I see it's basically a variable in the CPU state (icount_decr.u16.low), which is already accessed from the TB. Couldn't we adjust it using additional code before generating an exception, when in icount mode.

For example for MIPS, we can add some code before generate_exception which use the value from s->gen_opc_icount[j] to adjust the variable icount_decr.u16.low.

It is possible, but it will incur additional overhead, because we will have to update icount every time the exception might be generated. We'll have to update icount value before and after every helper call, that can cause an exception:

icount -= n
...
instr_k
icount += n - k
helper
icount -= n - k
...

And this overhead will slowdown the code even if no exception occur.

From: Aurelien Jarno [mailto:[email protected]]

Quoted 46 lines · Pavel Dovgaluk

Quoted 24 lines

From: Aurelien Jarno [mailto:[email protected]]

Quoted 14 lines · Pavel Dovgalyuk

In icount mode every translation block looks as follows:

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

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

Quoted 7 lines

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 exception raising helper into TB), then PC is not passed as retaddr and correct icount is not recovered. In such cases icount will be decreased by the value equal to the size of TB.

Quoted 8 lines

Looking at how icount work, I see it's basically a variable in the CPU state (icount_decr.u16.low), which is already accessed from the TB. Couldn't we adjust it using additional code before generating an exception, when in icount mode.

For example for MIPS, we can add some code before generate_exception which use the value from s->gen_opc_icount[j] to adjust the variable icount_decr.u16.low.

It is possible, but it will incur additional overhead, because we will have to update icount every time the exception might be generated. We'll have to update icount value before and after every helper call, that can cause an exception:

icount -= n
...
instr_k
icount += n - k
helper
icount -= n - k
...

Quoted 1 line

And this overhead will slowdown the code even if no exception occur.

That's where I might disagree. Retranslation seems a very good idea on the paper, but in practice it doesn't seems to always bring the performance improvement it should. In addition it seems to be highly dependent on the target. Just to give some numbers, on MIPS (as your patch originally concerns this architecture), 40% of code generation is actually due to retranslation. The problem is that over the time we have improved a lot the code generation (liveness analysis, better register allocation, constant propagation, ...) and thus we have increased the code generation time. While it clearly has some benefits when this code is actually executed, it's not the case when the code is simply retranslated. In short we spend more time to find the CPU state corresponding to an exception than before.

All of that to say that I am worried for the performances to see more paths through the retranslation code, especially on MIPS as it seems to be costly. That said I haven't really look in details at other targets, nor hosts.

Now to come back about your patches, we might want to simply fix icount first, even if it has some performance impact, and deal with the retranslation issue separately, as it concerns more than just icount.

On x86, this patch brings a 5% boot time improvement on MIPS. One of the reason is that the TCG code generator has a good knowledge about which TCG ops or helpers can trigger an exception, so it can optimize out part of the instructions saving the CPU state. I guess that the host CPUs have also evolved over the time, now being superscalar and out-of-order so that saving the CPU state can be done "in background". Also it's just a quick and dirty patch, we can probably even do better.

All of that to say that I am worried for the performances to see more paths through the retranslation code, especially on MIPS as it seems to be costly. That said I haven't really look in details at other targets, nor hosts.

For an i386 guest still on an x86 host, I get a 4% slower boot time by not using retranslation (see patch below). This is not that much compared to the complexity retranslation bring us.

target-i386/translate.c +3 -0

index 58b1959..de65bba 100644
@@ -8001,6 +8001,9 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,
 
     gen_tb_start(tb);
     for(;;) {
+        gen_update_cc_op(dc);
+        gen_jmp_im(pc_ptr - dc->cs_base);
+
         if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
             QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
                 if (bp->pc == pc_ptr &&

translate-all.c +2 -0

index b6b0e1c..3d4c017 100644
@@ -212,6 +212,8 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
     int64_t ti;
 #endif
 
+    return -1;
+
 #ifdef CONFIG_PROFILER
     ti = profile_getclock();
 #endif

Quoted 3 lines · Aurelien Jarno

For an i386 guest still on an x86 host, I get a 4% slower boot time by not using retranslation (see patch below). This is not that much compared to the complexity retranslation bring us.

QEMU could just always compute and store the restore_state information.
TCG needs to help filling it in (a new TCG opcode?), but it should be easy.

Quoted 2 lines

Quoted 2 lines

QEMU could just always compute and store the restore_state information.
TCG needs to help filling it in (a new TCG opcode?), but it should be easy.

Yes, that was another approach I have in mind (I called it exception table in my other mail),

but it requires a tiny more work than just
saving the CPU state all the time. The problem is that the state information we want to save are varying for target to target. Going through a TCG opcode means we can use the liveness analysis pass to save the minimum amount of data.

That said I would like to push further the idea of always saving the CPU state a bit more to see if we can keep the same performances. There are still improvements to do, by removing more code on the core side (like finding the call to tb_finc_pc which is now useless), or on the target side by checking/improving helper flags. We might save the CPU state too often if a helper doesn't declare it doesn't touch globals.

Quoted 4 lines · Aurelien Jarno

Quoted 2 lines

Quoted 2 lines

QEMU could just always compute and store the restore_state information.
TCG needs to help filling it in (a new TCG opcode?), but it should be easy.

Yes, that was another approach I have in mind (I called it exception table in my other mail),

Okay, understood. My idea was more like always generating the gen_op_* arrays.

Quoted 5 lines

but it requires a tiny more work than just
saving the CPU state all the time. The problem is that the state information we want to save are varying for target to target. Going through a TCG opcode means we can use the liveness analysis pass to save the minimum amount of data.

I mentioned a TCG opcode because the target PC is not available inside the translator. So the translator could pepper the TCG instruction

stream with things like

checkpoint $target_pc, $target_cc_op, $0

TCG can then use them to fill in an array stored inside the TranslationBlock, together with the host PC. Since the gen_opc_pc, gen_opc_instr_start, gen_opc_icount arrays are inside tcg_ctx, it may be a good idea to store the checkpoint information compressed in a byte array (e.g. as a series of ULEB128 values---the host and target PCs can even be stored as deltas from the last value).

As a first step, gen_intermediate_code_pc and tcg_gen_code_search_pc can then be merged into a single target-independent function that uncompresses the byte array up to the required host PC into tcg_ctx. Later you can optimize them to remove the tcg_ctx arrays altogether.

So the patches could be something like this:

1) SPARC: put the jump target information directly in gen_opc_* without using gen_opc_jump_pc (not trivial)

2) a few targets: instead of gen_opc_* arrays, use a new generic member of tcg_ctx (similar to how csbase is used generically), e.g. tcg_ctx.gen_opc_target1[] and tcg_ctx.gen_opc_target2[].

3) all targets: always fill in tcg_ctx.gen_*, even if search_pc is false

4) TCG: add support for a checkpoint operation, make it fill in tcg_ctx.gen_*

5) all targets: change explicit filling of tcg_ctx.gen_* to use the checkpoint operation

6) TCG/translate-all: convert gen_intermediate_code_pc as outlined above

From: Aurelien Jarno [mailto:[email protected]]

Quoted 7 lines · Paolo Bonzini

TCG can then use them to fill in an array stored inside the TranslationBlock, together with the host PC. Since the gen_opc_pc, gen_opc_instr_start, gen_opc_icount arrays are inside tcg_ctx, it may be a good idea to store the checkpoint information compressed in a byte array (e.g. as a series of ULEB128 values---the host and target PCs can even be stored as deltas from the last value).

Either as deltas to the last value or as delta from the start of the TB. What I am worried about is the size of the checkpoint information, even if we do some compression, we might have one per guest instruction. I have implemented a naive version of that without compression, storing the checkpoint data at the end of the generated code, and it's about 30% of the size of the TB for MIPS. It's probably smaller on architectures storing only the PC. Also it's size is quite variable. That's why it's probably not a good idea to store it directly in the TranslationBlock. I don't like storing it directly in the generated code either, especially given this part is supposed to be executable.

Quoted 19 lines

As a first step, gen_intermediate_code_pc and tcg_gen_code_search_pc can then be merged into a single target-independent function that uncompresses the byte array up to the required host PC into tcg_ctx. Later you can optimize them to remove the tcg_ctx arrays altogether.

So the patches could be something like this:

1) SPARC: put the jump target information directly in gen_opc_* without using gen_opc_jump_pc (not trivial)

2) a few targets: instead of gen_opc_* arrays, use a new generic member of tcg_ctx (similar to how csbase is used generically), e.g. tcg_ctx.gen_opc_target1[] and tcg_ctx.gen_opc_target2[].

3) all targets: always fill in tcg_ctx.gen_*, even if search_pc is false

4) TCG: add support for a checkpoint operation, make it fill in tcg_ctx.gen_*

Quoted 4 lines

5) all targets: change explicit filling of tcg_ctx.gen_* to use the checkpoint operation

6) TCG/translate-all: convert gen_intermediate_code_pc as outlined above

That's sounds like a plan when I have more time ;-)

Quoted 40 lines

From: Aurelien Jarno [mailto:[email protected]]

Quoted 7 lines · Paolo Bonzini

TCG can then use them to fill in an array stored inside the TranslationBlock, together with the host PC. Since the gen_opc_pc, gen_opc_instr_start, gen_opc_icount arrays are inside tcg_ctx, it may be a good idea to store the checkpoint information compressed in a byte array (e.g. as a series of ULEB128 values---the host and target PCs can even be stored as deltas from the last value).

Either as deltas to the last value or as delta from the start of the TB. What I am worried about is the size of the checkpoint information, even if we do some compression, we might have one per guest instruction. I have implemented a naive version of that without compression, storing the checkpoint data at the end of the generated code, and it's about 30% of the size of the TB for MIPS. It's probably smaller on architectures storing only the PC. Also it's size is quite variable. That's why it's probably not a good idea to store it directly in the TranslationBlock. I don't like storing it directly in the generated code either, especially given this part is supposed to be executable.

Quoted 19 lines

As a first step, gen_intermediate_code_pc and tcg_gen_code_search_pc can then be merged into a single target-independent function that uncompresses the byte array up to the required host PC into tcg_ctx. Later you can optimize them to remove the tcg_ctx arrays altogether.

So the patches could be something like this:

1) SPARC: put the jump target information directly in gen_opc_* without using gen_opc_jump_pc (not trivial)

2) a few targets: instead of gen_opc_* arrays, use a new generic member of tcg_ctx (similar to how csbase is used generically), e.g. tcg_ctx.gen_opc_target1[] and tcg_ctx.gen_opc_target2[].

3) all targets: always fill in tcg_ctx.gen_*, even if search_pc is false

4) TCG: add support for a checkpoint operation, make it fill in tcg_ctx.gen_*

Quoted 6 lines

Quoted 4 lines

5) all targets: change explicit filling of tcg_ctx.gen_* to use the checkpoint operation

6) TCG/translate-all: convert gen_intermediate_code_pc as outlined above

That's sounds like a plan when I have more time ;-)

Doesn't this approach still require my fixes to work correctly?

Quoted 9 lines

From: Aurelien Jarno [mailto:[email protected]]
Looking at how icount work, I see it's basically a variable in the CPU state (icount_decr.u16.low), which is already accessed from the TB. Couldn't we adjust it using additional code before generating an exception, when in icount mode.

For example for MIPS, we can add some code before generate_exception which use the value from s->gen_opc_icount[j] to adjust the variable icount_decr.u16.low.

It is possible, but it will incur additional overhead, because we will have to update icount every time the exception might be generated. We'll have to update icount value before and after every helper call, that can cause an exception:

icount -= n
...
instr_k
icount += n - k
helper
icount -= n - k
...

And this overhead will slowdown the code even if no exception occur.

From: Peter Maydell [mailto:[email protected]]

Quoted 23 lines · Pavel Dovgaluk

Quoted 9 lines

From: Aurelien Jarno [mailto:[email protected]]
Looking at how icount work, I see it's basically a variable in the CPU state (icount_decr.u16.low), which is already accessed from the TB. Couldn't we adjust it using additional code before generating an exception, when in icount mode.

For example for MIPS, we can add some code before generate_exception which use the value from s->gen_opc_icount[j] to adjust the variable icount_decr.u16.low.

It is possible, but it will incur additional overhead, because we will have to update icount every time the exception might be generated. We'll have to update icount value before and after every helper call, that can cause an exception:

icount -= n
...
instr_k
icount += n - k
helper
icount -= n - k
...

Quoted 1 line

And this overhead will slowdown the code even if no exception occur.

Right, this is a tradeoff: in some cases it's faster to assume no exception and handle state resync by doing a retranslate. In some cases it's faster to assume there will be an exception and do a manual sync. Guest load/store is obviously in the first category. Guest doing an instruction which always takes an exception (like syscall insns) is in the second category. For other cases there's a choice. We need to support both approaches; obviously you can argue for any particular case whether it should be approach 1 or approach 2.