[Qemu-devel] [PATCH v2 0/2] monitor+disas: Remove uses of ENV_GET_CPU

[Qemu-devel] [PATCH v2 0/2] monitor+disas: Remove uses of ENV_GET_CPU

[Peter Crosthwaite]

Neither the monitor or disassembly core has a good reason to navigate from an
env pointer to a cpu pointer. Disas should not need env awarness at all, that
is removed in P2.

The monitor is trickier, the env is still needed by some #ifdef switched target
specific code but all common code only needs to trade in CPU pointers. As the
monitor always has access to a CPU pointer naturally, remove ENV_GET_CPU usages
(P1).

This is related to my multi-arch work, where the goal is to minimise use of
architecture defined global definitions, ENV_GET_CPU being a major headache in
that whole effort. The longer term goal is to limit ENV_GET_CPU use to genuinely
architecture specific code.

But I think these two patches stand in their own right, so sending ahead of the
motherload series. This brings both modules closer to common-oby-y'ification.

First RFC for multi arch is avaiable here:

https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01771.html

The two patches are done together to avoid a conflict with monitor_disas which
is touched by both patches. If one patch gets acked, the other nacked then
either can be merged independently with trivial edits.

Changed since v1:
Addressed RH and Andreas comments on P1.

Peter Crosthwaite (2):
  monitor: Split mon_get_cpu fn to remove ENV_GET_CPU
  disas: Remove uses of CPU env

 disas.c                       | 14 +++++-----
 include/disas/disas.h         |  4 +--
 include/qemu/log.h            |  4 +--
 monitor.c                     | 65 +++++++++++++++++++------------------------
 target-alpha/translate.c      |  2 +-
 target-arm/translate-a64.c    |  2 +-
 target-arm/translate.c        |  2 +-
 target-cris/translate.c       |  2 +-
 target-i386/translate.c       |  2 +-
 target-lm32/translate.c       |  2 +-
 target-m68k/translate.c       |  2 +-
 target-microblaze/translate.c |  2 +-
 target-mips/translate.c       |  2 +-
 target-openrisc/translate.c   |  2 +-
 target-ppc/translate.c        |  2 +-
 target-s390x/translate.c      |  2 +-
 target-sh4/translate.c        |  2 +-
 target-sparc/translate.c      |  2 +-
 target-tricore/translate.c    |  2 +-
 target-unicore32/translate.c  |  2 +-
 target-xtensa/translate.c     |  2 +-
 21 files changed, 57 insertions(+), 64 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/2] monitor: Split mon_get_cpu fn to remove ENV_GET_CPU

[Peter Crosthwaite]

The monitor currently has one helper, mon_get_cpu() which will return
an env pointer. The target specific users of this API want an env, but
all the target agnostic users really just want the cpu pointer. These
users then need to use the target-specifically defined ENV_GET_CPU to
navigate back up to the CPU from the ENV. Split the API for the two
uses cases to remove all need for ENV_GET_CPU.

Reviewed-by: Richard Henderson <rth@twiddle.net>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Changed since v1:
s/mon_get_env/mon_get_cpu_env (Andreas review)
Avoid C99 declaration (RH review)
---
 monitor.c | 65 ++++++++++++++++++++++++++++-----------------------------------
 1 file changed, 29 insertions(+), 36 deletions(-)

diff --git a/monitor.c b/monitor.c
index b2561e1..365c0e4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1010,28 +1010,28 @@ int monitor_set_cpu(int cpu_index)
     return 0;
 }
 
