Skip to contentSkip to conversation

More core code ENV_GET_CPU removals

PATCH Superseded Peter Crosthwaite 7 participants 34 messages May 25, 2015 – Jul 11, 2015

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Peter Crosthwaite

author
#0 lore raw

cover More core code ENV_GET_CPU removals

Hi Andreas, Richard and all,

I'm moving towards the goal of having no core code usages of ENV_GET_CPU. This has two advantages:

1: It means we are closer to common-obj'ing core code like exec.c, cpus.c and friends.
2: Multi arch is easier if ENV_GET_CPU stays arch specific. It means I don't need those patches where I reorder the env within the arch specific CPUState. This allows continuing placement of arch specifics before the env in the CPU container (which has TCG perf advantages).

There's a couple more after this pack to get the multi-arch thing going, but due to point 1, I'm sending this ahead as I think it has standalone value.

Regards,
Peter

Peter Crosthwaite (4):
translate-all: Change tb_flush env argument to cpu
gdbserver: _fork: Change fn to accept cpu instead of env
cpus: Change tcg_cpu_exec arg to cpu, not env
cpus: Change exec_init arg to cpu, not env

 bsd-user/main.c             | 2 +-
 cpus.c                      | 7 +++----
 exec.c                      | 7 +++----
 gdbstub.c                   | 9 +++------
 include/exec/exec-all.h     | 4 ++--
 include/exec/gdbstub.h      | 2 +-
 linux-user/main.c           | 2 +-
 linux-user/signal.c         | 2 +-
 target-alpha/cpu.c          | 2 +-
 target-alpha/sys_helper.c   | 2 +-
 target-arm/cpu.c            | 2 +-
 target-cris/cpu.c           | 2 +-
 target-i386/cpu.c           | 2 +-
 target-i386/translate.c     | 2 +-
 target-lm32/cpu.c           | 2 +-
 target-m68k/cpu.c           | 2 +-
 target-microblaze/cpu.c     | 2 +-
 target-mips/cpu.c           | 2 +-
 target-moxie/cpu.c          | 2 +-
 target-openrisc/cpu.c       | 2 +-
 target-ppc/translate_init.c | 2 +-
 target-s390x/cpu.c          | 2 +-
 target-sh4/cpu.c            | 2 +-
 target-sparc/cpu.c          | 2 +-
 target-tricore/cpu.c        | 2 +-
 target-unicore32/cpu.c      | 2 +-
 target-xtensa/cpu.c         | 2 +-
 translate-all.c             | 6 ++----
 28 files changed, 36 insertions(+), 43 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 1/4 translate-all: Change tb_flush env argument to cpu

All of the core-code usages of this API have the cpu pointer handy so pass it in. There are only 3 architecture specific usages (2 of which are commented out) which can just use ENV_GET_CPU locally to get the cpu pointer. The reduces core code usage of the CPU env, which brings us closer to common-obj'ing these core files.

Cc Riku Voipio <[email protected]>
Cc Eduardo Habkost <[email protected]>
Cc Paolo Bonzini <[email protected]>
Signed-off-by Peter Crosthwaite <[email protected]>
---
 exec.c                    | 3 +--
 gdbstub.c                 | 6 ++----
 include/exec/exec-all.h   | 2 +-
 linux-user/signal.c       | 2 +-
 target-alpha/sys_helper.c | 2 +-
 target-i386/translate.c   | 2 +-
 translate-all.c           | 6 ++----
 7 files changed, 9 insertions(+), 14 deletions(-)
exec.c +1 -2
index e19ab22..69bd49e 100644
@@ -762,8 +762,7 @@ void cpu_single_step(CPUState *cpu, int enabled)
         } else {
             /* must flush all the translated code to avoid inconsistencies */
             /* XXX: only flush what is necessary */
-            CPUArchState *env = cpu->env_ptr;
-            tb_flush(env);
+            tb_flush(cpu);
         }
     }
 }
gdbstub.c +2 -4
index 8abcb8a..c1e9321 100644
@@ -1210,7 +1210,6 @@ void gdb_set_stop_cpu(CPUState *cpu)
 static void gdb_vm_state_change(void *opaque, int running, RunState state)
 {
     GDBState *s = gdbserver_state;
-    CPUArchState *env = s->c_cpu->env_ptr;
     CPUState *cpu = s->c_cpu;
     char buf[256];
     const char *type;

@@ -1245,7 +1244,7 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
             cpu->watchpoint_hit = NULL;
             goto send_packet;
         }
-        tb_flush(env);
+        tb_flush(cpu);
         ret = GDB_SIGNAL_TRAP;
         break;
     case RUN_STATE_PAUSED:

@@ -1474,7 +1473,6 @@ gdb_queuesig (void)
 int
 gdb_handlesig(CPUState *cpu, int sig)
 {
-    CPUArchState *env = cpu->env_ptr;
     GDBState *s;
     char buf[256];
     int n;

@@ -1486,7 +1484,7 @@ gdb_handlesig(CPUState *cpu, int sig)
 
     /* disable single step if it was enabled */
     cpu_single_step(cpu, 0);
-    tb_flush(env);
+    tb_flush(cpu);
 
     if (sig != 0) {
         snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig));
include/exec/exec-all.h +1 -1
index b58cd47..304537f 100644
@@ -218,7 +218,7 @@ static inline unsigned int tb_phys_hash_func(tb_page_addr_t pc)
 }
 
 void tb_free(TranslationBlock *tb);
-void tb_flush(CPUArchState *env);
+void tb_flush(CPUState *cpu);
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
 
 #if defined(USE_DIRECT_JUMP)
linux-user/signal.c +1 -1
index 5bb399e..1833e75 100644
@@ -2348,7 +2348,7 @@ static void setup_frame(int sig, struct target_sigaction *ka,
 
 		/* Flush instruction space. */
 		//flush_sig_insns(current->mm, (unsigned long) &(sf->insns[0]));
-                //		tb_flush(env);
+                //		tb_flush(ENV_GET_CPU(env));
 	}
         unlock_user(sf, sf_addr, sizeof(struct target_signal_frame));
 	return;
target-alpha/sys_helper.c +1 -1
index ae2e174..302cc1f 100644
@@ -74,7 +74,7 @@ void helper_tbis(CPUAlphaState *env, uint64_t p)
 
 void helper_tb_flush(CPUAlphaState *env)
 {
-    tb_flush(env);
+    tb_flush(ENV_GET_CPU(env));
 }
 
 void helper_halt(uint64_t restart)
target-i386/translate.c +1 -1
index 723e0cb..920aca4 100644
@@ -6925,7 +6925,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         gen_debug(s, pc_start - s->cs_base);
 #else
         /* start debug */
-        tb_flush(env);
+        tb_flush(ENV_GET_CPU(env));
         qemu_set_log(CPU_LOG_INT | CPU_LOG_TB_IN_ASM);
 #endif
         break;
translate-all.c +2 -4
index 536008f..62042af 100644
@@ -769,10 +769,8 @@ static void page_flush_tb(void)
 
 /* flush all the translation blocks */
 /* XXX: tb_flush is currently not thread safe */
