[Qemu-devel] [PATCH 0/5] cpu-exec: Restructure cpu_exec()

[Qemu-devel] [PATCH 0/5] cpu-exec: Restructure cpu_exec()

[Sergey Fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

cpu_exec() was a huge function also sprinkled with some preprocessor
directives. It was hard to read and see the main loop crowded by all
this code. Restructure cpu_exec() by moving its conceptual parts into
separate static inline functions. That makes it possible to see the
whole main loop at once, especially its sigsetjmp() handling part.

This series is based on commit
    40f646483a11 (cpu-exec: Remove relic orphaned comment)
from
    git://github.com/rth7680/qemu.git tcg-next
and is available at
    git://github.com/sergefdrv/qemu.git cpu_exec-restructure-v2

Summary of changes in v2:
 * Declare x86_cpu variable in the inner-most block [PATCH 1/5]

Sergey Fedorov (5):
  cpu-exec: Move halt handling out of cpu_exec()
  cpu-exec: Move exception handling out of cpu_exec()
  cpu-exec: Move interrupt handling out of cpu_exec()
  cpu-exec: Move TB execution stuff out of cpu_exec()
  cpu-exec: Remove unused 'x86_cpu' and 'env' from cpu_exec()

 cpu-exec.c | 395 +++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 211 insertions(+), 184 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/5] cpu-exec: Move halt handling out of cpu_exec()