-static CPUArchState *mon_get_cpu(void)
+static CPUState *mon_get_cpu(void)
 {
     if (!cur_mon->mon_cpu) {
         monitor_set_cpu(0);
     }
     cpu_synchronize_state(cur_mon->mon_cpu);
-    return cur_mon->mon_cpu->env_ptr;
+    return cur_mon->mon_cpu;
+}
+
+static CPUArchState *mon_get_cpu_env(void)
+{
+    return mon_get_cpu()->env_ptr;
 }
 
 int monitor_get_cpu_index(void)
 {
-    CPUState *cpu = ENV_GET_CPU(mon_get_cpu());
-    return cpu->cpu_index;
+    return mon_get_cpu()->cpu_index;
 }
 
 static void hmp_info_registers(Monitor *mon, const QDict *qdict)
 {
-    CPUState *cpu;
-    CPUArchState *env;
-    env = mon_get_cpu();
-    cpu = ENV_GET_CPU(env);
-    cpu_dump_state(cpu, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
+    cpu_dump_state(mon_get_cpu(), (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
 }
 
 static void hmp_info_jit(Monitor *mon, const QDict *qdict)
@@ -1064,12 +1064,7 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
 
 static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
 {
-    CPUState *cpu;
-    CPUArchState *env;
-
-    env = mon_get_cpu();
-    cpu = ENV_GET_CPU(env);
-    cpu_dump_statistics(cpu, (FILE *)mon, &monitor_fprintf, 0);
+    cpu_dump_statistics(mon_get_cpu(), (FILE *)mon, &monitor_fprintf, 0);
 }
 
 static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
@@ -1208,16 +1203,14 @@ static void monitor_printc(Monitor *mon, int c)
 static void memory_dump(Monitor *mon, int count, int format, int wsize,
                         hwaddr addr, int is_physical)
 {
-    CPUArchState *env;
     int l, line_size, i, max_digits, len;
     uint8_t buf[16];
     uint64_t v;
 
     if (format == 'i') {
-        int flags;
-        flags = 0;
-        env = mon_get_cpu();
+        int flags = 0;
 #ifdef TARGET_I386
+        CPUArchState *env = mon_get_cpu_env();
         if (wsize == 2) {
             flags = 1;
         } else if (wsize == 4) {
@@ -1238,10 +1231,11 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
         }
 #endif
 #ifdef TARGET_PPC
+        CPUArchState *env = mon_get_cpu_env();
         flags = msr_le << 16;
         flags |= env->bfd_mach;
 #endif
-        monitor_disas(mon, env, addr, count, is_physical, flags);
+        monitor_disas(mon, mon_get_cpu_env(), addr, count, is_physical, flags);
         return;
     }
 
@@ -1280,8 +1274,7 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
         if (is_physical) {
             cpu_physical_memory_read(addr, buf, l);
         } else {
-            env = mon_get_cpu();
-            if (cpu_memory_rw_debug(ENV_GET_CPU(env), addr, buf, l, 0) < 0) {
+            if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < 0) {
                 monitor_printf(mon, " Cannot access memory\n");
                 break;
             }
@@ -1660,7 +1653,7 @@ static void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env;
 
-    env = mon_get_cpu();
+    env = mon_get_cpu_env();
 
     if (!(env->cr[0] & CR0_PG_MASK)) {
         monitor_printf(mon, "PG disabled\n");
@@ -1883,7 +1876,7 @@ static void hmp_info_mem(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env;
 
-    env = mon_get_cpu();
+    env = mon_get_cpu_env();
 
     if (!(env->cr[0] & CR0_PG_MASK)) {
         monitor_printf(mon, "PG disabled\n");
@@ -1920,7 +1913,7 @@ static void print_tlb(Monitor *mon, int idx, tlb_t *tlb)
 
 static void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
     int i;
 
     monitor_printf (mon, "ITLB:\n");
@@ -1936,7 +1929,7 @@ static void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 #if defined(TARGET_SPARC) || defined(TARGET_PPC) || defined(TARGET_XTENSA)
 static void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
-    CPUArchState *env1 = mon_get_cpu();
+    CPUArchState *env1 = mon_get_cpu_env();
 
     dump_mmu((FILE*)mon, (fprintf_function)monitor_printf, env1);
 }
@@ -2969,7 +2962,7 @@ typedef struct MonitorDef {
 #if defined(TARGET_I386)
 static target_long monitor_get_pc (const struct MonitorDef *md, int val)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
     return env->eip + env->segs[R_CS].base;
 }
 #endif
@@ -2977,7 +2970,7 @@ static target_long monitor_get_pc (const struct MonitorDef *md, int val)
 #if defined(TARGET_PPC)
 static target_long monitor_get_ccr (const struct MonitorDef *md, int val)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
     unsigned int u;
     int i;
 
@@ -2990,31 +2983,31 @@ static target_long monitor_get_ccr (const struct MonitorDef *md, int val)
 
 static target_long monitor_get_msr (const struct MonitorDef *md, int val)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
     return env->msr;
 }
 
 static target_long monitor_get_xer (const struct MonitorDef *md, int val)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
     return env->xer;
 }
 
 static target_long monitor_get_decr (const struct MonitorDef *md, int val)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
     return cpu_ppc_load_decr(env);
 }
 
 static target_long monitor_get_tbu (const struct MonitorDef *md, int val)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
     return cpu_ppc_load_tbu(env);
 }
 
 static target_long monitor_get_tbl (const struct MonitorDef *md, int val)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
     return cpu_ppc_load_tbl(env);
 }
 #endif
@@ -3023,7 +3016,7 @@ static target_long monitor_get_tbl (const struct MonitorDef *md, int val)
 #ifndef TARGET_SPARC64
 static target_long monitor_get_psr (const struct MonitorDef *md, int val)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
 
     return cpu_get_psr(env);
 }
@@ -3031,7 +3024,7 @@ static target_long monitor_get_psr (const struct MonitorDef *md, int val)
 
 static target_long monitor_get_reg(const struct MonitorDef *md, int val)
 {
-    CPUArchState *env = mon_get_cpu();
+    CPUArchState *env = mon_get_cpu_env();
     return env->regwptr[val];
 }
 #endif
@@ -3367,7 +3360,7 @@ static int get_monitor_def(target_long *pval, const char *name)
             if (md->get_value) {
                 *pval = md->get_value(md, md->offset);
             } else {
-                CPUArchState *env = mon_get_cpu();
+                CPUArchState *env = mon_get_cpu_env();
                 ptr = (uint8_t *)env + md->offset;
                 switch(md->type) {
                 case MD_I32:
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/2] disas: Remove uses of CPU env

[Peter Crosthwaite]

disas does not need to access the CPU env for any reason. Change the
APIs to accept CPU pointers instead. Small change pattern needs to be
applied to all target translate.c. This brings us closer to making
disas.o a common-obj and less architecture specific in general.

Cc: Richard Henderson <rth@twiddle.net>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Michael Walle <michael@walle.cc>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Leon Alrae <leon.alrae@imgtec.com>
Cc: Jia Liu <proljc@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Reviewed-by: Richard Henderson <rth@twiddle.net>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 disas.c                       | 14 +++++++-------
 include/disas/disas.h         |  4 ++--
 include/qemu/log.h            |  4 ++--
 monitor.c                     |  2 +-
 target-alpha/translate.c      |  2 +-
 target-arm/translate-a64.c    |  2 +-
 target-arm/translate.c        |  2 +-
 target-cris/translate.c       |  2 +-
 target-i386/translate.c       |  2 +-
 target-lm32/translate.c       |  2 +-
 target-m68k/translate.c       |  2 +-
 target-microblaze/translate.c |  2 +-
 target-mips/translate.c       |  2 +-
 target-openrisc/translate.c   |  2 +-
 target-ppc/translate.c        |  2 +-
 target-s390x/translate.c      |  2 +-
 target-sh4/translate.c        |  2 +-
 target-sparc/translate.c      |  2 +-
 target-tricore/translate.c    |  2 +-
 target-unicore32/translate.c  |  2 +-
 target-xtensa/translate.c     |  2 +-
 21 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/disas.c b/disas.c
index 44a019a..576c6a4 100644
--- a/disas.c
+++ b/disas.c
@@ -9,7 +9,7 @@
 
 typedef struct CPUDebug {
     struct disassemble_info info;
-    CPUArchState *env;
+    CPUState *cpu;
 } CPUDebug;
 
 /* Filled in by elfload.c.  Simplistic, but will do for now. */
@@ -39,7 +39,7 @@ target_read_memory (bfd_vma memaddr,
 {
     CPUDebug *s = container_of(info, CPUDebug, info);
 
-    cpu_memory_rw_debug(ENV_GET_CPU(s->env), memaddr, myaddr, length, 0);
+    cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, 0);
     return 0;
 }
 
@@ -195,7 +195,7 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
            bit 16 indicates little endian.
     other targets - unused
  */
-void target_disas(FILE *out, CPUArchState *env, target_ulong code,
+void target_disas(FILE *out, CPUState *cpu, target_ulong code,
                   target_ulong size, int flags)
 {
     target_ulong pc;
@@ -205,7 +205,7 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
 
     INIT_DISASSEMBLE_INFO(s.info, out, fprintf);
 
-    s.env = env;
+    s.cpu = cpu;
     s.info.read_memory_func = target_read_memory;
     s.info.buffer_vma = code;
     s.info.buffer_length = size;
@@ -430,7 +430,7 @@ monitor_read_memory (bfd_vma memaddr, bfd_byte *myaddr, int length,
     if (monitor_disas_is_physical) {
         cpu_physical_memory_read(memaddr, myaddr, length);
     } else {
-        cpu_memory_rw_debug(ENV_GET_CPU(s->env), memaddr, myaddr, length, 0);
+        cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, 0);
     }
     return 0;
 }
@@ -447,7 +447,7 @@ monitor_fprintf(FILE *stream, const char *fmt, ...)
 
 /* Disassembler for the monitor.
    See target_disas for a description of flags. */
-void monitor_disas(Monitor *mon, CPUArchState *env,
+void monitor_disas(Monitor *mon, CPUState *cpu,
                    target_ulong pc, int nb_insn, int is_physical, int flags)
 {
     int count, i;
@@ -456,7 +456,7 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
 
     INIT_DISASSEMBLE_INFO(s.info, (FILE *)mon, monitor_fprintf);
 
-    s.env = env;
+    s.cpu = cpu;
     monitor_disas_is_physical = is_physical;
     s.info.read_memory_func = monitor_read_memory;
     s.info.print_address_func = generic_print_target_address;
diff --git a/include/disas/disas.h b/include/disas/disas.h
index c13ca9a..2b9293b 100644
--- a/include/disas/disas.h
+++ b/include/disas/disas.h
@@ -6,10 +6,10 @@
 #ifdef NEED_CPU_H
 /* Disassemble this for me please... (debugging). */
 void disas(FILE *out, void *code, unsigned long size);
-void target_disas(FILE *out, CPUArchState *env, target_ulong code,
+void target_disas(FILE *out, CPUState *cpu, target_ulong code,
                   target_ulong size, int flags);
 
-void monitor_disas(Monitor *mon, CPUArchState *env,
+void monitor_disas(Monitor *mon, CPUState *cpu,
                    target_ulong pc, int nb_insn, int is_physical, int flags);
 
 /* Look up symbol for debugging purpose.  Returns "" if unknown. */
diff --git a/include/qemu/log.h b/include/qemu/log.h
index 195f665..f880e66 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -104,10 +104,10 @@ static inline void log_cpu_state_mask(int mask, CPUState *cpu, int flags)
 
 #ifdef NEED_CPU_H
 /* disas() and target_disas() to qemu_logfile: */
-static inline void log_target_disas(CPUArchState *env, target_ulong start,
+static inline void log_target_disas(CPUState *cpu, target_ulong start,
                                     target_ulong len, int flags)
 {
-    target_disas(qemu_logfile, env, start, len, flags);
+    target_disas(qemu_logfile, cpu, start, len, flags);
 }
 
 static inline void log_disas(void *code, unsigned long size)
diff --git a/monitor.c b/monitor.c
index 365c0e4..1a17cf3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1235,7 +1235,7 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
         flags = msr_le << 16;
         flags |= env->bfd_mach;
 #endif
-        monitor_disas(mon, mon_get_cpu_env(), addr, count, is_physical, flags);
+        monitor_disas(mon, mon_get_cpu(), addr, count, is_physical, flags);
         return;
     }
 
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index e9927b5..81d4ff8 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2939,7 +2939,7 @@ static inline void gen_intermediate_code_internal(AlphaCPU *cpu,
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, ctx.pc - pc_start, 1);
+        log_target_disas(cs, pc_start, ctx.pc - pc_start, 1);
         qemu_log("\n");
     }
 #endif
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 0b192a1..220f055 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -11114,7 +11114,7 @@ done_generating:
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, dc->pc - pc_start,
+        log_target_disas(cs, pc_start, dc->pc - pc_start,
                          4 | (dc->bswap_code << 1));
         qemu_log("\n");
     }
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 9116529..cd91b13 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11365,7 +11365,7 @@ done_generating:
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, dc->pc - pc_start,
+        log_target_disas(cs, pc_start, dc->pc - pc_start,
                          dc->thumb | (dc->bswap_code << 1));
         qemu_log("\n");
     }
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 687c88b..3e59601 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3345,7 +3345,7 @@ gen_intermediate_code_internal(CRISCPU *cpu, TranslationBlock *tb,
 #ifdef DEBUG_DISAS
 #if !DISAS_CRIS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
-        log_target_disas(env, pc_start, dc->pc - pc_start,
+        log_target_disas(cs, pc_start, dc->pc - pc_start,
                          env->pregs[PR_VR]);
         qemu_log("\nisize=%d osize=%d\n",
                  dc->pc - pc_start, tcg_op_buf_count());
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 305ce50..723e0cb 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -8093,7 +8093,7 @@ done_generating:
         else
 #endif
             disas_flags = !dc->code32;
-        log_target_disas(env, pc_start, pc_ptr - pc_start, disas_flags);
+        log_target_disas(cs, pc_start, pc_ptr - pc_start, disas_flags);
         qemu_log("\n");
     }
 #endif
diff --git a/target-lm32/translate.c b/target-lm32/translate.c
index 81a204f..cf7042e 100644
--- a/target-lm32/translate.c
+++ b/target-lm32/translate.c
@@ -1168,7 +1168,7 @@ void gen_intermediate_code_internal(LM32CPU *cpu,
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("\n");
-        log_target_disas(env, pc_start, dc->pc - pc_start, 0);
+        log_target_disas(cs, pc_start, dc->pc - pc_start, 0);
         qemu_log("\nisize=%d osize=%d\n",
                  dc->pc - pc_start, tcg_op_buf_count());
     }
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 4959b97..22ecc20 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -3077,7 +3077,7 @@ gen_intermediate_code_internal(M68kCPU *cpu, TranslationBlock *tb,
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, dc->pc - pc_start, 0);
+        log_target_disas(cs, pc_start, dc->pc - pc_start, 0);
         qemu_log("\n");
     }
 #endif
diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
index 4068946..633131e 100644
--- a/target-microblaze/translate.c
+++ b/target-microblaze/translate.c
@@ -1855,7 +1855,7 @@ gen_intermediate_code_internal(MicroBlazeCPU *cpu, TranslationBlock *tb,
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("\n");
 #if DISAS_GNU
-        log_target_disas(env, pc_start, dc->pc - pc_start, 0);
+        log_target_disas(cs, pc_start, dc->pc - pc_start, 0);
 #endif
         qemu_log("\nisize=%d osize=%d\n",
                  dc->pc - pc_start, tcg_op_buf_count());
diff --git a/target-mips/translate.c b/target-mips/translate.c
index fd063a2..f8f8f94 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -19273,7 +19273,7 @@ done_generating:
     LOG_DISAS("\n");
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, ctx.pc - pc_start, 0);
+        log_target_disas(cs, pc_start, ctx.pc - pc_start, 0);
         qemu_log("\n");
     }
 #endif
diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c
index dc76789..a62cbf4 100644
--- a/target-openrisc/translate.c
+++ b/target-openrisc/translate.c
@@ -1770,7 +1770,7 @@ static inline void gen_intermediate_code_internal(OpenRISCCPU *cpu,
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("\n");
-        log_target_disas(&cpu->env, pc_start, dc->pc - pc_start, 0);
+        log_target_disas(cs, pc_start, dc->pc - pc_start, 0);
         qemu_log("\nisize=%d osize=%d\n",
                  dc->pc - pc_start, tcg_op_buf_count());
     }
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 8f255ea..84c5cea 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -11607,7 +11607,7 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
         flags = env->bfd_mach;
         flags |= ctx.le_mode << 16;
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, ctx.nip - pc_start, flags);
+        log_target_disas(cs, pc_start, ctx.nip - pc_start, flags);
         qemu_log("\n");
     }
 #endif
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 80e3a54..2fdd542 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -4990,7 +4990,7 @@ static inline void gen_intermediate_code_internal(S390CPU *cpu,
 #if defined(S390X_DEBUG_DISAS)
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, dc.pc - pc_start, 1);
+        log_target_disas(cs, pc_start, dc.pc - pc_start, 1);
         qemu_log("\n");
     }
 #endif
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index 41aa928..27858e5 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -1974,7 +1974,7 @@ gen_intermediate_code_internal(SuperHCPU *cpu, TranslationBlock *tb,
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
 	qemu_log("IN:\n");	/* , lookup_symbol(pc_start)); */
-        log_target_disas(env, pc_start, ctx.pc - pc_start, 0);
+        log_target_disas(cs, pc_start, ctx.pc - pc_start, 0);
 	qemu_log("\n");
     }
 #endif
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 3708c01..c58dd4e 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -5334,7 +5334,7 @@ static inline void gen_intermediate_code_internal(SPARCCPU *cpu,
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("--------------\n");
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, last_pc + 4 - pc_start, 0);
+        log_target_disas(cs, pc_start, last_pc + 4 - pc_start, 0);
         qemu_log("\n");
     }
 #endif