-void tb_flush(CPUArchState *env1)
+void tb_flush(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env1);
-
 #if defined(DEBUG_FLUSH)
     printf("qemu: flush code_size=%ld nb_tbs=%d avg_tb_size=%ld\n",
            (unsigned long)(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer),

@@ -1011,7 +1009,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     tb = tb_alloc(pc);
     if (!tb) {
         /* flush must be done */
-        tb_flush(env);
+        tb_flush(cpu);
         /* cannot fail at this point */
         tb = tb_alloc(pc);
         /* Don't forget to invalidate previous TB info.  */
Signature · 1 line
1.9.1
Reviewed-by Acked-by on v1 1/4
#2 lore raw

Re: [Qemu-devel] [PATCH 1/4] translate-all: Change tb_flush env argument to cpu

Quoted 10 lines · Peter Crosthwaite

All of the core-code usages of this API have the cpu pointer handy so pass it in. There are only 3 architecture specific usages (2 of which are commented out) which can just use ENV_GET_CPU locally to get the cpu pointer. The reduces core code usage of the CPU env, which brings us closer to common-obj'ing these core files.

Cc Riku Voipio <[email protected]>
Cc Eduardo Habkost <[email protected]>
Cc Paolo Bonzini <[email protected]>
Signed-off-by Peter Crosthwaite <[email protected]>
Reviewed-by Eduardo Habkost <[email protected]>
Acked-by Eduardo Habkost <[email protected]>
Signature · 1 line
Eduardo
#3 lore raw

Re: [Qemu-devel] [PATCH 1/4] translate-all: Change tb_flush env argument to cpu

In reply to Peter Crosthwaite

Am 25.05.2015 um 15:22 schrieb Peter Crosthwaite:
[...]

target-alpha/sys_helper.c void helper_tbis(CPUAlphaState *env, uint64_t p) · line 77

Quoted 10 lines
target-alpha/sys_helper.c +1 -1
index ae2e174..302cc1f 100644
@@ -74,7 +74,7 @@ void helper_tbis(CPUAlphaState *env, uint64_t p)
 
 void helper_tb_flush(CPUAlphaState *env)
 {
-    tb_flush(env);
+    tb_flush(ENV_GET_CPU(env));

Please do not use ENV_GET_CPU() in target code, here and below (compare the previous cleanup commits). Otherwise looks great.

Regards,
Andreas

Quoted 16 lines
target-alpha/sys_helper.c +0 -0
 }
 
 void helper_halt(uint64_t restart)
target-i386/translate.c +1 -1
index 723e0cb..920aca4 100644
@@ -6925,7 +6925,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         gen_debug(s, pc_start - s->cs_base);
 #else
         /* start debug */
-        tb_flush(env);
+        tb_flush(ENV_GET_CPU(env));
         qemu_set_log(CPU_LOG_INT | CPU_LOG_TB_IN_ASM);
 #endif
         break;

[snip]

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
author
#4 lore raw

PATCH 2/4 gdbserver: _fork: Change fn to accept cpu instead of env

In reply to Peter Crosthwaite

All callsites to this function navigate the cpu->env_ptr only for the function to take the env ptr back to the original cpu ptr. Change the function to just pass in the CPU pointer instead. Removes a core code usage of ENV_GET_CPU (in gdbstub.c).

Cc Riku Voipio <[email protected]>
Signed-off-by Peter Crosthwaite <[email protected]>
---
 bsd-user/main.c        | 2 +-
 gdbstub.c              | 3 +--
 include/exec/gdbstub.h | 2 +-
 linux-user/main.c      | 2 +-
 4 files changed, 4 insertions(+), 5 deletions(-)
bsd-user/main.c +1 -1
index 5bfaf5c..1800b02 100644
@@ -92,7 +92,7 @@ void fork_start(void)
 void fork_end(int child)
 {
     if (child) {
-        gdbserver_fork((CPUArchState *)thread_cpu->env_ptr);
+        gdbserver_fork(thread_cpu);
     }
 }
gdbstub.c +1 -2
index c1e9321..d37943a 100644
@@ -1613,9 +1613,8 @@ int gdbserver_start(int port)
 }
 
 /* Disable gdb stub for child processes.  */
-void gdbserver_fork(CPUArchState *env)
+void gdbserver_fork(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env);
     GDBState *s = gdbserver_state;
 
     if (gdbserver_fd < 0 || s->fd < 0) {
include/exec/gdbstub.h +1 -1
index c633248..ab7ac33 100644
@@ -22,7 +22,7 @@ void gdb_exit(CPUArchState *, int);
 int gdb_queuesig (void);
 int gdb_handlesig(CPUState *, int);
 void gdb_signalled(CPUArchState *, int);
-void gdbserver_fork(CPUArchState *);
+void gdbserver_fork(CPUState *);
 #endif
 /* Get or set a register.  Returns the size of the register.  */
 typedef int (*gdb_reg_cb)(CPUArchState *env, uint8_t *buf, int reg);
linux-user/main.c +1 -1
index 3f32db0..48eff88 100644
@@ -130,7 +130,7 @@ void fork_end(int child)
         pthread_cond_init(&exclusive_cond, NULL);
         pthread_cond_init(&exclusive_resume, NULL);
         pthread_mutex_init(&tcg_ctx.tb_ctx.tb_lock, NULL);
-        gdbserver_fork((CPUArchState *)thread_cpu->env_ptr);
+        gdbserver_fork(thread_cpu);
     } else {
         pthread_mutex_unlock(&exclusive_lock);
         pthread_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
Signature · 1 line
1.9.1
Reviewed-by on v1 2/4
#5 lore raw

Re: [Qemu-devel] [PATCH 2/4] gdbserver: _fork: Change fn to accept cpu instead of env

Am 25.05.2015 um 15:22 schrieb Peter Crosthwaite:

Quoted 5 lines

All callsites to this function navigate the cpu->env_ptr only for the function to take the env ptr back to the original cpu ptr. Change the function to just pass in the CPU pointer instead. Removes a core code usage of ENV_GET_CPU (in gdbstub.c).

Quoted 8 lines
Cc Riku Voipio <[email protected]>
Signed-off-by Peter Crosthwaite <[email protected]>
---
 bsd-user/main.c        | 2 +-
 gdbstub.c              | 3 +--
 include/exec/gdbstub.h | 2 +-
 linux-user/main.c      | 2 +-
 4 files changed, 4 insertions(+), 5 deletions(-)

Great,

Reviewed-by Andreas Färber <[email protected]>

Thanks,
Andreas

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
author
#6 lore raw

PATCH 3/4 cpus: Change tcg_cpu_exec arg to cpu, not env

In reply to Peter Crosthwaite

The sole caller of this function navigates the cpu->env_ptr only for this function to take it back the cpu pointer straight away. Pass in cpu pointer instead and grab the env pointer locally in the function. Removes a core code usage of ENV_GET_CPU.

Signed-off-by Peter Crosthwaite <[email protected]>
---
 cpus.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
cpus.c +3 -4
index de6469f..1b8f05a 100644
@@ -1314,9 +1314,9 @@ int vm_stop_force_state(RunState state)
     }
 }
 
-static int tcg_cpu_exec(CPUArchState *env)
+static int tcg_cpu_exec(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env);
+    CPUArchState *env = cpu->env_ptr;
     int ret;
 #ifdef CONFIG_PROFILER
     int64_t ti;

