[4/5] cpu-exec: Move TB execution stuff out of cpu_exec()

Commit Message

[e-mail] May 11, 2016, 10:21 a.m. UTC

From: Sergey Fedorov <[e-mail]>

Simplify cpu_exec() by extracting TB execution code outside of
cpu_exec() into a new static inline function cpu_loop_exec_tb().

Signed-off-by: Sergey Fedorov <[e-mail]>
Signed-off-by: Sergey Fedorov <[e-mail]>
Reviewed-by: Richard Henderson <[e-mail]>
---
 cpu-exec.c | 119 +++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 64 insertions(+), 55 deletions(-)

Comments

Stefan Weil July 15, 2016, 6:45 a.m. UTC | #1

Hi,

Am 11.05.2016 um 12:21 schrieb Sergey Fedorov:
[...]
>   int cpu_exec(CPUState *cpu)
> @@ -516,8 +576,6 @@ int cpu_exec(CPUState *cpu)
>      CPUArchState *env = &x86_cpu->env;
>  #endif
>      int ret;
> -    TranslationBlock *tb, *last_tb;
> -    int tb_exit = 0;

Here tb_exit was only once set to 0, ...

>      SyncClocks sc;
>  
>      /* replay_interrupt may need current_cpu */
> @@ -544,6 +602,9 @@ int cpu_exec(CPUState *cpu)
>      init_delay_params(&sc, cpu);
>  
>      for(;;) {
> +        TranslationBlock *tb, *last_tb;
> +        int tb_exit = 0;

... while now it is zeroed in each iteration of the for loop.
I'm not sure whether the new code is still correct.

If it is, ...

> +
>          /* prepare setjmp context for exception handling */
>          if (sigsetjmp(cpu->jmp_env, 0) == 0) {

... the declaration of tb_exit could also be done here, after the sigsetjmp.
That would fix a compiler warning which I get when compiling with
-Wclobbered:


    cpu-exec.c:603:13: warning: variable ‘tb_exit’ might be clobbered by
‘longjmp’ or ‘vfork’ [-Wclobbered]


Regards

Stefan

Sergey Fedorov July 15, 2016, 7:32 p.m. UTC | #2

On 15/07/16 09:45, Stefan Weil wrote:
> Hi,
>
> Am 11.05.2016 um 12:21 schrieb Sergey Fedorov:
> [...]
>>   int cpu_exec(CPUState *cpu)
>> @@ -516,8 +576,6 @@ int cpu_exec(CPUState *cpu)
>>      CPUArchState *env = &x86_cpu->env;
>>  #endif
>>      int ret;
>> -    TranslationBlock *tb, *last_tb;
>> -    int tb_exit = 0;
> Here tb_exit was only once set to 0, ...
>
>>      SyncClocks sc;
>>  
>>      /* replay_interrupt may need current_cpu */
>> @@ -544,6 +602,9 @@ int cpu_exec(CPUState *cpu)
>>      init_delay_params(&sc, cpu);
>>  
>>      for(;;) {
>> +        TranslationBlock *tb, *last_tb;
>> +        int tb_exit = 0;
> ... while now it is zeroed in each iteration of the for loop.
> I'm not sure whether the new code is still correct.

That is okay because 'tb_exit' only makes sense when "last_tb != NULL".
But we always reset 'last_tb' in this loop:

    last_tb = NULL; /* forget the last executed TB after exception */

>
> If it is, ...
>
>> +
>>          /* prepare setjmp context for exception handling */
>>          if (sigsetjmp(cpu->jmp_env, 0) == 0) {
> ... the declaration of tb_exit could also be done here, after the sigsetjmp.
> That would fix a compiler warning which I get when compiling with
> -Wclobbered:
>
>
>     cpu-exec.c:603:13: warning: variable ‘tb_exit’ might be clobbered by
> ‘longjmp’ or ‘vfork’ [-Wclobbered]

I've sent the patch to fix this:

    Message-Id: <[e-mail]>

Thanks,
Sergey

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index a053b5e73af1..197861f8407b 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -506,6 +506,66 @@  static inline void cpu_handle_interrupt(CPUState *cpu,
     }
 }
 
+static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
+                                    TranslationBlock **last_tb, int *tb_exit,
+                                    SyncClocks *sc)
+{
+    uintptr_t ret;
+
+    if (unlikely(cpu->exit_request)) {
+        return;
+    }
+
+    trace_exec_tb(tb, tb->pc);
+    ret = cpu_tb_exec(cpu, tb);
+    *last_tb = (TranslationBlock *)(ret & ~TB_EXIT_MASK);
+    *tb_exit = ret & TB_EXIT_MASK;
+    switch (*tb_exit) {
+    case TB_EXIT_REQUESTED:
+        /* Something asked us to stop executing
+         * chained TBs; just continue round the main
+         * loop. Whatever requested the exit will also
+         * have set something else (eg exit_request or
+         * interrupt_request) which we will handle
+         * next time around the loop.  But we need to
+         * ensure the tcg_exit_req read in generated code
+         * comes before the next read of cpu->exit_request
+         * or cpu->interrupt_request.
+         */
+        smp_rmb();
+        *last_tb = NULL;
+        break;
+    case TB_EXIT_ICOUNT_EXPIRED:
+    {
+        /* Instruction counter expired.  */
+#ifdef CONFIG_USER_ONLY
+        abort();
+#else
+        int insns_left = cpu->icount_decr.u32;
+        if (cpu->icount_extra && insns_left >= 0) {
+            /* Refill decrementer and continue execution.  */
+            cpu->icount_extra += insns_left;
+            insns_left = MIN(0xffff, cpu->icount_extra);
+            cpu->icount_extra -= insns_left;
+            cpu->icount_decr.u16.low = insns_left;
+        } else {
+            if (insns_left > 0) {
+                /* Execute remaining instructions.  */
+                cpu_exec_nocache(cpu, insns_left, *last_tb, false);
+                align_clocks(sc, cpu);
+            }
+            cpu->exception_index = EXCP_INTERRUPT;
+            *last_tb = NULL;
+            cpu_loop_exit(cpu);
+        }
+        break;
+#endif
+    }
+    default:
+        break;
+    }
+}
+
 /* main execution loop */
 
 int cpu_exec(CPUState *cpu)
@@ -516,8 +576,6 @@  int cpu_exec(CPUState *cpu)
     CPUArchState *env = &x86_cpu->env;
 #endif
     int ret;
-    TranslationBlock *tb, *last_tb;
-    int tb_exit = 0;
     SyncClocks sc;
 
     /* replay_interrupt may need current_cpu */
@@ -544,6 +602,9 @@  int cpu_exec(CPUState *cpu)
     init_delay_params(&sc, cpu);
 
     for(;;) {
+        TranslationBlock *tb, *last_tb;
+        int tb_exit = 0;
+
         /* prepare setjmp context for exception handling */
         if (sigsetjmp(cpu->jmp_env, 0) == 0) {
             /* if an exception is pending, we execute it here */
@@ -556,59 +617,7 @@  int cpu_exec(CPUState *cpu)
             for(;;) {
                 cpu_handle_interrupt(cpu, &last_tb);
                 tb = tb_find_fast(cpu, &last_tb, tb_exit);
-                if (likely(!cpu->exit_request)) {
-                    uintptr_t ret;
-                    trace_exec_tb(tb, tb->pc);
-                    /* execute the generated code */
-                    ret = cpu_tb_exec(cpu, tb);
-                    last_tb = (TranslationBlock *)(ret & ~TB_EXIT_MASK);
-                    tb_exit = ret & TB_EXIT_MASK;
-                    switch (tb_exit) {
-                    case TB_EXIT_REQUESTED:
-                        /* Something asked us to stop executing
-                         * chained TBs; just continue round the main
-                         * loop. Whatever requested the exit will also
-                         * have set something else (eg exit_request or
-                         * interrupt_request) which we will handle
-                         * next time around the loop.  But we need to
-                         * ensure the tcg_exit_req read in generated code
-                         * comes before the next read of cpu->exit_request
-                         * or cpu->interrupt_request.
-                         */
-                        smp_rmb();
-                        last_tb = NULL;
-                        break;
-                    case TB_EXIT_ICOUNT_EXPIRED:
-                    {
-                        /* Instruction counter expired.  */
-#ifdef CONFIG_USER_ONLY
-                        abort();
-#else
-                        int insns_left = cpu->icount_decr.u32;
-                        if (cpu->icount_extra && insns_left >= 0) {
-                            /* Refill decrementer and continue execution.  */
-                            cpu->icount_extra += insns_left;
-                            insns_left = MIN(0xffff, cpu->icount_extra);
-                            cpu->icount_extra -= insns_left;
-                            cpu->icount_decr.u16.low = insns_left;
-                        } else {
-                            if (insns_left > 0) {
-                                /* Execute remaining instructions.  */
-                                cpu_exec_nocache(cpu, insns_left,
-                                                 last_tb, false);
-                                align_clocks(&sc, cpu);
-                            }
-                            cpu->exception_index = EXCP_INTERRUPT;
-                            last_tb = NULL;
-                            cpu_loop_exit(cpu);
-                        }
-                        break;
-#endif
-                    }
-                    default:
-                        break;
-                    }
-                }
+                cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
                 /* Try to align the host and virtual clocks
                    if the guest is in advance */
                 align_clocks(&sc, cpu);