diff --git a/target-tricore/translate.c b/target-tricore/translate.c
index 5f8eff0..e09f3b5 100644
--- a/target-tricore/translate.c
+++ b/target-tricore/translate.c
@@ -8335,7 +8335,7 @@ gen_intermediate_code_internal(TriCoreCPU *cpu, struct TranslationBlock *tb,
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, ctx.pc - pc_start, 0);
+        log_target_disas(cs, pc_start, ctx.pc - pc_start, 0);
         qemu_log("\n");
     }
 #endif
diff --git a/target-unicore32/translate.c b/target-unicore32/translate.c
index 9efcff5..2fc78e6 100644
--- a/target-unicore32/translate.c
+++ b/target-unicore32/translate.c
@@ -2039,7 +2039,7 @@ done_generating:
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, dc->pc - pc_start, 0);
+        log_target_disas(cs, pc_start, dc->pc - pc_start, 0);
         qemu_log("\n");
     }
 #endif
diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
index 6e5096c..3bdd052 100644
--- a/target-xtensa/translate.c
+++ b/target-xtensa/translate.c
@@ -3137,7 +3137,7 @@ void gen_intermediate_code_internal(XtensaCPU *cpu,
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(env, pc_start, dc.pc - pc_start, 0);
+        log_target_disas(cs, pc_start, dc.pc - pc_start, 0);
         qemu_log("\n");
     }
 #endif
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 0/2] monitor+disas: Remove uses of ENV_GET_CPU