@@ -1378,13 +1378,12 @@ static void tcg_exec_all(void)
     }
     for (; next_cpu != NULL && !exit_request; next_cpu = CPU_NEXT(next_cpu)) {
         CPUState *cpu = next_cpu;
-        CPUArchState *env = cpu->env_ptr;
 
         qemu_clock_enable(QEMU_CLOCK_VIRTUAL,
                           (cpu->singlestep_enabled & SSTEP_NOTIMER) == 0);
 
         if (cpu_can_run(cpu)) {
-            r = tcg_cpu_exec(env);
+            r = tcg_cpu_exec(cpu);
             if (r == EXCP_DEBUG) {
                 cpu_handle_guest_debug(cpu);
                 break;
Signature · 1 line
1.9.1
Reviewed-by on v1 3/4
#7 lore raw

Re: [Qemu-devel] [PATCH 3/4] cpus: Change tcg_cpu_exec arg to cpu, not env

Am 25.05.2015 um 15:22 schrieb Peter Crosthwaite:

Quoted 4 lines

The sole caller of this function navigates the cpu->env_ptr only for this function to take it back the cpu pointer straight away. Pass in cpu pointer instead and grab the env pointer locally in the function. Removes a core code usage of ENV_GET_CPU.

Please follow the convention of writing "foo()" for any function or parameterized macro, here and elsewhere.

Quoted 5 lines
Signed-off-by Peter Crosthwaite <[email protected]>
---
 cpus.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by Andreas Färber <[email protected]>

Thanks,
Andreas

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
author
#8 lore raw

PATCH 4/4 cpus: Change exec_init arg to cpu, not env

In reply to Peter Crosthwaite

The callers (most of them in target-foo/cpu.c) to this function all have the cpu pointer handy. Just pass it to avoid an ENV_GET_CPU from core code (in exec.c).

Cc Paolo Bonzini <[email protected]>
Cc Richard Henderson <[email protected]>
Cc Peter Maydell <[email protected]>
Cc "Edgar E. Iglesias" <[email protected]>
Cc "Andreas Färber" <[email protected]>
Cc Eduardo Habkost <[email protected]>
Cc Michael Walle <[email protected]>
Cc Aurelien Jarno <[email protected]>
Cc Leon Alrae <[email protected]>
Cc Anthony Green <[email protected]>
Cc Jia Liu <[email protected]>
Cc Alexander Graf <[email protected]>
Cc Blue Swirl <[email protected]>
Cc Mark Cave-Ayland <[email protected]>
Cc Bastian Koppelmann <[email protected]>
Cc Guan Xuetao <[email protected]>
Cc Max Filippov <[email protected]>
Signed-off-by Peter Crosthwaite <[email protected]>
---
 exec.c                      | 4 ++--
 include/exec/exec-all.h     | 2 +-
 target-alpha/cpu.c          | 2 +-
 target-arm/cpu.c            | 2 +-
 target-cris/cpu.c           | 2 +-
 target-i386/cpu.c           | 2 +-
 target-lm32/cpu.c           | 2 +-
 target-m68k/cpu.c           | 2 +-
 target-microblaze/cpu.c     | 2 +-
 target-mips/cpu.c           | 2 +-
 target-moxie/cpu.c          | 2 +-
 target-openrisc/cpu.c       | 2 +-
 target-ppc/translate_init.c | 2 +-
 target-s390x/cpu.c          | 2 +-
 target-sh4/cpu.c            | 2 +-
 target-sparc/cpu.c          | 2 +-
 target-tricore/cpu.c        | 2 +-
 target-unicore32/cpu.c      | 2 +-
 target-xtensa/cpu.c         | 2 +-
 19 files changed, 20 insertions(+), 20 deletions(-)
exec.c +2 -2
index 69bd49e..0cb54f7 100644
@@ -518,9 +518,8 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
 }
 #endif
 
-void cpu_exec_init(CPUArchState *env)
+void cpu_exec_init(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env);
     CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUState *some_cpu;
     int cpu_index;

@@ -549,6 +548,7 @@ void cpu_exec_init(CPUArchState *env)
         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
     }
 #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
+    CPUArchState *env = cpu->env_ptr;
     register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
                     cpu_save, cpu_load, env);
     assert(cc->vmsd == NULL);
include/exec/exec-all.h +1 -1
index 304537f..078f517 100644
@@ -88,7 +88,7 @@ void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
 TranslationBlock *tb_gen_code(CPUState *cpu,
                               target_ulong pc, target_ulong cs_base, int flags,
                               int cflags);
-void cpu_exec_init(CPUArchState *env);
+void cpu_exec_init(CPUState *env);
 void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
 int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
 void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
target-alpha/cpu.c +1 -1
index a98b7d8..9ffef3c 100644
@@ -257,7 +257,7 @@ static void alpha_cpu_initfn(Object *obj)
     CPUAlphaState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
     tlb_flush(cs, 1);
 
     alpha_translate_init();
target-arm/cpu.c +1 -1
index e39586b..f135529 100644
@@ -402,7 +402,7 @@ static void arm_cpu_initfn(Object *obj)
     static bool inited;
 
     cs->env_ptr = &cpu->env;
-    cpu_exec_init(&cpu->env);
+    cpu_exec_init(cs);
     cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
                                          g_free, g_free);
target-cris/cpu.c +1 -1
index d555ea0..50fbc9e 100644
@@ -184,7 +184,7 @@ static void cris_cpu_initfn(Object *obj)
     static bool tcg_initialized;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     env->pregs[PR_VR] = ccc->vr;
