[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

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>
---
 cpu-exec.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index d55faa5114c7..fb72f102742c 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)
+        X86CPU *x86_cpu = X86_CPU(cpu);
+
+        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 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

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

[Richard Henderson]

On 05/10/2016 05:46 AM, Sergey Fedorov wrote:
> +#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
> +        X86CPU *x86_cpu = X86_CPU(cpu);
> +
> +        if ((cpu->interrupt_request & CPU_INTERRUPT_POLL)
> +            && replay_interrupt()) {
> +            apic_poll_irq(x86_cpu->apic_state);

Since you're moving this around, you might as well place the x86_cpu variable 
in the inner-most if, next to its only use.

Otherwise,

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


r~

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

[Sergey Fedorov]

On 10/05/16 19:13, Richard Henderson wrote:
> On 05/10/2016 05:46 AM, Sergey Fedorov wrote:
>> +#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
>> +        X86CPU *x86_cpu = X86_CPU(cpu);
>> +
>> +        if ((cpu->interrupt_request & CPU_INTERRUPT_POLL)
>> +            && replay_interrupt()) {
>> +            apic_poll_irq(x86_cpu->apic_state);
>
> Since you're moving this around, you might as well place the x86_cpu
> variable in the inner-most if, next to its only use.

Agree, will fix in v2.

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

Kind regards,
Sergey

[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>
---
 cpu-exec.c | 93 +++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 52 insertions(+), 41 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index fb72f102742c..ad95ace38dd9 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

Re: [Qemu-devel] [PATCH 2/5] cpu-exec: Move exception handling 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 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.

If it's used only once, the compiler is going to do this anyway, and therefore 
there's no point in making the change.  Let's just leave off all the inline 
markers and trust the compiler, eh?

Otherwise,

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


r~

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

[Sergey Fedorov]

On 10/05/16 19:21, Richard Henderson wrote:
> On 05/10/2016 05:46 AM, Sergey Fedorov wrote:
>> 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.
>
> If it's used only once, the compiler is going to do this anyway, and
> therefore there's no point in making the change.  Let's just leave off
> all the inline markers and trust the compiler, eh?

I agree the compiler is smart enough to decide and inline such functions
by itself. But actually, I hope such "static inline" in .c file could
indicate for a reader of the code that this function is going to be used
this way.

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

Kind regards,
Sergey

[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>
---
 cpu-exec.c | 132 ++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 70 insertions(+), 62 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index ad95ace38dd9..0760d5dc274d 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

Re: [Qemu-devel] [PATCH 3/5] cpu-exec: Move interrupt handling 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 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>
> ---
>  cpu-exec.c | 132 ++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 70 insertions(+), 62 deletions(-)

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


> +        if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
> +            /* Do nothing */
> +        } else if (interrupt_request & CPU_INTERRUPT_HALT) {
> +        }
> +        else if (interrupt_request & CPU_INTERRUPT_RESET) {
> +        }
> +        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) {

Note for future cleanup: IMO this comment is cleaner if it's actually put where 
it's meaningful (and updated to reflect that do_interrupt no longer exists).  E.g.

     else {
	if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
	    *last_tb = NULL;
	}
	/* Reload the interrupt_request value as it may have
	   been updated by the target hook.  */
	interrupt_request = cpu->interrupt_request;
     }
     if (interupt_request & CPU_INTERRUPT_EXITTB) {
	...

But such a change of course belongs in a separate patch.


r~

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

[Sergey Fedorov]

On 10/05/16 19:34, Richard Henderson wrote:
> On 05/10/2016 05:46 AM, Sergey Fedorov wrote:
>> 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>
>> ---
>>  cpu-exec.c | 132
>> ++++++++++++++++++++++++++++++++-----------------------------
>>  1 file changed, 70 insertions(+), 62 deletions(-)
>
> Reviewed-by: Richard Henderson  <rth@twiddle.net>
>
>
>> +        if (replay_mode == REPLAY_MODE_PLAY &&
>> !replay_has_interrupt()) {
>> +            /* Do nothing */
>> +        } else if (interrupt_request & CPU_INTERRUPT_HALT) {
>> +        }
>> +        else if (interrupt_request & CPU_INTERRUPT_RESET) {
>> +        }
>> +        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) {
>
> Note for future cleanup: IMO this comment is cleaner if it's actually
> put where it's meaningful (and updated to reflect that do_interrupt no
> longer exists).  E.g.
>
>     else {
>     if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
>         *last_tb = NULL;
>     }
>     /* Reload the interrupt_request value as it may have
>        been updated by the target hook.  */
>     interrupt_request = cpu->interrupt_request;
>     }
>     if (interupt_request & CPU_INTERRUPT_EXITTB) {
>     ...
>
> But such a change of course belongs in a separate patch.

Cool, thanks for the suggestion. I've had feeling this could be
expressed in a better way, like you suggest :)

Kind regards,
Sergey

[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~

[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>
---
 cpu-exec.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 60f565074618..2526df0e3e40 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 5/5] cpu-exec: Remove unused 'x86_cpu' and 'env' from cpu_exec()

[Richard Henderson]

On 05/10/2016 05:46 AM, Sergey Fedorov wrote:
> 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>
> ---
>  cpu-exec.c | 12 ------------
>  1 file changed, 12 deletions(-)

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


r~

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

[Sergey Fedorov]

On 10/05/16 18:46, 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
>
> 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(-)
>

Sorry, +CC.

Regards,
Sergey

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