[Peter Crosthwaite]

Ping!

So there was some uncertainty around maintainerships earlier (on some
follow up work to this one) and I wonder what the target queue is for
this stuff.

Can the monitor people perhaps ack/nack P1?

Regards,
Peter

On Sun, May 24, 2015 at 2:20 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> Neither the monitor or disassembly core has a good reason to navigate from an
> env pointer to a cpu pointer. Disas should not need env awarness at all, that
> is removed in P2.
>
> The monitor is trickier, the env is still needed by some #ifdef switched target
> specific code but all common code only needs to trade in CPU pointers. As the
> monitor always has access to a CPU pointer naturally, remove ENV_GET_CPU usages
> (P1).
>
> This is related to my multi-arch work, where the goal is to minimise use of
> architecture defined global definitions, ENV_GET_CPU being a major headache in
> that whole effort. The longer term goal is to limit ENV_GET_CPU use to genuinely
> architecture specific code.
>
> But I think these two patches stand in their own right, so sending ahead of the
> motherload series. This brings both modules closer to common-oby-y'ification.
>
> First RFC for multi arch is avaiable here:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01771.html
>
> The two patches are done together to avoid a conflict with monitor_disas which
> is touched by both patches. If one patch gets acked, the other nacked then
> either can be merged independently with trivial edits.
>
> Changed since v1:
> Addressed RH and Andreas comments on P1.
>
> Peter Crosthwaite (2):
>   monitor: Split mon_get_cpu fn to remove ENV_GET_CPU
>   disas: Remove uses of CPU env
>
>  disas.c                       | 14 +++++-----
>  include/disas/disas.h         |  4 +--
>  include/qemu/log.h            |  4 +--
>  monitor.c                     | 65 +++++++++++++++++++------------------------
>  target-alpha/translate.c      |  2 +-
>  target-arm/translate-a64.c    |  2 +-
>  target-arm/translate.c        |  2 +-
>  target-cris/translate.c       |  2 +-
>  target-i386/translate.c       |  2 +-
>  target-lm32/translate.c       |  2 +-
>  target-m68k/translate.c       |  2 +-
>  target-microblaze/translate.c |  2 +-
>  target-mips/translate.c       |  2 +-
>  target-openrisc/translate.c   |  2 +-
>  target-ppc/translate.c        |  2 +-
>  target-s390x/translate.c      |  2 +-
>  target-sh4/translate.c        |  2 +-
>  target-sparc/translate.c      |  2 +-
>  target-tricore/translate.c    |  2 +-
>  target-unicore32/translate.c  |  2 +-
>  target-xtensa/translate.c     |  2 +-
>  21 files changed, 57 insertions(+), 64 deletions(-)
>
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH v2 0/2] monitor+disas: Remove uses of ENV_GET_CPU

