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 = _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(, 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.fedo...@linaro.org>

Thanks,
Sergey

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 = _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(, 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

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

[Sergey Fedorov]

From: Sergey Fedorov 

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 
Signed-off-by: Sergey Fedorov 
Reviewed-by: Richard Henderson 
---
 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(0x, 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 = _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(, 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, _tb);
 tb = tb_find_fast(cpu, _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 

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 

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 
Signed-off-by: Sergey Fedorov 
---
 cpu-exec.c | 119 +
 1 file changed, 64 insertions(+), 55 deletions(-)


Reviewed-by: Richard Henderson 


r~

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

[Sergey Fedorov]

From: Sergey Fedorov 

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 
Signed-off-by: Sergey Fedorov 
---
 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(0x, 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 = _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(, 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, _tb);
 tb = tb_find_fast(cpu, _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) {
-