[Sergey Fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

Simplify cpu_exec() by extracting CPU halt state handling code out of
cpu_exec() into a new static inline function cpu_handle_halt().

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---

Changes in v2:
 * Declare x86_cpu variable in the inner-most block

 cpu-exec.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index d55faa5114c7..1fcdc3fabb39 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -352,6 +352,29 @@ static inline TranslationBlock *tb_find_fast(CPUState *cpu,
     return tb;
 }
 
+static inline bool cpu_handle_halt(CPUState *cpu)
+{
+    if (cpu->halted) {
+#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
+
+        if ((cpu->interrupt_request & CPU_INTERRUPT_POLL)
+            && replay_interrupt()) {
+            X86CPU *x86_cpu = X86_CPU(cpu);
+            apic_poll_irq(x86_cpu->apic_state);
+            cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
+        }
+#endif
+        if (!cpu_has_work(cpu)) {
+            current_cpu = NULL;
+            return true;
+        }
+
+        cpu->halted = 0;
+    }
+
+    return false;
+}
+
 static void cpu_handle_debug_exception(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
@@ -383,20 +406,8 @@ int cpu_exec(CPUState *cpu)
     /* replay_interrupt may need current_cpu */
     current_cpu = cpu;
 
-    if (cpu->halted) {
-#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
-        if ((cpu->interrupt_request & CPU_INTERRUPT_POLL)
-            && replay_interrupt()) {
-            apic_poll_irq(x86_cpu->apic_state);
-            cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
-        }
-#endif
-        if (!cpu_has_work(cpu)) {
-            current_cpu = NULL;
-            return EXCP_HALTED;
-        }
-
-        cpu->halted = 0;
+    if (cpu_handle_halt(cpu)) {
+        return EXCP_HALTED;
     }
 
     atomic_mb_set(&tcg_current_cpu, cpu);
-- 
1.9.1

[Qemu-devel] [PATCH 2/5] cpu-exec: Move exception handling out of cpu_exec()

[Sergey Fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

Simplify cpu_exec() by extracting exception handling code out of
cpu_exec() into a new static inline function cpu_handle_exception().
Also make cpu_handle_debug_exception() inline as it is used only once.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 cpu-exec.c | 93 +++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 52 insertions(+), 41 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 1fcdc3fabb39..f7e175e9ae1c 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -375,7 +375,7 @@ static inline bool cpu_handle_halt(CPUState *cpu)
     return false;
 }
 
-static void cpu_handle_debug_exception(CPUState *cpu)
+static inline void cpu_handle_debug_exception(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUWatchpoint *wp;
@@ -389,6 +389,55 @@ static void cpu_handle_debug_exception(CPUState *cpu)
     cc->debug_excp_handler(cpu);
 }
 
+static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
+{
+    if (cpu->exception_index >= 0) {
+        if (cpu->exception_index >= EXCP_INTERRUPT) {
+            /* exit request from the cpu execution loop */
+            *ret = cpu->exception_index;
+            if (*ret == EXCP_DEBUG) {
+                cpu_handle_debug_exception(cpu);
+            }
+            cpu->exception_index = -1;
+            return true;
+        } else {
+#if defined(CONFIG_USER_ONLY)
+            /* if user mode only, we simulate a fake exception
+               which will be handled outside the cpu execution
+               loop */
+#if defined(TARGET_I386)
+            CPUClass *cc = CPU_GET_CLASS(cpu);
+            cc->do_interrupt(cpu);
+#endif
+            *ret = cpu->exception_index;
+            cpu->exception_index = -1;
+            return true;
+#else
+            if (replay_exception()) {
+                CPUClass *cc = CPU_GET_CLASS(cpu);
+                cc->do_interrupt(cpu);
+                cpu->exception_index = -1;
+            } else if (!replay_has_interrupt()) {
+                /* give a chance to iothread in replay mode */
+                *ret = EXCP_INTERRUPT;
+                return true;
+            }
+#endif
+        }
+#ifndef CONFIG_USER_ONLY
+    } else if (replay_has_exception()
+               && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
+        /* try to cause an exception pending in the log */
+        TranslationBlock *last_tb = NULL; /* Avoid chaining TBs */
+        cpu_exec_nocache(cpu, 1, tb_find_fast(cpu, &last_tb, 0), true);
+        *ret = -1;
+        return true;
+#endif
+    }
+
+    return false;
+}
+
 /* main execution loop */
 
 int cpu_exec(CPUState *cpu)
@@ -426,50 +475,12 @@ int cpu_exec(CPUState *cpu)
      */
     init_delay_params(&sc, cpu);
 
-    /* prepare setjmp context for exception handling */
     for(;;) {
+        /* prepare setjmp context for exception handling */
         if (sigsetjmp(cpu->jmp_env, 0) == 0) {
             /* if an exception is pending, we execute it here */
-            if (cpu->exception_index >= 0) {
-                if (cpu->exception_index >= EXCP_INTERRUPT) {
-                    /* exit request from the cpu execution loop */
-                    ret = cpu->exception_index;
-                    if (ret == EXCP_DEBUG) {
-                        cpu_handle_debug_exception(cpu);
-                    }
-                    cpu->exception_index = -1;
-                    break;
-                } else {
-#if defined(CONFIG_USER_ONLY)
-                    /* if user mode only, we simulate a fake exception
-                       which will be handled outside the cpu execution
-                       loop */
-#if defined(TARGET_I386)
-                    cc->do_interrupt(cpu);
-#endif
-                    ret = cpu->exception_index;
-                    cpu->exception_index = -1;
-                    break;
-#else
-                    if (replay_exception()) {
-                        cc->do_interrupt(cpu);
-                        cpu->exception_index = -1;
-                    } else if (!replay_has_interrupt()) {
-                        /* give a chance to iothread in replay mode */
-                        ret = EXCP_INTERRUPT;
-                        break;
-                    }
-#endif
-                }
-#ifndef CONFIG_USER_ONLY
-            } else if (replay_has_exception()
-                       && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
-                /* try to cause an exception pending in the log */
-                last_tb = NULL; /* Avoid chaining TBs */
-                cpu_exec_nocache(cpu, 1, tb_find_fast(cpu, &last_tb, 0), true);
-                ret = -1;
+            if (cpu_handle_exception(cpu, &ret)) {
                 break;
-#endif
             }
 
             last_tb = NULL; /* forget the last executed TB after exception */
-- 
1.9.1

[Qemu-devel] [PATCH 3/5] cpu-exec: Move interrupt handling out of cpu_exec()

[Sergey Fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

Simplify cpu_exec() by extracting interrupt handling code outside of
cpu_exec() into a new static inline function cpu_handle_interrupt().

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
Reviewed-by: Richard Henderson  <rth@twiddle.net>
---
 cpu-exec.c | 132 ++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 70 insertions(+), 62 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index f7e175e9ae1c..a053b5e73af1 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -438,6 +438,74 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
     return false;
 }
 
+static inline void cpu_handle_interrupt(CPUState *cpu,
+                                        TranslationBlock **last_tb)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+    int interrupt_request = cpu->interrupt_request;
+
+    if (unlikely(interrupt_request)) {
+        if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {
+            /* Mask out external interrupts for this step. */
+            interrupt_request &= ~CPU_INTERRUPT_SSTEP_MASK;
+        }
+        if (interrupt_request & CPU_INTERRUPT_DEBUG) {
+            cpu->interrupt_request &= ~CPU_INTERRUPT_DEBUG;
+            cpu->exception_index = EXCP_DEBUG;
+            cpu_loop_exit(cpu);
+        }
+        if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
+            /* Do nothing */
+        } else if (interrupt_request & CPU_INTERRUPT_HALT) {
+            replay_interrupt();
+            cpu->interrupt_request &= ~CPU_INTERRUPT_HALT;
+            cpu->halted = 1;
+            cpu->exception_index = EXCP_HLT;
+            cpu_loop_exit(cpu);
+        }
+#if defined(TARGET_I386)
+        else if (interrupt_request & CPU_INTERRUPT_INIT) {
+            X86CPU *x86_cpu = X86_CPU(cpu);
+            CPUArchState *env = &x86_cpu->env;
+            replay_interrupt();
+            cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0);
+            do_cpu_init(x86_cpu);
+            cpu->exception_index = EXCP_HALTED;
+            cpu_loop_exit(cpu);
+        }
+#else
+        else if (interrupt_request & CPU_INTERRUPT_RESET) {
+            replay_interrupt();
+            cpu_reset(cpu);
+            cpu_loop_exit(cpu);
+        }
+#endif
+        /* The target hook has 3 exit conditions:
+           False when the interrupt isn't processed,
+           True when it is, and we should restart on a new TB,
+           and via longjmp via cpu_loop_exit.  */
+        else {
+            replay_interrupt();
+            if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
+                *last_tb = NULL;
+            }
+        }
+        /* Don't use the cached interrupt_request value,
+           do_interrupt may have updated the EXITTB flag. */
+        if (cpu->interrupt_request & CPU_INTERRUPT_EXITTB) {
+            cpu->interrupt_request &= ~CPU_INTERRUPT_EXITTB;
+            /* ensure that no TB jump will be modified as
+               the program flow was changed */
+            *last_tb = NULL;
+        }
+    }
+    if (unlikely(cpu->exit_request || replay_has_interrupt())) {
+        cpu->exit_request = 0;
+        cpu->exception_index = EXCP_INTERRUPT;
+        cpu_loop_exit(cpu);
+    }
+}
+
 /* main execution loop */
 
 int cpu_exec(CPUState *cpu)
@@ -447,7 +515,7 @@ int cpu_exec(CPUState *cpu)
     X86CPU *x86_cpu = X86_CPU(cpu);
     CPUArchState *env = &x86_cpu->env;
 #endif
-    int ret, interrupt_request;
+    int ret;
     TranslationBlock *tb, *last_tb;
     int tb_exit = 0;
     SyncClocks sc;
@@ -486,67 +554,7 @@ int cpu_exec(CPUState *cpu)
             last_tb = NULL; /* forget the last executed TB after exception */
             cpu->tb_flushed = false; /* reset before first TB lookup */
             for(;;) {
-                interrupt_request = cpu->interrupt_request;
-                if (unlikely(interrupt_request)) {
-                    if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {
-                        /* Mask out external interrupts for this step. */
-                        interrupt_request &= ~CPU_INTERRUPT_SSTEP_MASK;
-                    }
-                    if (interrupt_request & CPU_INTERRUPT_DEBUG) {
-                        cpu->interrupt_request &= ~CPU_INTERRUPT_DEBUG;
-                        cpu->exception_index = EXCP_DEBUG;
-                        cpu_loop_exit(cpu);
-                    }
-                    if (replay_mode == REPLAY_MODE_PLAY
-                        && !replay_has_interrupt()) {
-                        /* Do nothing */
-                    } else if (interrupt_request & CPU_INTERRUPT_HALT) {
-                        replay_interrupt();
-                        cpu->interrupt_request &= ~CPU_INTERRUPT_HALT;
-                        cpu->halted = 1;
-                        cpu->exception_index = EXCP_HLT;
-                        cpu_loop_exit(cpu);
-                    }
-#if defined(TARGET_I386)
-                    else if (interrupt_request & CPU_INTERRUPT_INIT) {
-                        replay_interrupt();
-                        cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0);
-                        do_cpu_init(x86_cpu);
-                        cpu->exception_index = EXCP_HALTED;
-                        cpu_loop_exit(cpu);
-                    }
-#else
-                    else if (interrupt_request & CPU_INTERRUPT_RESET) {
-                        replay_interrupt();
-                        cpu_reset(cpu);
-                        cpu_loop_exit(cpu);
-                    }
-#endif
-                    /* The target hook has 3 exit conditions:
-                       False when the interrupt isn't processed,
-                       True when it is, and we should restart on a new TB,
-                       and via longjmp via cpu_loop_exit.  */
-                    else {
-                        replay_interrupt();
-                        if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
-                            last_tb = NULL;
-                        }
-                    }
-                    /* Don't use the cached interrupt_request value,
-                       do_interrupt may have updated the EXITTB flag. */
-                    if (cpu->interrupt_request & CPU_INTERRUPT_EXITTB) {
-                        cpu->interrupt_request &= ~CPU_INTERRUPT_EXITTB;
-                        /* ensure that no TB jump will be modified as
-                           the program flow was changed */
-                        last_tb = NULL;
-                    }
-                }
-                if (unlikely(cpu->exit_request
-                             || replay_has_interrupt())) {
-                    cpu->exit_request = 0;
-                    cpu->exception_index = EXCP_INTERRUPT;
-                    cpu_loop_exit(cpu);
-                }
+                cpu_handle_interrupt(cpu, &last_tb);
                 tb = tb_find_fast(cpu, &last_tb, tb_exit);
                 if (likely(!cpu->exit_request)) {
                     uintptr_t ret;
-- 
1.9.1

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

[Sergey Fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

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 <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 cpu-exec.c | 119 +++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 64 insertions(+), 55 deletions(-)

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);
-- 
1.9.1

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

[Stefan Weil]

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

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

[Sergey Fedorov]

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: <20160715193123.28113-1-sergey.fedorov@linaro.org>

Thanks,
Sergey

[Qemu-devel] [PATCH 5/5] cpu-exec: Remove unused 'x86_cpu' and 'env' from cpu_exec()

[Sergey Fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 cpu-exec.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 197861f8407b..b57f91ced162 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -571,10 +571,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
 int cpu_exec(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
-#ifdef TARGET_I386
-    X86CPU *x86_cpu = X86_CPU(cpu);
-    CPUArchState *env = &x86_cpu->env;
-#endif
     int ret;
     SyncClocks sc;
 
@@ -630,18 +626,10 @@ int cpu_exec(CPUState *cpu)
              * Newer versions of gcc would complain about this code (-Wclobbered). */
             cpu = current_cpu;
             cc = CPU_GET_CLASS(cpu);
-#ifdef TARGET_I386
-            x86_cpu = X86_CPU(cpu);
-            env = &x86_cpu->env;
-#endif
 #else /* buggy compiler */
             /* Assert that the compiler does not smash local variables. */
             g_assert(cpu == current_cpu);
             g_assert(cc == CPU_GET_CLASS(cpu));
-#ifdef TARGET_I386
-            g_assert(x86_cpu == X86_CPU(cpu));
-            g_assert(env == &x86_cpu->env);
-#endif
 #endif /* buggy compiler */
             cpu->can_do_io = 1;
             tb_lock_reset();
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/5] cpu-exec: Restructure cpu_exec()

[Richard Henderson]

On 05/11/2016 12:21 AM, Sergey Fedorov wrote:
> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> cpu_exec() was a huge function also sprinkled with some preprocessor
> directives. It was hard to read and see the main loop crowded by all
> this code. Restructure cpu_exec() by moving its conceptual parts into
> separate static inline functions. That makes it possible to see the
> whole main loop at once, especially its sigsetjmp() handling part.
>
> This series is based on commit
>     40f646483a11 (cpu-exec: Remove relic orphaned comment)
> from
>     git://github.com/rth7680/qemu.git tcg-next
> and is available at
>     git://github.com/sergefdrv/qemu.git cpu_exec-restructure-v2
>
> Summary of changes in v2:
>  * Declare x86_cpu variable in the inner-most block [PATCH 1/5]
>
> Sergey Fedorov (5):
>   cpu-exec: Move halt handling out of cpu_exec()
>   cpu-exec: Move exception handling out of cpu_exec()
>   cpu-exec: Move interrupt handling out of cpu_exec()
>   cpu-exec: Move TB execution stuff out of cpu_exec()
>   cpu-exec: Remove unused 'x86_cpu' and 'env' from cpu_exec()
>
>  cpu-exec.c | 395 +++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 211 insertions(+), 184 deletions(-)
>

Applied to tcg-next.


r~

[Qemu-devel] [PATCH 0/5] cpu-exec: Restructure cpu_exec()

[Sergey Fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

cpu_exec() was a huge function also sprinkled with some preprocessor
directives. It was hard to read and see the main loop crowded by all
this code. Restructure cpu_exec() by moving its conceptual parts into
separate static inline functions. That makes it possible to see the
whole main loop at once, especially its sigsetjmp() handling part.

This series is based on commit
    40f646483a11 (cpu-exec: Remove relic orphaned comment)
from
    git://github.com/rth7680/qemu.git tcg-next
and is available at
    git://github.com/sergefdrv/qemu.git cpu_exec-restructure

Sergey Fedorov (5):
  cpu-exec: Move halt handling out of cpu_exec()
  cpu-exec: Move exception handling out of cpu_exec()
  cpu-exec: Move interrupt handling out of cpu_exec()
  cpu-exec: Move TB execution stuff out of cpu_exec()
  cpu-exec: Remove unused 'x86_cpu' and 'env' from cpu_exec()

 cpu-exec.c | 395 +++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 211 insertions(+), 184 deletions(-)

-- 
1.9.1

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

[Sergey Fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

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 <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 cpu-exec.c | 119 +++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 64 insertions(+), 55 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 0760d5dc274d..60f565074618 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);
-- 
1.9.1

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

[Richard Henderson]

On 05/10/2016 05:46 AM, Sergey Fedorov wrote:
> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> 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 <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  cpu-exec.c | 119 +++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 64 insertions(+), 55 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~