Introduce foreach_cpu shorthand
PATCH 1/1 Introduce foreach_cpu shorthand
Cleanup patch to consolidate open-coded iterations over all CPUs via a foreach_cpu wrapper. Improves readability IMHO.
--- cpu-defs.h | 3 +++ exec.c | 8 +++----- monitor.c | 6 +++--- vl.c | 4 ++-- 4 files changed, 11 insertions(+), 10 deletions(-)
cpu-defs.h +3 -0
@@ -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
exec.c +3 -5
@@ -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 */
monitor.c +3 -3
@@ -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 ;
vl.c +2 -2
@@ -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);
Quoted 3 lines · Jan Kiszka
Cleanup patch to consolidate open-coded iterations over all CPUs via a
foreach_cpuwrapper. 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
Quoted 112 lines
Signed-off-by Jan Kiszka <[email protected]>--- 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
===================================================================cpu-defs.h +3
-0@@ -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
===================================================================exec.c +3
-5@@ -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
===================================================================monitor.c +3
-3@@ -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
===================================================================vl.c +2
-2@@ -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);
Hi,
I tend to dislike such macros...
Fabrice.
Quoted 3 lines · Jan Kiszka
Cleanup patch to consolidate open-coded iterations over all CPUs via a
foreach_cpuwrapper. Improves readability IMHO.
[...]
Quoted 3 lines · Fabrice Bellard
Hi,
I tend to dislike such macros...
Why?
Jan
Quoted 6 lines · Jan Kiszka
Quoted 3 lines · Fabrice Bellard
Hi,
I tend to dislike such macros...
Why?
IHMO it isn't a useful abstraction (and therefore code obfuscation).
Thiemo
Quoted 8 lines · Thiemo Seufer
Quoted 5 lines · Jan Kiszka
Quoted 3 lines · Fabrice Bellard
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
[foreach_cpu]
Quoted 3 lines
Quoted 1 line
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.
Quoted 1 line
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
Quoted 7 lines · Paul Brook
[
foreach_cpu]Quoted 2 lines
Quoted 1 line
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.
Quoted 5 lines
Quoted 2 lines
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