target-i386/cpu.c +1 -1
index 3305e09..14e6546 100644
@@ -2850,7 +2850,7 @@ static void x86_cpu_initfn(Object *obj)
     static int inited;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     object_property_add(obj, "family", "int",
                         x86_cpuid_version_get_family,
target-lm32/cpu.c +1 -1
index f8081f5..44cb4a7 100644
@@ -151,7 +151,7 @@ static void lm32_cpu_initfn(Object *obj)
     static bool tcg_initialized;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     env->flags = 0;
target-m68k/cpu.c +1 -1
index 4cfb725..a33aa8a 100644
@@ -168,7 +168,7 @@ static void m68k_cpu_initfn(Object *obj)
     static bool inited;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     if (tcg_enabled() && !inited) {
         inited = true;
target-microblaze/cpu.c +1 -1
index 89b8363..563ad46 100644
@@ -136,7 +136,7 @@ static void mb_cpu_initfn(Object *obj)
     static bool tcg_initialized;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     set_float_rounding_mode(float_round_nearest_even, &env->fp_status);
target-mips/cpu.c +1 -1
index 958c999..2e29bd7 100644
@@ -115,7 +115,7 @@ static void mips_cpu_initfn(Object *obj)
     CPUMIPSState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     if (tcg_enabled()) {
         mips_tcg_init();
target-moxie/cpu.c +1 -1
index 47b617f..de6e21a 100644
@@ -66,7 +66,7 @@ static void moxie_cpu_initfn(Object *obj)
     static int inited;
 
     cs->env_ptr = &cpu->env;
-    cpu_exec_init(&cpu->env);
+    cpu_exec_init(cs);
 
     if (tcg_enabled() && !inited) {
         inited = 1;
target-openrisc/cpu.c +1 -1
index 39bedc1..aa0e333 100644
@@ -92,7 +92,7 @@ static void openrisc_cpu_initfn(Object *obj)
     static int inited;
 
     cs->env_ptr = &cpu->env;
-    cpu_exec_init(&cpu->env);
+    cpu_exec_init(cs);
 
 #ifndef CONFIG_USER_ONLY
     cpu_openrisc_mmu_init(cpu);
target-ppc/translate_init.c +1 -1
index d74f4f0..e519cc0 100644
@@ -9633,7 +9633,7 @@ static void ppc_cpu_initfn(Object *obj)
     CPUPPCState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
     cpu->cpu_dt_id = cs->cpu_index;
 
     env->msr_mask = pcc->msr_mask;
target-s390x/cpu.c +1 -1
index d2f9836..856a684 100644
@@ -187,7 +187,7 @@ static void s390_cpu_initfn(Object *obj)
 #endif
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
     qemu_get_timedate(&tm, 0);
target-sh4/cpu.c +1 -1
index d187a2b..f781e18 100644
@@ -247,7 +247,7 @@ static void superh_cpu_initfn(Object *obj)
     CPUSH4State *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     env->movcal_backup_tail = &(env->movcal_backup);
target-sparc/cpu.c +1 -1
index a952097..ebf5af7 100644
@@ -802,7 +802,7 @@ static void sparc_cpu_initfn(Object *obj)
     CPUSPARCState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     if (tcg_enabled()) {
         gen_intermediate_code_init(env);
target-tricore/cpu.c +1 -1
index b3e5512..425525f 100644
@@ -92,7 +92,7 @@ static void tricore_cpu_initfn(Object *obj)
     CPUTriCoreState *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     if (tcg_enabled()) {
         tricore_tcg_init();
target-unicore32/cpu.c +1 -1
index 5b32987..76d680c 100644
@@ -111,7 +111,7 @@ static void uc32_cpu_initfn(Object *obj)
     static bool inited;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
 #ifdef CONFIG_USER_ONLY
     env->uncached_asr = ASR_MODE_USER;
target-xtensa/cpu.c +1 -1
index 2b75678..83a86d3 100644
@@ -114,7 +114,7 @@ static void xtensa_cpu_initfn(Object *obj)
 
     cs->env_ptr = env;
     env->config = xcc->config;
-    cpu_exec_init(env);
+    cpu_exec_init(cs);
 
     if (tcg_enabled() && !tcg_inited) {
         tcg_inited = true;
Signature · 1 line
1.9.1
#9 lore raw

Re: [Qemu-devel] [PATCH 4/4] cpus: Change exec_init arg to cpu, not env

Quoted 14 lines · Peter Crosthwaite

The callers (most of them in target-foo/cpu.c) to this function all have the cpu pointer handy. Just pass it to avoid an ENV_GET_CPU from core code (in exec.c).

Cc Paolo Bonzini <[email protected]>
Cc Richard Henderson <[email protected]>
Cc Peter Maydell <[email protected]>
Cc "Edgar E. Iglesias" <[email protected]>
Cc "Andreas Färber" <[email protected]>
Cc Eduardo Habkost <[email protected]>
Cc Michael Walle <[email protected]>
Cc Aurelien Jarno <[email protected]>
Cc Leon Alrae <[email protected]>
Cc Anthony Green <[email protected]>
Quoted 8 lines
Cc Jia Liu <[email protected]>
Cc Alexander Graf <[email protected]>
Cc Blue Swirl <[email protected]>
Cc Mark Cave-Ayland <[email protected]>
Cc Bastian Koppelmann <[email protected]>
Cc Guan Xuetao <[email protected]>
Cc Max Filippov <[email protected]>
Signed-off-by Peter Crosthwaite <[email protected]>

This conflicts with this series from Bharata B Rao:
Subject: [PATCH v3 0/3] Bitmap based CPU enumeration

Can you rebase on top of that series?

Signature · 1 line
Eduardo
Reviewed-by on v1 4/4
#10 lore raw

Re: [Qemu-devel] [PATCH 4/4] cpus: Change exec_init arg to cpu, not env

In reply to Peter Crosthwaite

Am 25.05.2015 um 15:22 schrieb Peter Crosthwaite:

Quoted 35 lines

The callers (most of them in target-foo/cpu.c) to this function all have the cpu pointer handy. Just pass it to avoid an ENV_GET_CPU from core code (in exec.c).

Cc Paolo Bonzini <[email protected]>
Cc Richard Henderson <[email protected]>
Cc Peter Maydell <[email protected]>
Cc "Edgar E. Iglesias" <[email protected]>
Cc "Andreas Färber" <[email protected]>
Cc Eduardo Habkost <[email protected]>
Cc Michael Walle <[email protected]>
Cc Aurelien Jarno <[email protected]>
Cc Leon Alrae <[email protected]>
Cc Anthony Green <[email protected]>
Cc Jia Liu <[email protected]>
Cc Alexander Graf <[email protected]>
Cc Blue Swirl <[email protected]>
Cc Mark Cave-Ayland <[email protected]>
Cc Bastian Koppelmann <[email protected]>
Cc Guan Xuetao <[email protected]>
Cc Max Filippov <[email protected]>
Signed-off-by Peter Crosthwaite <[email protected]>
---
 exec.c                      | 4 ++--
 include/exec/exec-all.h     | 2 +-
 target-alpha/cpu.c          | 2 +-
 target-arm/cpu.c            | 2 +-
 target-cris/cpu.c           | 2 +-
 target-i386/cpu.c           | 2 +-
 target-lm32/cpu.c           | 2 +-
 target-m68k/cpu.c           | 2 +-
 target-microblaze/cpu.c     | 2 +-
 target-mips/cpu.c           | 2 +-
 target-moxie/cpu.c          | 2 +-
 target-openrisc/cpu.c       | 2 +-
Quoted 8 lines
 target-ppc/translate_init.c | 2 +-
 target-s390x/cpu.c          | 2 +-
 target-sh4/cpu.c            | 2 +-
 target-sparc/cpu.c          | 2 +-
 target-tricore/cpu.c        | 2 +-
 target-unicore32/cpu.c      | 2 +-
 target-xtensa/cpu.c         | 2 +-
 19 files changed, 20 insertions(+), 20 deletions(-)
Reviewed-by Andreas Färber <[email protected]>

I assume this will conflict with another patch adding an Error** argument - Eduardo, please help coordinating this.

Thanks,
Andreas

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
#11 lore raw

Re: [Qemu-devel] [PATCH 4/4] cpus: Change exec_init arg to cpu, not env

Quoted 4 lines · Andreas Färber

Am 25.05.2015 um 15:22 schrieb Peter Crosthwaite:

Quoted 3 lines

The callers (most of them in target-foo/cpu.c) to this function all have the cpu pointer handy. Just pass it to avoid an ENV_GET_CPU from core code (in exec.c).

[...]

Quoted 4 lines
Reviewed-by Andreas Färber <[email protected]>

I assume this will conflict with another patch adding an Error** argument - Eduardo, please help coordinating this.

I asked Peter to rebase on top of Bharata's "Bitmap based CPU enumeration" series (that's already on v3 and got some Reviewed-by lines), but both Peter and Bharata need to know which tree those series are expected to be included.

Signature · 1 line
Eduardo
#12 lore raw

Re: [Qemu-devel] [PATCH 4/4] cpus: Change exec_init arg to cpu, not env

Am 05.06.2015 um 17:05 schrieb Eduardo Habkost:

Quoted 8 lines · Andreas Färber
Quoted 4 lines

Am 25.05.2015 um 15:22 schrieb Peter Crosthwaite:

Quoted 3 lines

The callers (most of them in target-foo/cpu.c) to this function all have the cpu pointer handy. Just pass it to avoid an ENV_GET_CPU from core code (in exec.c).

[...]

Quoted 2 lines
Reviewed-by Andreas Färber <[email protected]>
Quoted 7 lines
Quoted 2 lines

I assume this will conflict with another patch adding an Error** argument - Eduardo, please help coordinating this.

I asked Peter to rebase on top of Bharata's "Bitmap based CPU enumeration" series (that's already on v3 and got some Reviewed-by lines), but both Peter and Bharata need to know which tree those series are expected to be included.

I was expecting you to handle cpu_exec_init() through your x86 tree, but seems that didn't happen. I am going through CPU patches today, so could take open series but then would need to pick up the ENV_* stuff too, which I thought would go through Paolo in my absence...

Regards,
Andreas

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
#13 lore raw

Re: [Qemu-devel] [PATCH 4/4] cpus: Change exec_init arg to cpu, not env

Quoted 4 lines · Andreas Färber

I was expecting you to handle cpu_exec_init() through your x86 tree, but seems that didn't happen. I am going through CPU patches today, so could take open series but then would need to pick up the ENV_* stuff too, which I thought would go through Paolo in my absence...

I'm sorry for the misunderstanding. I myself left the series behind waiting for Bharata's series to get in, without really trying to see what depended on it and what didn't.

Paolo

#14 lore raw
In reply to Peter Crosthwaite
Quoted 45 lines · Peter Crosthwaite

Hi Andreas, Richard and all,

I'm moving towards the goal of having no core code usages of ENV_GET_CPU. This has two advantages:

1: It means we are closer to common-obj'ing core code like exec.c, cpus.c and friends.
2: Multi arch is easier if ENV_GET_CPU stays arch specific. It means I don't need those patches where I reorder the env within the arch specific CPUState. This allows continuing placement of arch specifics before the env in the CPU container (which has TCG perf advantages).

There's a couple more after this pack to get the multi-arch thing going, but due to point 1, I'm sending this ahead as I think it has standalone value.

Regards,
Peter

Peter Crosthwaite (4):
translate-all: Change tb_flush env argument to cpu
gdbserver: _fork: Change fn to accept cpu instead of env
cpus: Change tcg_cpu_exec arg to cpu, not env
cpus: Change exec_init arg to cpu, not env

 bsd-user/main.c             | 2 +-
 cpus.c                      | 7 +++----
 exec.c                      | 7 +++----
 gdbstub.c                   | 9 +++------
 include/exec/exec-all.h     | 4 ++--
 include/exec/gdbstub.h      | 2 +-
 linux-user/main.c           | 2 +-
 linux-user/signal.c         | 2 +-
 target-alpha/cpu.c          | 2 +-
 target-alpha/sys_helper.c   | 2 +-
 target-arm/cpu.c            | 2 +-
 target-cris/cpu.c           | 2 +-
 target-i386/cpu.c           | 2 +-
 target-i386/translate.c     | 2 +-
 target-lm32/cpu.c           | 2 +-
 target-m68k/cpu.c           | 2 +-
 target-microblaze/cpu.c     | 2 +-
 target-mips/cpu.c           | 2 +-
 target-moxie/cpu.c          | 2 +-
 target-openrisc/cpu.c       | 2 +-
 target-ppc/translate_init.c | 2 +-
Quoted 9 lines
 target-s390x/cpu.c          | 2 +-
 target-sh4/cpu.c            | 2 +-
 target-sparc/cpu.c          | 2 +-
 target-tricore/cpu.c        | 2 +-
 target-unicore32/cpu.c      | 2 +-
 target-xtensa/cpu.c         | 2 +-
 translate-all.c             | 6 ++----
 28 files changed, 36 insertions(+), 43 deletions(-)

Thanks, queued for 2.4.

Paolo

#15 lore raw
Quoted 58 lines · Paolo Bonzini
Quoted 45 lines · Peter Crosthwaite

Hi Andreas, Richard and all,

I'm moving towards the goal of having no core code usages of ENV_GET_CPU. This has two advantages:

1: It means we are closer to common-obj'ing core code like exec.c, cpus.c and friends.
2: Multi arch is easier if ENV_GET_CPU stays arch specific. It means I don't need those patches where I reorder the env within the arch specific CPUState. This allows continuing placement of arch specifics before the env in the CPU container (which has TCG perf advantages).

There's a couple more after this pack to get the multi-arch thing going, but due to point 1, I'm sending this ahead as I think it has standalone value.

Regards,
Peter

Peter Crosthwaite (4):
translate-all: Change tb_flush env argument to cpu
gdbserver: _fork: Change fn to accept cpu instead of env
cpus: Change tcg_cpu_exec arg to cpu, not env
cpus: Change exec_init arg to cpu, not env

 bsd-user/main.c             | 2 +-
 cpus.c                      | 7 +++----
 exec.c                      | 7 +++----
 gdbstub.c                   | 9 +++------
 include/exec/exec-all.h     | 4 ++--
 include/exec/gdbstub.h      | 2 +-
 linux-user/main.c           | 2 +-
 linux-user/signal.c         | 2 +-
 target-alpha/cpu.c          | 2 +-
 target-alpha/sys_helper.c   | 2 +-
 target-arm/cpu.c            | 2 +-
 target-cris/cpu.c           | 2 +-
 target-i386/cpu.c           | 2 +-
 target-i386/translate.c     | 2 +-
 target-lm32/cpu.c           | 2 +-
 target-m68k/cpu.c           | 2 +-
 target-microblaze/cpu.c     | 2 +-
 target-mips/cpu.c           | 2 +-
 target-moxie/cpu.c          | 2 +-
 target-openrisc/cpu.c       | 2 +-
 target-ppc/translate_init.c | 2 +-
Quoted 9 lines
 target-s390x/cpu.c          | 2 +-
 target-sh4/cpu.c            | 2 +-
 target-sparc/cpu.c          | 2 +-
 target-tricore/cpu.c        | 2 +-
 target-unicore32/cpu.c      | 2 +-
 target-xtensa/cpu.c         | 2 +-
 translate-all.c             | 6 ++----
 28 files changed, 36 insertions(+), 43 deletions(-)
Quoted 2 lines

Thanks, queued for 2.4.

Eduardo flagged a conflict with on-list work. Do you want me to handle it?

Regards,
Peter

Quoted 2 lines

Paolo

#16 lore raw
Quoted 1 line · Peter Crosthwaite

Eduardo flagged a conflict with on-list work. Do you want me to handle it?

I don't know, but I'll dequeue these patches.

Paolo

#17 lore raw
Quoted 7 lines · Paolo Bonzini
Quoted 1 line · Peter Crosthwaite

Eduardo flagged a conflict with on-list work. Do you want me to handle it?

I don't know, but I'll dequeue these patches.

Alright,

So I'll wait for the enqueue of Bharata's patches as they look very close. Who is going to merge them and should I target the same queue?

Regards,
Peter

Quoted 2 lines

Paolo

#18 lore raw
In reply to Paolo Bonzini

Am 25.05.2015 um 15:08 schrieb Paolo Bonzini:

Quoted 24 lines · Peter Crosthwaite
Quoted 23 lines

Hi Andreas, Richard and all,

I'm moving towards the goal of having no core code usages of ENV_GET_CPU. This has two advantages:

1: It means we are closer to common-obj'ing core code like exec.c, cpus.c and friends.
2: Multi arch is easier if ENV_GET_CPU stays arch specific. It means I don't need those patches where I reorder the env within the arch specific CPUState. This allows continuing placement of arch specifics before the env in the CPU container (which has TCG perf advantages).

There's a couple more after this pack to get the multi-arch thing going, but due to point 1, I'm sending this ahead as I think it has standalone value.

Regards,
Peter

Peter Crosthwaite (4):
translate-all: Change tb_flush env argument to cpu
gdbserver: _fork: Change fn to accept cpu instead of env
cpus: Change tcg_cpu_exec arg to cpu, not env
cpus: Change exec_init arg to cpu, not env

[...]

Quoted 2 lines

Thanks, queued for 2.4.

Apparently after qom-next you also want to take over qom-cpu, once again without pinging me first. Then make it official in MAINTAINERS and really review the patches line by line rather than just queuing them somewhere (again you didn't bother to state where). It can't go on like this and I'll be on vacation soon anyway.

Do you want to take over PReP too while you're at it?

Andreas

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
#19 lore raw
Quoted 25 lines · Andreas Färber

Am 25.05.2015 um 15:08 schrieb Paolo Bonzini:

Quoted 16 lines · Peter Crosthwaite
Quoted 15 lines

Hi Andreas, Richard and all,

I'm moving towards the goal of having no core code usages of ENV_GET_CPU. This has two advantages:

1: It means we are closer to common-obj'ing core code like exec.c, cpus.c and friends.
2: Multi arch is easier if ENV_GET_CPU stays arch specific. It means I don't need those patches where I reorder the env within the arch specific CPUState. This allows continuing placement of arch specifics before the env in the CPU container (which has TCG perf advantages).

There's a couple more after this pack to get the multi-arch thing going, but due to point 1, I'm sending this ahead as I think it has standalone value.

Quoted 8 lines
Quoted 8 lines

Regards,
Peter

Peter Crosthwaite (4):
translate-all: Change tb_flush env argument to cpu
gdbserver: _fork: Change fn to accept cpu instead of env
cpus: Change tcg_cpu_exec arg to cpu, not env
cpus: Change exec_init arg to cpu, not env

Quoted 6 lines

[...]

Quoted 2 lines

Thanks, queued for 2.4.

Apparently after qom-next you also want to take over qom-cpu, once again without pinging me first.

Uhm...

Main loop
M: Paolo Bonzini <[email protected]>
S: Maintained
F: cpus.c
F: main-loop.c
F: qemu-timer.c
F: vl.c

translate-all.c is "Odd fixes" with no specific maintainer, and gdbserver.c is not in MAINTAINERS altogether.

Paolo

#20 lore raw

Am 26.05.2015 um 10:05 schrieb Paolo Bonzini:

Quoted 43 lines · Andreas Färber
Quoted 25 lines

Am 25.05.2015 um 15:08 schrieb Paolo Bonzini:

Quoted 16 lines · Peter Crosthwaite
Quoted 15 lines

Hi Andreas, Richard and all,

I'm moving towards the goal of having no core code usages of ENV_GET_CPU. This has two advantages:

1: It means we are closer to common-obj'ing core code like exec.c, cpus.c and friends.
2: Multi arch is easier if ENV_GET_CPU stays arch specific. It means I don't need those patches where I reorder the env within the arch specific CPUState. This allows continuing placement of arch specifics before the env in the CPU container (which has TCG perf advantages).

There's a couple more after this pack to get the multi-arch thing going, but due to point 1, I'm sending this ahead as I think it has standalone value.

Quoted 8 lines
Quoted 8 lines

Regards,
Peter

Peter Crosthwaite (4):
translate-all: Change tb_flush env argument to cpu
gdbserver: _fork: Change fn to accept cpu instead of env
cpus: Change tcg_cpu_exec arg to cpu, not env
cpus: Change exec_init arg to cpu, not env

Quoted 6 lines

[...]

Quoted 2 lines

Thanks, queued for 2.4.

Apparently after qom-next you also want to take over qom-cpu, once again without pinging me first.

Uhm...

Main loop
M: Paolo Bonzini <[email protected]>
S: Maintained
F: cpus.c
F: main-loop.c
F: qemu-timer.c
F: vl.c

Quoted 2 lines

translate-all.c is "Odd fixes" with no specific maintainer, and gdbserver.c is not in MAINTAINERS altogether.

ENV_GET_CPU() is my QOM CPU macro. You picked the patchset up just hours after it arrived on the list, on a holiday, without giving me a chance to review. It's not about which tree it goes through, it's about you not asking first - which I reminded you of just days ago, so this appears deliberate.

And as you can see above, the cover letter was addressed to Richard and me, so we should get a chance to respond a) with change requests and b) with Reviewed-bys or Acked-bys that actually get put on the patch and not repeatedly ignored like on your last pull.

Andreas

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
#21 lore raw
Quoted 44 lines · Andreas Färber

Am 26.05.2015 um 10:05 schrieb Paolo Bonzini:

Quoted 43 lines · Andreas Färber
Quoted 25 lines

Am 25.05.2015 um 15:08 schrieb Paolo Bonzini:

Quoted 16 lines · Peter Crosthwaite
Quoted 15 lines

Hi Andreas, Richard and all,

I'm moving towards the goal of having no core code usages of ENV_GET_CPU. This has two advantages:

1: It means we are closer to common-obj'ing core code like exec.c, cpus.c and friends.
2: Multi arch is easier if ENV_GET_CPU stays arch specific. It means I don't need those patches where I reorder the env within the arch specific CPUState. This allows continuing placement of arch specifics before the env in the CPU container (which has TCG perf advantages).

There's a couple more after this pack to get the multi-arch thing going, but due to point 1, I'm sending this ahead as I think it has standalone value.

Quoted 8 lines
Quoted 8 lines

Regards,
Peter

Peter Crosthwaite (4):
translate-all: Change tb_flush env argument to cpu
gdbserver: _fork: Change fn to accept cpu instead of env
cpus: Change tcg_cpu_exec arg to cpu, not env
cpus: Change exec_init arg to cpu, not env

Quoted 6 lines

[...]

Quoted 2 lines

Thanks, queued for 2.4.

Apparently after qom-next you also want to take over qom-cpu, once again without pinging me first.

Uhm...

Main loop
M: Paolo Bonzini <[email protected]>
S: Maintained
F: cpus.c
F: main-loop.c
F: qemu-timer.c
F: vl.c

Quoted 4 lines
Quoted 2 lines

translate-all.c is "Odd fixes" with no specific maintainer, and gdbserver.c is not in MAINTAINERS altogether.

ENV_GET_CPU() is my QOM CPU macro.

Please, this is ridiculous. MAINTAINERS talks about files, not about macros. If somebody misuses the memory API and I'm on vacation (I know you're not, this is an example), I fix it myself, I don't complain with whomever applied the patches.

Quoted 5 lines

You picked the patchset up just hours
after it arrived on the list, on a holiday, without giving me a chance to review. It's not about which tree it goes through, it's about you not asking first - which I reminded you of just days ago, so this appears deliberate.

It certainly is.

Paolo

#22 lore raw

Am 26.05.2015 um 10:25 schrieb Paolo Bonzini:

Quoted 57 lines · Andreas Färber
Quoted 44 lines

Am 26.05.2015 um 10:05 schrieb Paolo Bonzini:

Quoted 43 lines · Andreas Färber
Quoted 25 lines

Am 25.05.2015 um 15:08 schrieb Paolo Bonzini:

Quoted 16 lines · Peter Crosthwaite
Quoted 15 lines

Hi Andreas, Richard and all,

I'm moving towards the goal of having no core code usages of ENV_GET_CPU. This has two advantages:

1: It means we are closer to common-obj'ing core code like exec.c, cpus.c and friends.
2: Multi arch is easier if ENV_GET_CPU stays arch specific. It means I don't need those patches where I reorder the env within the arch specific CPUState. This allows continuing placement of arch specifics before the env in the CPU container (which has TCG perf advantages).

There's a couple more after this pack to get the multi-arch thing going, but due to point 1, I'm sending this ahead as I think it has standalone value.

Quoted 8 lines
Quoted 8 lines

Regards,
Peter

Peter Crosthwaite (4):
translate-all: Change tb_flush env argument to cpu
gdbserver: _fork: Change fn to accept cpu instead of env
cpus: Change tcg_cpu_exec arg to cpu, not env
cpus: Change exec_init arg to cpu, not env

Quoted 6 lines

[...]

Quoted 2 lines

Thanks, queued for 2.4.

Apparently after qom-next you also want to take over qom-cpu, once again without pinging me first.

Uhm...

Main loop
M: Paolo Bonzini <[email protected]>
S: Maintained
F: cpus.c
F: main-loop.c
F: qemu-timer.c
F: vl.c

Quoted 4 lines
Quoted 2 lines

translate-all.c is "Odd fixes" with no specific maintainer, and gdbserver.c is not in MAINTAINERS altogether.

ENV_GET_CPU() is my QOM CPU macro.

Please, this is ridiculous. MAINTAINERS talks about files, not about macros. If somebody misuses the memory API and I'm on vacation (I know you're not, this is an example), I fix it myself, I don't complain with whomever applied the patches.

Quoted 7 lines
Quoted 5 lines

You picked the patchset up just hours
after it arrived on the list, on a holiday, without giving me a chance to review. It's not about which tree it goes through, it's about you not asking first - which I reminded you of just days ago, so this appears deliberate.

It certainly is.

Then you can fix up Daniel's patch yourself and I am stepping down as maintainer. Have fun.

Andreas

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
#23 lore raw
Quoted 64 lines · Andreas Färber

Am 26.05.2015 um 10:25 schrieb Paolo Bonzini:

Quoted 63 lines · Andreas Färber
Quoted 44 lines

Am 26.05.2015 um 10:05 schrieb Paolo Bonzini:

Quoted 43 lines · Andreas Färber
Quoted 25 lines

Am 25.05.2015 um 15:08 schrieb Paolo Bonzini:

Quoted 16 lines · Peter Crosthwaite
Quoted 15 lines

Hi Andreas, Richard and all,

I'm moving towards the goal of having no core code usages of ENV_GET_CPU. This has two advantages:

1: It means we are closer to common-obj'ing core code like exec.c, cpus.c and friends.
2: Multi arch is easier if ENV_GET_CPU stays arch specific. It means I don't need those patches where I reorder the env within the arch specific CPUState. This allows continuing placement of arch specifics before the env in the CPU container (which has TCG perf advantages).

There's a couple more after this pack to get the multi-arch thing going, but due to point 1, I'm sending this ahead as I think it has standalone value.

Quoted 8 lines
Quoted 8 lines

Regards,
Peter

Peter Crosthwaite (4):
translate-all: Change tb_flush env argument to cpu
gdbserver: _fork: Change fn to accept cpu instead of env
cpus: Change tcg_cpu_exec arg to cpu, not env
cpus: Change exec_init arg to cpu, not env

Quoted 6 lines

[...]

Quoted 2 lines

Thanks, queued for 2.4.

Apparently after qom-next you also want to take over qom-cpu, once again without pinging me first.

Uhm...

Main loop
M: Paolo Bonzini <[email protected]>
S: Maintained
F: cpus.c
F: main-loop.c
F: qemu-timer.c
F: vl.c

Quoted 4 lines
Quoted 2 lines

translate-all.c is "Odd fixes" with no specific maintainer, and gdbserver.c is not in MAINTAINERS altogether.

ENV_GET_CPU() is my QOM CPU macro.

Please, this is ridiculous. MAINTAINERS talks about files, not about macros. If somebody misuses the memory API and I'm on vacation (I know you're not, this is an example), I fix it myself, I don't complain with whomever applied the patches.

Quoted 5 lines

You picked the patchset up just hours
after it arrived on the list, on a holiday, without giving me a chance to review. It's not about which tree it goes through, it's about you not asking first - which I reminded you of just days ago, so this appears deliberate.

Quoted 4 lines
Quoted 1 line

It certainly is.

Then you can fix up Daniel's patch yourself and I am stepping down as maintainer. Have fun.

Please take a big breath and don't do the Jocelyn Mayer thing ;).

How about we have the KVM call today and calmly talk about maintainer responsibility borders?

Alex

#24 lore raw
Quoted 2 lines · Alexander Graf

How about we have the KVM call today and calmly talk about maintainer responsibility borders?

I'd be happy to attend the call today, yes.

Paolo

#25 lore raw
Quoted 5 lines · Paolo Bonzini
Quoted 2 lines · Alexander Graf

How about we have the KVM call today and calmly talk about maintainer responsibility borders?

I'd be happy to attend the call today, yes.

Was there a call? Any conclusions?

Signature · 1 line
Eduardo
#26 lore raw

Ping!

Was there an outcome?

Regards,
Peter

Quoted 12 lines · Eduardo Habkost
Quoted 5 lines · Paolo Bonzini
Quoted 2 lines · Alexander Graf

How about we have the KVM call today and calmly talk about maintainer responsibility borders?

I'd be happy to attend the call today, yes.

Was there a call? Any conclusions?

--
Eduardo

#27 lore raw
Quoted 3 lines · Peter Crosthwaite

Ping!

Was there an outcome?

Andreas will handle the patches, but he's on vacation now.

Paolo

Quoted 13 lines · Eduardo Habkost
Quoted 12 lines · Paolo Bonzini
Quoted 5 lines · Alexander Graf
Quoted 2 lines

How about we have the KVM call today and calmly talk about maintainer responsibility borders?

I'd be happy to attend the call today, yes.

Was there a call? Any conclusions?

--
Eduardo

#28 lore raw
In reply to Peter Crosthwaite

Hi Peter,

Am 04.06.2015 um 03:10 schrieb Peter Crosthwaite:

Quoted 1 line

Was there an outcome?

The conclusion for this series was that I would get a chance to review it (which I did later than I wanted to, after my talks from a hotel...) and that future patch series should spend a certain minimum time on the list, either before or after being applied by a maintainer but before being included in a PULL. The idea is to assure a) general review opportunity for both affected and non-affected non-maintainers and b) opportunity to react to maintainer's oversights before they become facts in Git history.

