[Qemu-devel] [PATCH] Introduce foreach_cpu shorthand

[Qemu-devel] [PATCH] Introduce foreach_cpu shorthand

[Jan Kiszka]

Cleanup patch to consolidate open-coded iterations over all CPUs via a
foreach_cpu wrapper. Improves readability IMHO.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 cpu-defs.h |    3 +++
 exec.c     |    8 +++-----
 monitor.c  |    6 +++---
 vl.c       |    4 ++--
 4 files changed, 11 insertions(+), 10 deletions(-)

Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -171,4 +171,7 @@ typedef struct CPUTLBEntry {
                                                                         \
     const char *cpu_model_str;
 
+#define foreach_cpu(env) \
+    for (env = first_cpu; env != NULL; env = env->next_cpu)
+
 #endif
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -639,10 +639,9 @@ static inline void tb_phys_invalidate(Tr
 
     /* remove the TB from the hash list */
     h = tb_jmp_cache_hash_func(tb->pc);
-    for(env = first_cpu; env != NULL; env = env->next_cpu) {
+    foreach_cpu(env)
         if (env->tb_jmp_cache[h] == tb)
             env->tb_jmp_cache[h] = NULL;
-    }
 
     /* suppress this TB from the two jump lists */
     tb_jmp_remove(tb, 0);
@@ -1649,7 +1648,7 @@ void cpu_physical_memory_reset_dirty(ram
     /* we modify the TLB cache so that the dirty bit will be set again
        when accessing the range */
     start1 = start + (unsigned long)phys_ram_base;
-    for(env = first_cpu; env != NULL; env = env->next_cpu) {
+    foreach_cpu(env) {
         for(i = 0; i < CPU_TLB_SIZE; i++)
             tlb_reset_dirty_range(&env->tlb_table[0][i], start1, length);
         for(i = 0; i < CPU_TLB_SIZE; i++)
@@ -2217,9 +2216,8 @@ void cpu_register_physical_memory(target
     /* since each CPU stores ram addresses in its TLB cache, we must
        reset the modified entries */
     /* XXX: slow ! */
-    for(env = first_cpu; env != NULL; env = env->next_cpu) {
+    foreach_cpu(env)
         tlb_flush(env, 1);
-    }
 }
 
 /* XXX: temporary until new memory mapping API */
Index: b/monitor.c
===================================================================
--- a/monitor.c
+++ b/monitor.c
@@ -269,7 +269,7 @@ static int mon_set_cpu(int cpu_index)
 {
     CPUState *env;
 
-    for(env = first_cpu; env != NULL; env = env->next_cpu) {
+    foreach_cpu(env) {
         if (env->cpu_index == cpu_index) {
             mon_cpu = env;
             return 0;
@@ -308,7 +308,7 @@ static void do_info_cpus(void)
     /* just to set the default cpu if not already done */
     mon_get_cpu();
 
-    for(env = first_cpu; env != NULL; env = env->next_cpu) {
+    foreach_cpu(env) {
         term_printf("%c CPU #%d:",
                     (env == mon_cpu) ? '*' : ' ',
                     env->cpu_index);
@@ -1297,7 +1297,7 @@ static void do_inject_nmi(int cpu_index)
 {
     CPUState *env;
 
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
+    foreach_cpu(env)
         if (env->cpu_index == cpu_index) {
             cpu_interrupt(env, CPU_INTERRUPT_NMI);
             break;
Index: b/vl.c
===================================================================
--- a/vl.c
+++ b/vl.c
@@ -470,7 +470,7 @@ void hw_error(const char *fmt, ...)
     fprintf(stderr, "qemu: hardware error: ");
     vfprintf(stderr, fmt, ap);
     fprintf(stderr, "\n");
-    for(env = first_cpu; env != NULL; env = env->next_cpu) {
+    foreach_cpu(env) {
         fprintf(stderr, "CPU #%d:\n", env->cpu_index);
 #ifdef TARGET_I386
         cpu_dump_state(env, stderr, fprintf, X86_DUMP_FPU);
@@ -6047,7 +6047,7 @@ static int qemu_loadvm_state(QEMUFile *f
         ret = -1;
         goto the_end;
     }
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
+    foreach_cpu(env)
         env->interrupt_request = 0;
     total_len = qemu_get_be64(f);
     end_pos = total_len + qemu_ftell(f);

Re: [Qemu-devel] [PATCH] Introduce foreach_cpu shorthand

[Anthony Liguori]

Jan Kiszka wrote:
> Cleanup patch to consolidate open-coded iterations over all CPUs via a
> foreach_cpu wrapper. Improves readability IMHO.
>   

I agree, but I recall Fabrice reverting a bit of code of mine a long 
time ago that did something like this because he didn't like #define'ing 
for loops.  Should get him to weigh in.

Regards,

Anthony Liguori

> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> ---
>  cpu-defs.h |    3 +++
>  exec.c     |    8 +++-----
>  monitor.c  |    6 +++---
>  vl.c       |    4 ++--
>  4 files changed, 11 insertions(+), 10 deletions(-)
>
> Index: b/cpu-defs.h
> ===================================================================
> --- a/cpu-defs.h
> +++ b/cpu-defs.h
> @@ -171,4 +171,7 @@ typedef struct CPUTLBEntry {
>                                                                          \
>      const char *cpu_model_str;
>  
> +#define foreach_cpu(env) \
> +    for (env = first_cpu; env != NULL; env = env->next_cpu)
> +
>  #endif
> Index: b/exec.c
> ===================================================================
> --- a/exec.c
> +++ b/exec.c
> @@ -639,10 +639,9 @@ static inline void tb_phys_invalidate(Tr
>  
>      /* remove the TB from the hash list */
>      h = tb_jmp_cache_hash_func(tb->pc);
> -    for(env = first_cpu; env != NULL; env = env->next_cpu) {
> +    foreach_cpu(env)
>          if (env->tb_jmp_cache[h] == tb)
>              env->tb_jmp_cache[h] = NULL;
> -    }
>  
>      /* suppress this TB from the two jump lists */
>      tb_jmp_remove(tb, 0);
> @@ -1649,7 +1648,7 @@ void cpu_physical_memory_reset_dirty(ram
>      /* we modify the TLB cache so that the dirty bit will be set again
>         when accessing the range */
>      start1 = start + (unsigned long)phys_ram_base;
> -    for(env = first_cpu; env != NULL; env = env->next_cpu) {
> +    foreach_cpu(env) {
>          for(i = 0; i < CPU_TLB_SIZE; i++)
>              tlb_reset_dirty_range(&env->tlb_table[0][i], start1, length);
>          for(i = 0; i < CPU_TLB_SIZE; i++)
> @@ -2217,9 +2216,8 @@ void cpu_register_physical_memory(target
>      /* since each CPU stores ram addresses in its TLB cache, we must
>         reset the modified entries */
>      /* XXX: slow ! */
> -    for(env = first_cpu; env != NULL; env = env->next_cpu) {
> +    foreach_cpu(env)
>          tlb_flush(env, 1);
> -    }
>  }
>  
>  /* XXX: temporary until new memory mapping API */
> Index: b/monitor.c
> ===================================================================
> --- a/monitor.c
> +++ b/monitor.c
> @@ -269,7 +269,7 @@ static int mon_set_cpu(int cpu_index)
>  {
>      CPUState *env;
>  
> -    for(env = first_cpu; env != NULL; env = env->next_cpu) {
> +    foreach_cpu(env) {
>          if (env->cpu_index == cpu_index) {
>              mon_cpu = env;
>              return 0;
> @@ -308,7 +308,7 @@ static void do_info_cpus(void)
>      /* just to set the default cpu if not already done */
>      mon_get_cpu();
>  
> -    for(env = first_cpu; env != NULL; env = env->next_cpu) {
> +    foreach_cpu(env) {
>          term_printf("%c CPU #%d:",
>                      (env == mon_cpu) ? '*' : ' ',
>                      env->cpu_index);
> @@ -1297,7 +1297,7 @@ static void do_inject_nmi(int cpu_index)
>  {
>      CPUState *env;
>  
> -    for (env = first_cpu; env != NULL; env = env->next_cpu)
> +    foreach_cpu(env)
>          if (env->cpu_index == cpu_index) {
>              cpu_interrupt(env, CPU_INTERRUPT_NMI);
>              break;
> Index: b/vl.c
> ===================================================================
> --- a/vl.c
> +++ b/vl.c
> @@ -470,7 +470,7 @@ void hw_error(const char *fmt, ...)
>      fprintf(stderr, "qemu: hardware error: ");
>      vfprintf(stderr, fmt, ap);
>      fprintf(stderr, "\n");
> -    for(env = first_cpu; env != NULL; env = env->next_cpu) {
> +    foreach_cpu(env) {
>          fprintf(stderr, "CPU #%d:\n", env->cpu_index);
>  #ifdef TARGET_I386
>          cpu_dump_state(env, stderr, fprintf, X86_DUMP_FPU);
> @@ -6047,7 +6047,7 @@ static int qemu_loadvm_state(QEMUFile *f
>          ret = -1;
>          goto the_end;
>      }
> -    for (env = first_cpu; env != NULL; env = env->next_cpu)
> +    foreach_cpu(env)
>          env->interrupt_request = 0;
>      total_len = qemu_get_be64(f);
>      end_pos = total_len + qemu_ftell(f);
>
>
>   

Re: [Qemu-devel] [PATCH] Introduce foreach_cpu shorthand

[Fabrice Bellard]

Hi,

I tend to dislike such macros...

Fabrice.

Jan Kiszka wrote:
> Cleanup patch to consolidate open-coded iterations over all CPUs via a
> foreach_cpu wrapper. Improves readability IMHO.
> [...]

[Qemu-devel] Re: [PATCH] Introduce foreach_cpu shorthand

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 88 bytes --]

Fabrice Bellard wrote:
> Hi,
> 
> I tend to dislike such macros...

Why?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] Re: [PATCH] Introduce foreach_cpu shorthand

[Thiemo Seufer]

Jan Kiszka wrote:
> Fabrice Bellard wrote:
> > Hi,
> > 
> > I tend to dislike such macros...
> 
> Why?

IHMO it isn't a useful abstraction (and therefore code obfuscation).


Thiemo

Re: [Qemu-devel] Re: [PATCH] Introduce foreach_cpu shorthand

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

Thiemo Seufer wrote:
> Jan Kiszka wrote:
>> Fabrice Bellard wrote:
>>> Hi,
>>>
>>> I tend to dislike such macros...
>> Why?
> 
> IHMO it isn't a useful abstraction (and therefore code obfuscation).

What possible misunderstandings do you see in its naming? Granted, I'm
used to it as this scheme is standard under Linux (with variations for
CPU hotplugging).

The code becomes shorter and thus quicker graspable IMO. And I guess the
more QEMU heads for SMP guests, there more often this pattern will
occur. My debugger SMP fix already comes with 5 more use cases.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] Re: [PATCH] Introduce foreach_cpu shorthand

[Paul Brook]

[foreach_cpu]
> > I tend to dislike such macros...
>
> Why?

I agree with Fabrice in this case. Iterating over all cpus isn't a 
particularly onerous task to start with. Using bizarre macros means that the 
code no longer looks like normal C.

> My debugger SMP fix already comes with 5 more use cases.

I'm unconvinced by SMP debugger support. FSF gdb isn't currently capable of 
handling SMP targets, so whatever you're doing is likely to be an ugly hack.

Paul

[Qemu-devel] Re: [PATCH] Introduce foreach_cpu shorthand

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

Paul Brook wrote:
> [foreach_cpu]
>>> I tend to dislike such macros...
>> Why?
> 
> I agree with Fabrice in this case. Iterating over all cpus isn't a 
> particularly onerous task to start with. Using bizarre macros means that the 
> code no longer looks like normal C.

OK, got it. Looks like tastes are different here.

> 
>> My debugger SMP fix already comes with 5 more use cases.
> 
> I'm unconvinced by SMP debugger support. FSF gdb isn't currently capable of 
> handling SMP targets, so whatever you're doing is likely to be an ugly hack.

Well, it makes things useable on SMP hosts. Please suggest alternatives
if you think that this is the wrong approach, but leaving this unfixed
would be a mistake IMO.

Also note that gdb used for this case is in no way different from gdb
used with the kgdb stub. In both cases, the debugger (silently) expects
global scope of breakpoints. And that is what my patch ensures + it
enables focus switching for the gdbstub. Again, I'm open for alternative
approaches to achieve comparable usability. I can only repeat that
debugging SMP guest _depends_ on such changes.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]