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

Commit Message

[e-mail] May 10, 2016, 3:46 p.m. UTC

From: Sergey Fedorov <[e-mail]>

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 <[e-mail]>
Signed-off-by: Sergey Fedorov <[e-mail]>
---
 cpu-exec.c | 132 ++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 70 insertions(+), 62 deletions(-)

Comments

Richard Henderson May 10, 2016, 4:34 p.m. UTC | #1

On 05/10/2016 05:46 AM, Sergey Fedorov wrote:
> From: Sergey Fedorov <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Sergey Fedorov <[e-mail]>
> ---
>  cpu-exec.c | 132 ++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 70 insertions(+), 62 deletions(-)

Reviewed-by: Richard Henderson  <[e-mail]>


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

Sergey Fedorov May 10, 2016, 7:24 p.m. UTC | #2

On 10/05/16 19:34, Richard Henderson wrote:
> On 05/10/2016 05:46 AM, Sergey Fedorov wrote:
>> From: Sergey Fedorov <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Sergey Fedorov <[e-mail]>
>> ---
>>  cpu-exec.c | 132
>> ++++++++++++++++++++++++++++++++-----------------------------
>>  1 file changed, 70 insertions(+), 62 deletions(-)
>
> Reviewed-by: Richard Henderson  <[e-mail]>
>
>
>> +        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

Patch

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;