Differences of opinion still exist in what commitment to and extend of patch review we expect before patches get applied, and to what degree code/commit conventions vary from maintainer to maintainer or subsystem to subsystem.

Am I seeing correctly that no respin or follow-up happened yet after I reviewed it?

Regards,
Andreas

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
#29 lore raw
Quoted 11 lines · Andreas Färber

Hi Peter,

Am 04.06.2015 um 03:10 schrieb Peter Crosthwaite:

Quoted 1 line

Was there an outcome?

The conclusion for this series was that I would get a chance to review it (which I did later than I wanted to, after my talks from a hotel...) and that future patch series should spend a certain minimum time on the list, either before or after being applied by a maintainer but before being included in a PULL. The idea is to assure a) general review opportunity for both affected and non-affected non-maintainers and b)

It's been a month now. The opportunity has been there with replies from a good number of people.

Quoted 3 lines

opportunity to react to maintainer's oversights before they become facts in Git history.

That's unusual. If the maintainer and contributor are on the same page it doesn't make sense for an objection to occur after the ack. If reviewers want to nack, they should do it regardless of whether the maintainer has weighed in yet. If you get a nack or a comment, there is no difference, the contributor can leave it on list for as long as they like.

I agree that if the maintainer acks it instantly it should be left on list for a reasonable time if it is a first version or has substantial change, but should you be ok with v4 of this series (which only addresses trivial comments) I don't see a need for yet more list time.

