[Qemu-devel] [PATCH 0/4] Clean up cpu_exec part 2/n

[Qemu-devel] [PATCH 0/4] Clean up cpu_exec part 2/n

[Paolo Bonzini]

The management of env->current_tb is quite complicated.  In particular,
a while loop that has it as a test condition is actually executed just
once, and it is cleared long after it has ceased being meaningful.

This patch set straightens things a bit.  Patch 1 clears env->current_tb
when it is not meaningful anymore.  Patch 2 adds assertions that test
the change done in patch 3.  These are then removed in patch 4.

I preferred to be defensive, but I'd understand squashing the three 
patches together as well.

Paolo Bonzini (4):
  clean up env->current_tb
  add assertions about env->current_tb
  change while to if
  remove assertions

 cpu-exec.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH 1/4] clean up env->current_tb

[Paolo Bonzini]

There are three paths from the innermost while loop of cpu_exec
to the top of the outermost for loop.  Two do not reset
env->current_tb.  Fix this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 6f6ed14..9128df9 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -56,6 +56,7 @@ int qemu_cpu_has_work(CPUState *env)
 
 void cpu_loop_exit(void)
 {
+    env->current_tb = NULL;
     longjmp(env->jmp_env, 1);
 }
 
@@ -107,6 +108,7 @@ static void cpu_exec_nocache(int max_cycles, TranslationBlock *orig_tb)
     env->current_tb = tb;
     /* execute the generated code */
     next_tb = tcg_qemu_tb_exec(tb->tc_ptr);
+    env->current_tb = NULL;
 
     if ((next_tb & 3) == 2) {
         /* Restore PC.  This may happen if async event occurs before
-- 
1.6.5.2

[Qemu-devel] [PATCH 2/4] add assertions about env->current_tb

[Paolo Bonzini]

By virtue of the previous patch env->current_tb will always be NULL at
the top of cpu_exec's outermost for loop, and at the end of the innermost
while loop.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 9128df9..d974141 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -22,6 +22,8 @@
 #include "tcg.h"
 #include "kvm.h"
 
+#include <assert.h>
+
 #if !defined(CONFIG_SOFTMMU)
 #undef EAX
 #undef ECX
@@ -260,7 +262,7 @@ int cpu_exec(CPUState *env1)
                     env = cpu_single_env;
 #define env cpu_single_env
 #endif
-            env->current_tb = NULL;
+            assert (env->current_tb == NULL);
             /* if an exception is pending, we execute it here */
             if (env->exception_index >= 0) {
                 if (env->exception_index >= EXCP_INTERRUPT) {
@@ -595,6 +597,7 @@ int cpu_exec(CPUState *env1)
                 }
                 spin_unlock(&tb_lock);
                 env->current_tb = tb;
+                assert (env->current_tb);
 
                 /* cpu_interrupt might be called while translating the
                    TB, but before it is linked into a potentially
@@ -640,6 +643,7 @@ int cpu_exec(CPUState *env1)
                             cpu_loop_exit();
                         }
                     }
+                    assert (env->current_tb == NULL);
                 }
                 /* reset soft MMU for next block (it can currently
                    only be set by a memory fault) */
-- 
1.6.5.2

[Qemu-devel] [PATCH 3/4] change while to if

[Paolo Bonzini]

The while loop will be executed exactly 0 or 1 times, depending on
env->exit_request.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index d974141..f00151f 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -596,17 +596,13 @@ int cpu_exec(CPUState *env1)
                     tb_add_jump((TranslationBlock *)(next_tb & ~3), next_tb & 3, tb);
                 }
                 spin_unlock(&tb_lock);
-                env->current_tb = tb;
-                assert (env->current_tb);
 
                 /* cpu_interrupt might be called while translating the
                    TB, but before it is linked into a potentially
                    infinite loop and becomes env->current_tb. Avoid
                    starting execution if there is a pending interrupt. */
-                if (unlikely (env->exit_request))
-                    env->current_tb = NULL;
-
-                while (env->current_tb) {
+                if (!unlikely (env->exit_request)) {
+                    env->current_tb = tb;
                     tc_ptr = tb->tc_ptr;
                 /* execute the generated code */
 #if defined(__sparc__) && !defined(CONFIG_SOLARIS)
@@ -643,8 +639,8 @@ int cpu_exec(CPUState *env1)
                             cpu_loop_exit();
                         }
                     }
-                    assert (env->current_tb == NULL);
                 }
+                assert (env->current_tb == NULL);
                 /* reset soft MMU for next block (it can currently
                    only be set by a memory fault) */
             } /* for(;;) */
-- 
1.6.5.2

[Qemu-devel] [PATCH 4/4] remove assertions

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index f00151f..0256edf 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -22,8 +22,6 @@
 #include "tcg.h"
 #include "kvm.h"
 
-#include <assert.h>
-
 #if !defined(CONFIG_SOFTMMU)
 #undef EAX
 #undef ECX
@@ -262,7 +260,6 @@ int cpu_exec(CPUState *env1)
                     env = cpu_single_env;
 #define env cpu_single_env
 #endif
-            assert (env->current_tb == NULL);
             /* if an exception is pending, we execute it here */
             if (env->exception_index >= 0) {
                 if (env->exception_index >= EXCP_INTERRUPT) {
@@ -640,7 +637,6 @@ int cpu_exec(CPUState *env1)
                         }
                     }
                 }
-                assert (env->current_tb == NULL);
                 /* reset soft MMU for next block (it can currently
                    only be set by a memory fault) */
             } /* for(;;) */
-- 
1.6.5.2

Re: [Qemu-devel] [PATCH 1/4] clean up env->current_tb

[Anthony Liguori]

On 01/15/2010 02:42 AM, Paolo Bonzini wrote:
> There are three paths from the innermost while loop of cpu_exec
> to the top of the outermost for loop.  Two do not reset
> env->current_tb.  Fix this.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   cpu-exec.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 6f6ed14..9128df9 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -56,6 +56,7 @@ int qemu_cpu_has_work(CPUState *env)
>
>   void cpu_loop_exit(void)
>   {
> +    env->current_tb = NULL;
>       longjmp(env->jmp_env, 1);
>   }
>
> @@ -107,6 +108,7 @@ static void cpu_exec_nocache(int max_cycles, TranslationBlock *orig_tb)
>       env->current_tb = tb;
>       /* execute the generated code */
>       next_tb = tcg_qemu_tb_exec(tb->tc_ptr);
> +    env->current_tb = NULL;
>
>       if ((next_tb&  3) == 2) {
>           /* Restore PC.  This may happen if async event occurs before
>