[Luiz Capitulino]

On Tue, 2 Jun 2015 01:06:11 -0700
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> Ping!
> 
> So there was some uncertainty around maintainerships earlier (on some
> follow up work to this one) and I wonder what the target queue is for
> this stuff.

It's the monitor queue, for which I'm the maintainer. But I won't have
to go over patches this week.

If Markus, or any other maintainer, wants to take this one and the other
patches I have queued through their trees, that would be more than
welcome.

Re: [Qemu-devel] [PATCH v2 0/2] monitor+disas: Remove uses of ENV_GET_CPU

[Luiz Capitulino]

On Sun, 24 May 2015 14:20:39 -0700
Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:

> Neither the monitor or disassembly core has a good reason to navigate from an
> env pointer to a cpu pointer. Disas should not need env awarness at all, that
> is removed in P2.
> 
> The monitor is trickier, the env is still needed by some #ifdef switched target
> specific code but all common code only needs to trade in CPU pointers. As the
> monitor always has access to a CPU pointer naturally, remove ENV_GET_CPU usages
> (P1).
> 
> This is related to my multi-arch work, where the goal is to minimise use of
> architecture defined global definitions, ENV_GET_CPU being a major headache in
> that whole effort. The longer term goal is to limit ENV_GET_CPU use to genuinely
> architecture specific code.
> 
> But I think these two patches stand in their own right, so sending ahead of the
> motherload series. This brings both modules closer to common-oby-y'ification.
> 
> First RFC for multi arch is avaiable here:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01771.html
> 
> The two patches are done together to avoid a conflict with monitor_disas which
> is touched by both patches. If one patch gets acked, the other nacked then
> either can be merged independently with trivial edits.

Unfortunately, I'm quite busy and won't have time to push this
through my tree. Markus is going to pick up this series soon.

Acked-by: Luiz Capitulino <lcapitulino@redhat.com>

> 
> Changed since v1:
> Addressed RH and Andreas comments on P1.
> 
> Peter Crosthwaite (2):
>   monitor: Split mon_get_cpu fn to remove ENV_GET_CPU
>   disas: Remove uses of CPU env
> 
>  disas.c                       | 14 +++++-----
>  include/disas/disas.h         |  4 +--
>  include/qemu/log.h            |  4 +--
>  monitor.c                     | 65 +++++++++++++++++++------------------------
>  target-alpha/translate.c      |  2 +-
>  target-arm/translate-a64.c    |  2 +-
>  target-arm/translate.c        |  2 +-
>  target-cris/translate.c       |  2 +-
>  target-i386/translate.c       |  2 +-
>  target-lm32/translate.c       |  2 +-
>  target-m68k/translate.c       |  2 +-
>  target-microblaze/translate.c |  2 +-
>  target-mips/translate.c       |  2 +-
>  target-openrisc/translate.c   |  2 +-
>  target-ppc/translate.c        |  2 +-
>  target-s390x/translate.c      |  2 +-
>  target-sh4/translate.c        |  2 +-
>  target-sparc/translate.c      |  2 +-
>  target-tricore/translate.c    |  2 +-
>  target-unicore32/translate.c  |  2 +-
>  target-xtensa/translate.c     |  2 +-
>  21 files changed, 57 insertions(+), 64 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 1/2] monitor: Split mon_get_cpu fn to remove ENV_GET_CPU

[Markus Armbruster]

Peter Crosthwaite <crosthwaitepeter@gmail.com> writes:

> The monitor currently has one helper, mon_get_cpu() which will return
> an env pointer. The target specific users of this API want an env, but
> all the target agnostic users really just want the cpu pointer. These
> users then need to use the target-specifically defined ENV_GET_CPU to
> navigate back up to the CPU from the ENV. Split the API for the two
> uses cases to remove all need for ENV_GET_CPU.
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> Changed since v1:
> s/mon_get_env/mon_get_cpu_env (Andreas review)
> Avoid C99 declaration (RH review)

  CC    x86_64-softmmu/monitor.o