Quoted 8 lines

Differences of opinion still exist in what commitment to and extend of patch review we expect before patches get applied, and to what degree code/commit conventions vary from maintainer to maintainer or subsystem to subsystem.

Am I seeing correctly that no respin or follow-up happened yet after I reviewed it?

No, there have been two versions since this one. I have addresses Eduardo and Aurelien's comments on v2 and v3.

I have respun everything I have pending for you with "PATCH qom" subject prefix yesterday/today. All patches were on list well before soft freeze and almost all patches have RBs. You can safely ignore all patches from me from before except the QOM-prop-overloading/A9-mpcore series which is still relevant and not respun.

For a summary here is the review state of the multi-arch patch queue. I have too many reviewed-but-unapplied patches I have stopped working on the feature until I get some of this already-reviewed work off my hands (read bottom up):

d22de8e345 HACK: mb: boot: Disable dtb load in multi-arch
c3202f0920 HACK: mb: boot: Assume using -firmware for mb software

8c27d2cf5a     arm: xilinx_zynq: Add a Microblaze
5f9e0c9c84     arm: boot: Don't assume all CPUs are ARM
3a47db3595     mb: Remove ELF_MACHINE from cpu.h
f886dd4de8     hw: mb: Explicitly include cpu.h for consumers
34b67400b9     arm: Remove ELF_MACHINE from cpu.h
eb628c8ad8     hw: arm: Explicitly include cpu.h for consumers
3981bb283f     core: Introduce multi-arch build
1ac2ff9e6d     arm: enable multi-arch
e7c9f09397     arm: register cpu_list function
69a42b153d     target-arm: Split cp helper API to new C file
a53b4dcedf     arm: cpu: static inline cpu_arm_init
717d43b73e     microblaze: enable multi-arch
1053a79602     target-*: Don't redefine cpu_exec
cf406390fb     core: virtualise CPU interfaces completely
d515ec68c5     HACK: Set tcg_enabled to true
b9397d1c40     cputlb,translate-all: Remove target_ulong from API
a5337ca686     cputlb: Change tlb_set_dirty arg to cpu
e07c23a328     cputlb: move CPU_LOOP for tlb_reset to exec.c
0943c367a6     disas: Defeature print_target_address
95d8a2096a     HACK: monitor: uninclude cpu_ldst
3baa9eeab0     HACK: monitor: Comment out TCG profile ops
105ec2f9ee     cpu-defs: Allow multiple inclusions
a41db438ab     cpu-defs: Move out TB_JMP defines                           R:rth
ccdc103068     translate: move real_host_page setting to -common
2e90cadb43     include/exec: Move tb hash functions out                    R:rth
624c0529d5     include/exec: Move cputlb exec.c defs out                   R:rth
b332fa4c3a     include/exec: Split target_long def to new header
a59e060282     include/exec: Move standard exceptions to cpu-all.h         R:rth
b01e4a1f42     translate-all: Move tcg_handle_interrupt to -common         R:rth
bac799c2fd     exec-all: Move cpu_can_do_io to qom/cpu.h                   R:rth
fa2e6624f9     cpu-common: Define tb_page_addr_t for everyone
326a80bb02     cpus: Listify cpu_list function
f4c00d94c4     translate: Listify tcg_exec_init                            R:rth
8f66b6c191     cpu-exec: Migrate some generic fns to cpus.c                R:rth
0a32363577     Makefile.target: Introduce arch-obj
3f51f6f027     cpu-defs: Move CPU_TEMP_BUF_NLONGS to tcg                   R:rth
5d2163a21b     cpu_defs: Simply CPUTLB padding logic
3a9e19c7f7     vfio: cpu: Use "real" page size API