/work/armbru/qemu/monitor.c: In function ‘memory_search’:
/work/armbru/qemu/monitor.c:1222:9: warning: passing argument 1 of ‘x86_env_get_cpu’ from incompatible pointer type [enabled by default]
         } else if (cpu_memory_rw_debug(ENV_GET_CPU(mon_get_cpu()), addr,
         ^
In file included from /work/armbru/qemu/target-i386/cpu.h:982:0,
                 from /work/armbru/qemu/include/qemu-common.h:124,
                 from /work/armbru/qemu/include/hw/hw.h:5,
                 from /work/armbru/qemu/monitor.c:25:
/work/armbru/qemu/target-i386/cpu-qom.h:119:23: note: expected ‘struct CPUX86State *’ but argument is of type ‘struct CPUState *’
 static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
                       ^

Re: [Qemu-devel] [PATCH v2 1/2] monitor: Split mon_get_cpu fn to remove ENV_GET_CPU

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Peter Crosthwaite <crosthwaitepeter@gmail.com> writes:
>
>> The monitor currently has one helper, mon_get_cpu() which will return
>> an env pointer. The target specific users of this API want an env, but
>> all the target agnostic users really just want the cpu pointer. These
>> users then need to use the target-specifically defined ENV_GET_CPU to
>> navigate back up to the CPU from the ENV. Split the API for the two
>> uses cases to remove all need for ENV_GET_CPU.
>>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> Changed since v1:
>> s/mon_get_env/mon_get_cpu_env (Andreas review)
>> Avoid C99 declaration (RH review)
>
>   CC    x86_64-softmmu/monitor.o
> /work/armbru/qemu/monitor.c: In function ‘memory_search’:
> /work/armbru/qemu/monitor.c:1222:9: warning: passing argument 1 of ‘x86_env_get_cpu’ from incompatible pointer type [enabled by default]
>          } else if (cpu_memory_rw_debug(ENV_GET_CPU(mon_get_cpu()), addr,
>          ^
> In file included from /work/armbru/qemu/target-i386/cpu.h:982:0,
>                  from /work/armbru/qemu/include/qemu-common.h:124,
>                  from /work/armbru/qemu/include/hw/hw.h:5,
>                  from /work/armbru/qemu/monitor.c:25:
> /work/armbru/qemu/target-i386/cpu-qom.h:119:23: note: expected ‘struct CPUX86State *’ but argument is of type ‘struct CPUState *’
>  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
>                        ^

Semantic conflict with
[RFC v6 0/2] monitor: add memory search commands s, sp

Since that series is marked RFC, I'm picking up yours, and will ask
Claudio to rebase.

Re: [Qemu-devel] [PATCH v2 1/2] monitor: Split mon_get_cpu fn to remove ENV_GET_CPU

[Markus Armbruster]

Peter Crosthwaite <crosthwaitepeter@gmail.com> writes:

> The monitor currently has one helper, mon_get_cpu() which will return
> an env pointer. The target specific users of this API want an env, but
> all the target agnostic users really just want the cpu pointer. These
> users then need to use the target-specifically defined ENV_GET_CPU to
> navigate back up to the CPU from the ENV. Split the API for the two
> uses cases to remove all need for ENV_GET_CPU.
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> Changed since v1:
> s/mon_get_env/mon_get_cpu_env (Andreas review)
> Avoid C99 declaration (RH review)
> ---
>  monitor.c | 65 ++++++++++++++++++++++++++++-----------------------------------
>  1 file changed, 29 insertions(+), 36 deletions(-)
>
[...]
> @@ -1208,16 +1203,14 @@ static void monitor_printc(Monitor *mon, int c)
>  static void memory_dump(Monitor *mon, int count, int format, int wsize,
>                          hwaddr addr, int is_physical)
>  {
> -    CPUArchState *env;
>      int l, line_size, i, max_digits, len;
>      uint8_t buf[16];
>      uint64_t v;
>  
>      if (format == 'i') {
> -        int flags;
> -        flags = 0;
> -        env = mon_get_cpu();
> +        int flags = 0;
>  #ifdef TARGET_I386
> +        CPUArchState *env = mon_get_cpu_env();
>          if (wsize == 2) {
>              flags = 1;
>          } else if (wsize == 4) {
> @@ -1238,10 +1231,11 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>          }
>  #endif
>  #ifdef TARGET_PPC
> +        CPUArchState *env = mon_get_cpu_env();
>          flags = msr_le << 16;
>          flags |= env->bfd_mach;
>  #endif
> -        monitor_disas(mon, env, addr, count, is_physical, flags);
> +        monitor_disas(mon, mon_get_cpu_env(), addr, count, is_physical, flags);
>          return;
>      }
>  
> @@ -1280,8 +1274,7 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>          if (is_physical) {
>              cpu_physical_memory_read(addr, buf, l);
>          } else {
> -            env = mon_get_cpu();
> -            if (cpu_memory_rw_debug(ENV_GET_CPU(env), addr, buf, l, 0) < 0) {
> +            if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < 0) {
>                  monitor_printf(mon, " Cannot access memory\n");
>                  break;
>              }

You call mon_get_cpu_env() and mon_get_cpu() multiple times in this
function, and declare CPUArchState *env twice (which rang my shadowing
alarm bell; fortunately the two are under mutually exclusive #ifdef).
Why not simply do

    CPUState *cpu = mon_get_cpu();
    CPUArchState *env = mon_get_cpu_env();

right at the beginning and be done with it?

Not a big deal, I'm willing to take this patch through my tree as is.

[...]

Re: [Qemu-devel] [PATCH v2 1/2] monitor: Split mon_get_cpu fn to remove ENV_GET_CPU

[Peter Crosthwaite]

On Tue, Jun 16, 2015 at 8:09 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Crosthwaite <crosthwaitepeter@gmail.com> writes:
>
>> The monitor currently has one helper, mon_get_cpu() which will return
>> an env pointer. The target specific users of this API want an env, but
>> all the target agnostic users really just want the cpu pointer. These
>> users then need to use the target-specifically defined ENV_GET_CPU to
>> navigate back up to the CPU from the ENV. Split the API for the two
>> uses cases to remove all need for ENV_GET_CPU.
>>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> Changed since v1:
>> s/mon_get_env/mon_get_cpu_env (Andreas review)
>> Avoid C99 declaration (RH review)
>> ---
>>  monitor.c | 65 ++++++++++++++++++++++++++++-----------------------------------
>>  1 file changed, 29 insertions(+), 36 deletions(-)
>>
> [...]
>> @@ -1208,16 +1203,14 @@ static void monitor_printc(Monitor *mon, int c)
>>  static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>                          hwaddr addr, int is_physical)
>>  {
>> -    CPUArchState *env;
>>      int l, line_size, i, max_digits, len;
>>      uint8_t buf[16];
>>      uint64_t v;
>>
>>      if (format == 'i') {
>> -        int flags;
>> -        flags = 0;
>> -        env = mon_get_cpu();
>> +        int flags = 0;
>>  #ifdef TARGET_I386
>> +        CPUArchState *env = mon_get_cpu_env();
>>          if (wsize == 2) {
>>              flags = 1;
>>          } else if (wsize == 4) {
>> @@ -1238,10 +1231,11 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>          }
>>  #endif
>>  #ifdef TARGET_PPC
>> +        CPUArchState *env = mon_get_cpu_env();
>>          flags = msr_le << 16;
>>          flags |= env->bfd_mach;
>>  #endif
>> -        monitor_disas(mon, env, addr, count, is_physical, flags);
>> +        monitor_disas(mon, mon_get_cpu_env(), addr, count, is_physical, flags);
>>          return;
>>      }
>>
>> @@ -1280,8 +1274,7 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>          if (is_physical) {
>>              cpu_physical_memory_read(addr, buf, l);
>>          } else {
>> -            env = mon_get_cpu();
>> -            if (cpu_memory_rw_debug(ENV_GET_CPU(env), addr, buf, l, 0) < 0) {
>> +            if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < 0) {
>>                  monitor_printf(mon, " Cannot access memory\n");
>>                  break;
>>              }
>
> You call mon_get_cpu_env() and mon_get_cpu() multiple times in this
> function, and declare CPUArchState *env twice (which rang my shadowing
> alarm bell; fortunately the two are under mutually exclusive #ifdef).
> Why not simply do
>
>     CPUState *cpu = mon_get_cpu();

This we can do easily and the choice of the existing code is pure
personal preference. I generally prefer inline calls to trivial
getters for a low number of call sites as I think code is slightly
easier to read when you don't have to do a local variable lookup on
where all the function args are coming from.

>     CPUArchState *env = mon_get_cpu_env();
>

So the multiple decl of env is now needed to avoid an unused variable
werror for non-x86-non-PPC builds when considering the change in P2.
Not strictly needed until P2, but doing it this way straight up
slightly minimises churn. The ENV_GET_CPU removal and the change in P2
remove the only two unconditional uses of env forcing this variable to
go local to its #ifdef usages. It also has the advantage that only
those two arches use the env at all. Envlessness is a good thing for
common code so prefer to leave the multi-defs in target specific code
with a plan to get rid of them one by one with X86/PPC specific
patches that operate solely on their respective #ifdef sections.

FWIW, the follow up to this series adds the infrastructure needed for
getting rid of these #ifdef sections (once I muster the courage to
patch X86 and PPC):

https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04742.html

> right at the beginning and be done with it?
>
> Not a big deal, I'm willing to take this patch through my tree as is.
>

Let me know if you need respin for the first change. I can turn it
around quickly, i'd really like to get this one through as its
blocking a lot of the multi-arch work!

Regards,
Peter

> [...]
>

Re: [Qemu-devel] [PATCH v2 0/2] monitor+disas: Remove uses of ENV_GET_CPU

[Markus Armbruster]

Peter Crosthwaite <crosthwaitepeter@gmail.com> writes:

> Neither the monitor or disassembly core has a good reason to navigate from an
> env pointer to a cpu pointer. Disas should not need env awarness at all, that
> is removed in P2.
>
> The monitor is trickier, the env is still needed by some #ifdef switched target
> specific code but all common code only needs to trade in CPU pointers. As the
> monitor always has access to a CPU pointer naturally, remove ENV_GET_CPU usages
> (P1).
>
> This is related to my multi-arch work, where the goal is to minimise use of
> architecture defined global definitions, ENV_GET_CPU being a major headache in
> that whole effort. The longer term goal is to limit ENV_GET_CPU use to genuinely
> architecture specific code.
>
> But I think these two patches stand in their own right, so sending ahead of the
> motherload series. This brings both modules closer to common-oby-y'ification.
>
> First RFC for multi arch is avaiable here:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01771.html
>
> The two patches are done together to avoid a conflict with monitor_disas which
> is touched by both patches. If one patch gets acked, the other nacked then
> either can be merged independently with trivial edits.

Applied to my (badly named) qapi-next branch, thanks!

Re: [Qemu-devel] [PATCH v2 1/2] monitor: Split mon_get_cpu fn to remove ENV_GET_CPU

[Markus Armbruster]

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> On Tue, Jun 16, 2015 at 8:09 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Crosthwaite <crosthwaitepeter@gmail.com> writes:
>>
>>> The monitor currently has one helper, mon_get_cpu() which will return
>>> an env pointer. The target specific users of this API want an env, but
>>> all the target agnostic users really just want the cpu pointer. These
>>> users then need to use the target-specifically defined ENV_GET_CPU to
>>> navigate back up to the CPU from the ENV. Split the API for the two
>>> uses cases to remove all need for ENV_GET_CPU.
>>>
>>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>> ---
>>> Changed since v1:
>>> s/mon_get_env/mon_get_cpu_env (Andreas review)
>>> Avoid C99 declaration (RH review)
>>> ---
>>>  monitor.c | 65
>>> ++++++++++++++++++++++++++++-----------------------------------
>>>  1 file changed, 29 insertions(+), 36 deletions(-)
>>>
>> [...]
>>> @@ -1208,16 +1203,14 @@ static void monitor_printc(Monitor *mon, int c)
>>>  static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>>                          hwaddr addr, int is_physical)
>>>  {
>>> -    CPUArchState *env;
>>>      int l, line_size, i, max_digits, len;
>>>      uint8_t buf[16];
>>>      uint64_t v;
>>>
>>>      if (format == 'i') {
>>> -        int flags;
>>> -        flags = 0;
>>> -        env = mon_get_cpu();
>>> +        int flags = 0;
>>>  #ifdef TARGET_I386
>>> +        CPUArchState *env = mon_get_cpu_env();
>>>          if (wsize == 2) {
>>>              flags = 1;
>>>          } else if (wsize == 4) {
>>> @@ -1238,10 +1231,11 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>>          }
>>>  #endif
>>>  #ifdef TARGET_PPC
>>> +        CPUArchState *env = mon_get_cpu_env();
>>>          flags = msr_le << 16;
>>>          flags |= env->bfd_mach;
>>>  #endif
>>> -        monitor_disas(mon, env, addr, count, is_physical, flags);
>>> +        monitor_disas(mon, mon_get_cpu_env(), addr, count, is_physical, flags);
>>>          return;
>>>      }
>>>
>>> @@ -1280,8 +1274,7 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>>          if (is_physical) {
>>>              cpu_physical_memory_read(addr, buf, l);
>>>          } else {
>>> -            env = mon_get_cpu();
>>> -            if (cpu_memory_rw_debug(ENV_GET_CPU(env), addr, buf, l, 0) < 0) {
>>> +            if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < 0) {
>>>                  monitor_printf(mon, " Cannot access memory\n");
>>>                  break;
>>>              }
>>
>> You call mon_get_cpu_env() and mon_get_cpu() multiple times in this
>> function, and declare CPUArchState *env twice (which rang my shadowing
>> alarm bell; fortunately the two are under mutually exclusive #ifdef).
>> Why not simply do
>>
>>     CPUState *cpu = mon_get_cpu();
>
> This we can do easily and the choice of the existing code is pure
> personal preference. I generally prefer inline calls to trivial
> getters for a low number of call sites as I think code is slightly
> easier to read when you don't have to do a local variable lookup on
> where all the function args are coming from.

Point taken.

The getter isn't quite trivial, though: cpu_synchronize_state().

>>     CPUArchState *env = mon_get_cpu_env();
>>
>
> So the multiple decl of env is now needed to avoid an unused variable
> werror for non-x86-non-PPC builds when considering the change in P2.
> Not strictly needed until P2, but doing it this way straight up
> slightly minimises churn. The ENV_GET_CPU removal and the change in P2
> remove the only two unconditional uses of env forcing this variable to
> go local to its #ifdef usages. It also has the advantage that only
> those two arches use the env at all. Envlessness is a good thing for
> common code so prefer to leave the multi-defs in target specific code
> with a plan to get rid of them one by one with X86/PPC specific
> patches that operate solely on their respective #ifdef sections.
>
> FWIW, the follow up to this series adds the infrastructure needed for
> getting rid of these #ifdef sections (once I muster the courage to
> patch X86 and PPC):
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04742.html
>
>> right at the beginning and be done with it?
>>
>> Not a big deal, I'm willing to take this patch through my tree as is.
>>
>
> Let me know if you need respin for the first change. I can turn it

Your choice.  As I said, I'm willing to take it as is.

> around quickly, i'd really like to get this one through as its
> blocking a lot of the multi-arch work!

Aiming for a pull request this week.

Re: [Qemu-devel] [PATCH v2 1/2] monitor: Split mon_get_cpu fn to remove ENV_GET_CPU

[Peter Crosthwaite]

On Tue, Jun 16, 2015 at 11:42 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>
>> On Tue, Jun 16, 2015 at 8:09 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Peter Crosthwaite <crosthwaitepeter@gmail.com> writes:
>>>
>>>> The monitor currently has one helper, mon_get_cpu() which will return
>>>> an env pointer. The target specific users of this API want an env, but
>>>> all the target agnostic users really just want the cpu pointer. These
>>>> users then need to use the target-specifically defined ENV_GET_CPU to
>>>> navigate back up to the CPU from the ENV. Split the API for the two
>>>> uses cases to remove all need for ENV_GET_CPU.
>>>>
>>>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>>> ---
>>>> Changed since v1:
>>>> s/mon_get_env/mon_get_cpu_env (Andreas review)
>>>> Avoid C99 declaration (RH review)
>>>> ---
>>>>  monitor.c | 65
>>>> ++++++++++++++++++++++++++++-----------------------------------
>>>>  1 file changed, 29 insertions(+), 36 deletions(-)
>>>>
>>> [...]
>>>> @@ -1208,16 +1203,14 @@ static void monitor_printc(Monitor *mon, int c)
>>>>  static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>>>                          hwaddr addr, int is_physical)
>>>>  {
>>>> -    CPUArchState *env;
>>>>      int l, line_size, i, max_digits, len;
>>>>      uint8_t buf[16];
>>>>      uint64_t v;
>>>>
>>>>      if (format == 'i') {
>>>> -        int flags;
>>>> -        flags = 0;
>>>> -        env = mon_get_cpu();
>>>> +        int flags = 0;
>>>>  #ifdef TARGET_I386
>>>> +        CPUArchState *env = mon_get_cpu_env();
>>>>          if (wsize == 2) {
>>>>              flags = 1;
>>>>          } else if (wsize == 4) {
>>>> @@ -1238,10 +1231,11 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>>>          }
>>>>  #endif
>>>>  #ifdef TARGET_PPC
>>>> +        CPUArchState *env = mon_get_cpu_env();
>>>>          flags = msr_le << 16;
>>>>          flags |= env->bfd_mach;
>>>>  #endif
>>>> -        monitor_disas(mon, env, addr, count, is_physical, flags);
>>>> +        monitor_disas(mon, mon_get_cpu_env(), addr, count, is_physical, flags);
>>>>          return;
>>>>      }
>>>>
>>>> @@ -1280,8 +1274,7 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>>>          if (is_physical) {
>>>>              cpu_physical_memory_read(addr, buf, l);
>>>>          } else {
>>>> -            env = mon_get_cpu();
>>>> -            if (cpu_memory_rw_debug(ENV_GET_CPU(env), addr, buf, l, 0) < 0) {
>>>> +            if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < 0) {
>>>>                  monitor_printf(mon, " Cannot access memory\n");
>>>>                  break;
>>>>              }
>>>
>>> You call mon_get_cpu_env() and mon_get_cpu() multiple times in this
>>> function, and declare CPUArchState *env twice (which rang my shadowing
>>> alarm bell; fortunately the two are under mutually exclusive #ifdef).
>>> Why not simply do
>>>
>>>     CPUState *cpu = mon_get_cpu();
>>
>> This we can do easily and the choice of the existing code is pure
>> personal preference. I generally prefer inline calls to trivial
>> getters for a low number of call sites as I think code is slightly
>> easier to read when you don't have to do a local variable lookup on
>> where all the function args are coming from.
>
> Point taken.
>
> The getter isn't quite trivial, though: cpu_synchronize_state().
>
>>>     CPUArchState *env = mon_get_cpu_env();
>>>
>>
>> So the multiple decl of env is now needed to avoid an unused variable
>> werror for non-x86-non-PPC builds when considering the change in P2.
>> Not strictly needed until P2, but doing it this way straight up
>> slightly minimises churn. The ENV_GET_CPU removal and the change in P2
>> remove the only two unconditional uses of env forcing this variable to
>> go local to its #ifdef usages. It also has the advantage that only
>> those two arches use the env at all. Envlessness is a good thing for
>> common code so prefer to leave the multi-defs in target specific code
>> with a plan to get rid of them one by one with X86/PPC specific
>> patches that operate solely on their respective #ifdef sections.
>>
>> FWIW, the follow up to this series adds the infrastructure needed for
>> getting rid of these #ifdef sections (once I muster the courage to
>> patch X86 and PPC):
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04742.html
>>
>>> right at the beginning and be done with it?
>>>
>>> Not a big deal, I'm willing to take this patch through my tree as is.
>>>
>>
>> Let me know if you need respin for the first change. I can turn it
>
> Your choice.  As I said, I'm willing to take it as is.
>

Thanks, as it is queued I will leave it and I have made a note in my
todos to follow this up.

Regards,
Peter

>> around quickly, i'd really like to get this one through as its
>> blocking a lot of the multi-arch work!
>
> Aiming for a pull request this week.
>