T:alexey
edbb1333e9 cpu-all: complete "real" host page size API
T:alexey
9eee05f488 memory_mapping: Rework cpu related includes
A:paolo
4561937a3b cpu-exec: Purge all uses of ENV_GET_CPU()
888b57e00f cpus: Change exec_init() arg to cpu, not env
R:af R:aurel
952ac05646 cpus: Change tcg_cpu_exec() arg to cpu, not env
R:af R:eduardo
860c916334 gdbserver: _fork: Change fn to accept cpu instead of
env R:af R:eduardo
df44b66c87 translate-all: Change tb_flush() env argument to cpu
R:eduardo A:eduardo
fb7dc6a221 ppc: Move cpu_exec_init() call to realize function
R:dgibson R:me_x A:paolo
6405382188 cpus: Convert cpu_index into a bitmap
R:eduardo R:igor R:dgibson R:me_x A:paolo
a9e1838b66 cpus: Add Error argument to cpu_exec_init()
R:eduardo R:igor R:dgibson R:me_x A:paolo
ea291ffcde microblaze: boot: Use cpu_set_pc() R:af
11d804948d arm: boot: Use cpu_set_pc()
R:pmm R:af

13dd9e90b1     gdbstub: Use cpu_set_pc() helper
07ca0ce6b4     cpu: Add wrapper to the set-pc() hook
4d46aa8725     disas: cris: QOMify target specific disas setup

R:edgar_x
22bbba694f disas: cris: Fix 0 buffer length case
R:edgar_x
8b16aff59c disas: microblaze: QOMify target specific disas setup
R:edgar_x
8c08b3d14a disas: arm: QOMify target specific disas setup R:pmm
7ca536d824 disas: arm-a64: Make printfer and stream variable
R:claudio T:claudio R:rth
b316905406 disas: QOMify target specific setup R:rth
d10c5b0fc8 disas: Add print_insn to disassemble info R:rth

I was hoping to flush out all the reviewed stuff and get those patches with RTHs RB in for hard freeze but I am stalled on the deps. Then during hard freeze continue the review of the multi-arch core work for a merge in 2.5.

Regards,
Peter

Quoted 8 lines

Regards,
Andreas

--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)

#30 lore raw
Quoted 4 lines · Peter Crosthwaite

For a summary here is the review state of the multi-arch patch queue. I have too many reviewed-but-unapplied patches I have stopped working on the feature until I get some of this already-reviewed work off my hands (read bottom up):

git log --reverse FTW. :)

For now I have queued these:

d7daf77764   cpu-defs: Move out TB_JMP defines
527c8d113c   include/exec: Move tb hash functions out
dfe5fb5153   include/exec: Move standard exceptions to cpu-all.h
edff94d9b5   cpu-defs: Move CPU_TEMP_BUF_NLONGS to tcg
d8faad7ed2   memory_mapping: Rework cpu related includes

Now on to the others. Some are blocked by Bharata's work, and no one knows what tree will shepherd them?!?

Then: these will probably be queued by Andreas?

Quoted 6 lines

ea291ffcde microblaze: boot: Use cpu_set_pc() R:af
11d804948d arm: boot: Use cpu_set_pc()
R:pmm R:af

13dd9e90b1     gdbstub: Use cpu_set_pc() helper
07ca0ce6b4     cpu: Add wrapper to the set-pc() hook
4d46aa8725     disas: cris: QOMify target specific disas setup
Quoted 10 lines

R:edgar_x
22bbba694f disas: cris: Fix 0 buffer length case
R:edgar_x
8b16aff59c disas: microblaze: QOMify target specific disas setup
R:edgar_x
8c08b3d14a disas: arm: QOMify target specific disas setup R:pmm
7ca536d824 disas: arm-a64: Make printfer and stream variable
R:claudio T:claudio R:rth
b316905406 disas: QOMify target specific setup R:rth
d10c5b0fc8 disas: Add print_insn to disassemble info R:rth

For this one I hadn't answered you, and I have looked into it further today. However, I'm leaning towards omitting it for 2.4:

Quoted 1 line

fa2e6624f9 cpu-common: Define tb_page_addr_t for everyone

This one fails compilation for me when extracted from the series:

Quoted 1 line

9d708b430b include/exec: Move cputlb exec.c defs out

This one, I cannot find on the list:

Quoted 1 line

5d2163a21b cpu_defs: Simply CPUTLB padding logic

The first below need an ack from Alex Williamson, but these can also go in 2.4:

Quoted 4 lines

3a9e19c7f7 vfio: cpu: Use "real" page size API
T:alexey
edbb1333e9 cpu-all: complete "real" host page size API
T:alexey

Paolo

#31 lore raw
Quoted 10 lines · Paolo Bonzini
Quoted 4 lines · Peter Crosthwaite

For a summary here is the review state of the multi-arch patch queue. I have too many reviewed-but-unapplied patches I have stopped working on the feature until I get some of this already-reviewed work off my hands (read bottom up):

git log --reverse FTW. :)

Added that to my script :) Much more readable.

Quoted 8 lines

For now I have queued these:

d7daf77764   cpu-defs: Move out TB_JMP defines
527c8d113c   include/exec: Move tb hash functions out
dfe5fb5153   include/exec: Move standard exceptions to cpu-all.h
edff94d9b5   cpu-defs: Move CPU_TEMP_BUF_NLONGS to tcg
d8faad7ed2   memory_mapping: Rework cpu related includes
Quoted 5 lines

Now on to the others. Some are blocked by Bharata's work, and no one knows what tree will shepherd them?!?

Then: these will probably be queued by Andreas?

Yes between you, Andreas and Alex, everything merge-intended (+more - thanks for the cherry picks from the RFC) is in on the hard-freeze merges.

Quoted 17 lines
Quoted 6 lines

ea291ffcde microblaze: boot: Use cpu_set_pc() R:af
11d804948d arm: boot: Use cpu_set_pc()
R:pmm R:af

13dd9e90b1     gdbstub: Use cpu_set_pc() helper
07ca0ce6b4     cpu: Add wrapper to the set-pc() hook
4d46aa8725     disas: cris: QOMify target specific disas setup
Quoted 10 lines

R:edgar_x
22bbba694f disas: cris: Fix 0 buffer length case
R:edgar_x
8b16aff59c disas: microblaze: QOMify target specific disas setup
R:edgar_x
8c08b3d14a disas: arm: QOMify target specific disas setup R:pmm
7ca536d824 disas: arm-a64: Make printfer and stream variable
R:claudio T:claudio R:rth
b316905406 disas: QOMify target specific setup R:rth
d10c5b0fc8 disas: Add print_insn to disassemble info R:rth

Quoted 7 lines

For this one I hadn't answered you, and I have looked into it further today. However, I'm leaning towards omitting it for 2.4:

Quoted 1 line

fa2e6624f9 cpu-common: Define tb_page_addr_t for everyone

This one fails compilation for me when extracted from the series:

Yes, I am now working through fully-configured build testing on the next RFC. My strategy is to send the big RFCs which are semi-functional to gives reviewers full context on what I am trying to do and sometimes things aren't merge ready. At the same time I am extracting components as shorter series or singles as non-RFCs (which AFAIK are problem free) so they can be taken into a number of different queues independently.

With the hard freeze merges 23 down, 37 to go :). RFCv3 coming shortly. Hoping to collect RBs during the freeze then target 2.5 first-flushes.

Regards,
Peter

Quoted 16 lines
Quoted 1 line

9d708b430b include/exec: Move cputlb exec.c defs out

This one, I cannot find on the list:

Quoted 1 line

5d2163a21b cpu_defs: Simply CPUTLB padding logic

The first below need an ack from Alex Williamson, but these can also go in 2.4:

Quoted 4 lines

3a9e19c7f7 vfio: cpu: Use "real" page size API
T:alexey
edbb1333e9 cpu-all: complete "real" host page size API
T:alexey

Paolo

#32 lore raw
In reply to Andreas Färber
Quoted 8 lines · Andreas Färber
Quoted 5 lines
Quoted 3 lines

It's not about which tree it goes through, it's about you not asking first - which I reminded you of just days ago, so this appears deliberate.

It certainly is.

Then you can fix up Daniel's patch yourself and I am stepping down as maintainer. Have fun.

This seems a bit of an overreaction.

If you mean deliberate as in "stepping on someone else's toes", that was not it.

If you mean deliberate as in "disagreeing on whether identifiers contribute to maintainance areas for such a trivial patch", and applying this patch because I truly believe(d) you wouldn't care, then it was.

Paolo

#33 lore raw
Quoted 11 lines · Paolo Bonzini
Quoted 8 lines · Andreas Färber
Quoted 5 lines
Quoted 3 lines

It's not about which tree it goes through, it's about you not asking first - which I reminded you of just days ago, so this appears deliberate.

It certainly is.

Then you can fix up Daniel's patch yourself and I am stepping down as maintainer. Have fun.

This seems a bit of an overreaction.

I think your rather abrupt statement above was rather unfortunately open to misinterpretation...

Quoted 6 lines

If you mean deliberate as in "stepping on someone else's toes", that was not it.

If you mean deliberate as in "disagreeing on whether identifiers contribute to maintainance areas for such a trivial patch", and applying this patch because I truly believe(d) you wouldn't care, then it was.

...since it didn't have this clarification.

-- PMM