[Qemu-devel] [PATCH 0/7] disas: Unify target_disas and monitor_disas

[Qemu-devel] [PATCH 0/7] disas: Unify target_disas and monitor_disas

[Peter Crosthwaite]

There two functions are mostly trying to do the same thing, which is
disassemble a target instruction (sequence) for printfing. The
architecture specific setup is largely duped between the two functions.
Most architectures are carbon copy and can be factored out without
issue (P1). Three architectures (microblaze, cris and ARM) are patched
to make the monitor_disas consistent with target_disas.

Microblaze is trivial, the target_disas setup is directly applicable
to monitor_disas to bring in microblaze monitor disas support (P2).

Cris had a small hiccup, a patch is needed to handle monitor_disas's
0 buffer length (P3). Then cris is patched to enable monitor disas
in same way as microblaze (P4).

ARM is the harder. The vixl A64 disas was hardcoded to fprintf with
a statically inited output stream (matching target_disas). The vixl
printfery is patched to be runtime variable (P5). ARM is also missing
setting of the disas flags from monitor.c. Fixed in (P6). P7 bring
ARM monitor disassembly online (via unification as target_disas
implementation).

Two architectures remain unresolved. PPC has some diffs
between target_disas and monitor_disas. Alpha looks easier, but I have
no test case (can anyone link me a random elf or kernel? Doesn't need
to do anything meaningful other than enter execution of instructions).

Alpha sets the BFD for target_disas but not monitor_disas? Should it
just always set the BFD? That is this:

s.info.mach = bfd_mach_alpha_ev6;

PPC sets the disassembler_options to "any" but only for target_disas.

s.info.disassembler_options = (char *)"any";

Can/should this be applied to monitor disas to remove the diff?

Regards,
Peter

Peter Crosthwaite (7):
  disas: Create factored out fn for monitor and target disas
  disas: microblaze: Migrate setup to common code
  disas: cris: Fix 0 buffer length case
  disas: cris: Migrate setup to common code
  disas: arm-a64: Make printfer and stream variable
  monitor: "i": Add ARM specifics
  disas: arm: Use target_disas impl for monitor

 disas.c          | 202 ++++++++++++++++++++++++-------------------------------
 disas/arm-a64.cc |  22 ++++--
 disas/cris.c     |   6 +-
 monitor.c        |  11 +++
 4 files changed, 118 insertions(+), 123 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/7] disas: Create factored out fn for monitor and target disas

[Peter Crosthwaite]

The monitor_ and target_ disas function do mostly the same thing.
One dissambles target instructions on behalf of the log, the other
for the monitor command "xp/i" and friends.

There is a #if defined TARGET_FOO switch duplicated between both
functions and arch-specific setup for disas is copied between the two.
Factor out this duped code for arches that are consistent between
monitor and target disas, that is I386, SPARC, M68K, MIPS, SH4, S390X,
MOXIE and LM32.

No functional change.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 disas.c | 148 ++++++++++++++++++++++++++--------------------------------------
 1 file changed, 61 insertions(+), 87 deletions(-)

diff --git a/disas.c b/disas.c
index 44a019a..1c80567 100644
--- a/disas.c
+++ b/disas.c
@@ -187,6 +187,55 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
     return print_insn_objdump(pc, info, "OBJD-T");
 }
 
+static inline void
+target_disas_set_info(int (**print_insn)(bfd_vma pc, disassemble_info *info),
+                      CPUDebug *s, CPUArchState *env, int flags)
+{
+    s->env = env;
+    s->info.print_address_func = generic_print_target_address;
+
+#ifdef TARGET_WORDS_BIGENDIAN
+    s->info.endian = BFD_ENDIAN_BIG;
+#else
+    s->info.endian = BFD_ENDIAN_LITTLE;
+#endif
+#if defined(TARGET_I386)
+    if (flags == 2) {
+        s->info.mach = bfd_mach_x86_64;
+    } else if (flags == 1) {
+        s->info.mach = bfd_mach_i386_i8086;
+    } else {
+        s->info.mach = bfd_mach_i386_i386;
+    }
+    *print_insn = print_insn_i386;
+#elif defined(TARGET_SPARC)
+    *print_insn = print_insn_sparc;
+#ifdef TARGET_SPARC64
+    s->info.mach = bfd_mach_sparc_v9b;
+#endif
+#elif defined(TARGET_M68K)
+    *print_insn = print_insn_m68k;
+#elif defined(TARGET_MIPS)
+#ifdef TARGET_WORDS_BIGENDIAN
+    *print_insn = print_insn_big_mips;
+#else
+    *print_insn = print_insn_little_mips;
+#endif
+#elif defined(TARGET_SH4)
+    s->info.mach = bfd_mach_sh4;
+    *print_insn = print_insn_sh;
+#elif defined(TARGET_S390X)
+    s->info.mach = bfd_mach_s390_64;
+    *print_insn = print_insn_s390;
+#elif defined(TARGET_MOXIE)
+    s->info.mach = bfd_arch_moxie;
+    *print_insn = print_insn_moxie;
+#elif defined(TARGET_LM32)
+    s->info.mach = bfd_mach_lm32;
+    *print_insn = print_insn_lm32;
+#endif
+}
+
 /* Disassemble this for me please... (debugging). 'flags' has the following
    values:
     i386 - 1 means 16 bit code, 2 means 64 bit code
@@ -205,27 +254,13 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
 
     INIT_DISASSEMBLE_INFO(s.info, out, fprintf);
 
-    s.env = env;
+    target_disas_set_info(&print_insn, &s, env, flags);
+
     s.info.read_memory_func = target_read_memory;
     s.info.buffer_vma = code;
     s.info.buffer_length = size;
-    s.info.print_address_func = generic_print_target_address;
 
-#ifdef TARGET_WORDS_BIGENDIAN
-    s.info.endian = BFD_ENDIAN_BIG;
-#else
-    s.info.endian = BFD_ENDIAN_LITTLE;
-#endif
-#if defined(TARGET_I386)
-    if (flags == 2) {
-        s.info.mach = bfd_mach_x86_64;
-    } else if (flags == 1) {
-        s.info.mach = bfd_mach_i386_i8086;
-    } else {
-        s.info.mach = bfd_mach_i386_i386;
-    }
-    print_insn = print_insn_i386;
-#elif defined(TARGET_ARM)
+#if defined(TARGET_ARM)
     if (flags & 4) {
         /* We might not be compiled with the A64 disassembler
          * because it needs a C++ compiler; in that case we will
@@ -246,11 +281,6 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
         s.info.endian = BFD_ENDIAN_BIG;
 #endif
     }
-#elif defined(TARGET_SPARC)
-    print_insn = print_insn_sparc;
-#ifdef TARGET_SPARC64
-    s.info.mach = bfd_mach_sparc_v9b;
-#endif
 #elif defined(TARGET_PPC)
     if ((flags >> 16) & 1) {
         s.info.endian = BFD_ENDIAN_LITTLE;
@@ -267,17 +297,6 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
     }
     s.info.disassembler_options = (char *)"any";
     print_insn = print_insn_ppc;
-#elif defined(TARGET_M68K)
-    print_insn = print_insn_m68k;
-#elif defined(TARGET_MIPS)
-#ifdef TARGET_WORDS_BIGENDIAN
-    print_insn = print_insn_big_mips;
-#else
-    print_insn = print_insn_little_mips;
-#endif
-#elif defined(TARGET_SH4)
-    s.info.mach = bfd_mach_sh4;
-    print_insn = print_insn_sh;
 #elif defined(TARGET_ALPHA)
     s.info.mach = bfd_mach_alpha_ev6;
     print_insn = print_insn_alpha;
@@ -289,18 +308,9 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
         s.info.mach = bfd_mach_cris_v32;
         print_insn = print_insn_crisv32;
     }
-#elif defined(TARGET_S390X)
-    s.info.mach = bfd_mach_s390_64;
-    print_insn = print_insn_s390;
 #elif defined(TARGET_MICROBLAZE)
     s.info.mach = bfd_arch_microblaze;
     print_insn = print_insn_microblaze;
-#elif defined(TARGET_MOXIE)
-    s.info.mach = bfd_arch_moxie;
-    print_insn = print_insn_moxie;
-#elif defined(TARGET_LM32)
-    s.info.mach = bfd_mach_lm32;
-    print_insn = print_insn_lm32;
 #endif
     if (print_insn == NULL) {
         print_insn = print_insn_od_target;
@@ -452,10 +462,12 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
 {
     int count, i;
     CPUDebug s;
-    int (*print_insn)(bfd_vma pc, disassemble_info *info);
+    int (*print_insn)(bfd_vma pc, disassemble_info *info) = NULL;
 
     INIT_DISASSEMBLE_INFO(s.info, (FILE *)mon, monitor_fprintf);
 
+    target_disas_set_info(&print_insn, &s, env, flags);
+
     s.env = env;
     monitor_disas_is_physical = is_physical;
     s.info.read_memory_func = monitor_read_memory;
@@ -463,29 +475,10 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
 
     s.info.buffer_vma = pc;
 
-#ifdef TARGET_WORDS_BIGENDIAN
-    s.info.endian = BFD_ENDIAN_BIG;
-#else
-    s.info.endian = BFD_ENDIAN_LITTLE;
-#endif
-#if defined(TARGET_I386)
-    if (flags == 2) {
-        s.info.mach = bfd_mach_x86_64;
-    } else if (flags == 1) {
-        s.info.mach = bfd_mach_i386_i8086;
-    } else {
-        s.info.mach = bfd_mach_i386_i386;
-    }
-    print_insn = print_insn_i386;
-#elif defined(TARGET_ARM)
+#if defined(TARGET_ARM)
     print_insn = print_insn_arm;
 #elif defined(TARGET_ALPHA)
     print_insn = print_insn_alpha;
-#elif defined(TARGET_SPARC)
-    print_insn = print_insn_sparc;
-#ifdef TARGET_SPARC64
-    s.info.mach = bfd_mach_sparc_v9b;
-#endif
 #elif defined(TARGET_PPC)
     if (flags & 0xFFFF) {
         /* If we have a precise definition of the instruction set, use it. */
@@ -501,31 +494,12 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
         s.info.endian = BFD_ENDIAN_LITTLE;
     }
     print_insn = print_insn_ppc;
-#elif defined(TARGET_M68K)
-    print_insn = print_insn_m68k;
-#elif defined(TARGET_MIPS)
-#ifdef TARGET_WORDS_BIGENDIAN
-    print_insn = print_insn_big_mips;
-#else
-    print_insn = print_insn_little_mips;
-#endif
-#elif defined(TARGET_SH4)
-    s.info.mach = bfd_mach_sh4;
-    print_insn = print_insn_sh;
-#elif defined(TARGET_S390X)
-    s.info.mach = bfd_mach_s390_64;
-    print_insn = print_insn_s390;
-#elif defined(TARGET_MOXIE)
-    s.info.mach = bfd_arch_moxie;
-    print_insn = print_insn_moxie;
-#elif defined(TARGET_LM32)
-    s.info.mach = bfd_mach_lm32;
-    print_insn = print_insn_lm32;
-#else
-    monitor_printf(mon, "0x" TARGET_FMT_lx
-                   ": Asm output not supported on this arch\n", pc);
-    return;
 #endif
+    if (!print_insn) {
+        monitor_printf(mon, "0x" TARGET_FMT_lx
+                       ": Asm output not supported on this arch\n", pc);
+        return;
+    }
 
     for(i = 0; i < nb_insn; i++) {
 	monitor_printf(mon, "0x" TARGET_FMT_lx ":  ", pc);
-- 
1.9.1

[Qemu-devel] [PATCH 2/7] disas: microblaze: Migrate setup to common code

[Peter Crosthwaite]

Migrate the target_disas TARGET_MICROBLAZE code to common code, so
that the disassambly works for the monitor (as well the log).

E.g.
(qemu) xp 0x90000000
0000000090000000: 0x94208001

And before this patch:
(qemu) xp/i 0x90000000
0x90000000: Asm output not supported on this arch

After:
(qemu) xp/i 0x90000000
0x90000000:  mfs    r1, rmsr

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 disas.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/disas.c b/disas.c
index 1c80567..2ffef58 100644
--- a/disas.c
+++ b/disas.c
@@ -233,6 +233,9 @@ target_disas_set_info(int (**print_insn)(bfd_vma pc, disassemble_info *info),
 #elif defined(TARGET_LM32)
     s->info.mach = bfd_mach_lm32;
     *print_insn = print_insn_lm32;
+#elif defined(TARGET_MICROBLAZE)
+    s->info.mach = bfd_arch_microblaze;
+    *print_insn = print_insn_microblaze;
 #endif
 }
 
@@ -308,9 +311,6 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
         s.info.mach = bfd_mach_cris_v32;
         print_insn = print_insn_crisv32;
     }
-#elif defined(TARGET_MICROBLAZE)
-    s.info.mach = bfd_arch_microblaze;
-    print_insn = print_insn_microblaze;
 #endif
     if (print_insn == NULL) {
         print_insn = print_insn_od_target;
-- 
1.9.1

[Qemu-devel] [PATCH 3/7] disas: cris: Fix 0 buffer length case

[Peter Crosthwaite]

Cris has the complication of variable length instructions and has
a check in place to clamp memory reads in case the disas request
doesn't have enough bytes for the instruction being disas'd. This
breaks down in the case where disassembling for the monitor where
the buffer length is defaulted to 0.

The buffer length should never be zero for a regular target_disas,
so we can safely assume the 0 case is for the monitor in which case
consider the buffer length to be the max for cris instructions.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 disas/cris.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/disas/cris.c b/disas/cris.c
index e6cff7a..1b76a09 100644
--- a/disas/cris.c
+++ b/disas/cris.c
@@ -2575,9 +2575,9 @@ print_insn_cris_generic (bfd_vma memaddr,
      If we can't get any data, or we do not get enough data, we print
      the error message.  */
 
-  nbytes = info->buffer_length;
-  if (nbytes > MAX_BYTES_PER_CRIS_INSN)
-	  nbytes = MAX_BYTES_PER_CRIS_INSN;
+  nbytes = info->buffer_length ? info->buffer_length
+                               : MAX_BYTES_PER_CRIS_INSN;
+  nbytes = MIN(nbytes, MAX_BYTES_PER_CRIS_INSN);
   status = (*info->read_memory_func) (memaddr, buffer, nbytes, info);  
 
   /* If we did not get all we asked for, then clear the rest.
-- 
1.9.1

[Qemu-devel] [PATCH 4/7] disas: cris: Migrate setup to common code

[Peter Crosthwaite]

Migrate the target_disas TARGET_CRIS code to common code, so that
the disassambly works for the monitor (as well the log).

E.g.
(qemu) xp 0x40004000
0000000040004000: 0x1e6f25f0

And before this patch:
(qemu) xp/i 0x40004000
0x40004000: Asm output not supported on this arch

After:
(qemu) xp/i 0x40004000
0x40004000:  di
(qemu) xp/i 0x40004002
0x40004002:  move.d 0xb003c004,$r1

Note: second example is 6-byte misaligned instruction!

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 disas.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/disas.c b/disas.c
index 2ffef58..498b05f 100644
--- a/disas.c
+++ b/disas.c
@@ -236,6 +236,14 @@ target_disas_set_info(int (**print_insn)(bfd_vma pc, disassemble_info *info),
 #elif defined(TARGET_MICROBLAZE)
     s->info.mach = bfd_arch_microblaze;
     *print_insn = print_insn_microblaze;
+#elif defined(TARGET_CRIS)
+    if (flags != 32) {
+        s->info.mach = bfd_mach_cris_v0_v10;
+        *print_insn = print_insn_crisv10;
+    } else {
+        s->info.mach = bfd_mach_cris_v32;
+        *print_insn = print_insn_crisv32;
+    }
 #endif
 }
 
@@ -303,14 +311,6 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
 #elif defined(TARGET_ALPHA)
     s.info.mach = bfd_mach_alpha_ev6;
     print_insn = print_insn_alpha;
-#elif defined(TARGET_CRIS)
-    if (flags != 32) {
-        s.info.mach = bfd_mach_cris_v0_v10;
-        print_insn = print_insn_crisv10;
-    } else {
-        s.info.mach = bfd_mach_cris_v32;
-        print_insn = print_insn_crisv32;
-    }
 #endif
     if (print_insn == NULL) {
         print_insn = print_insn_od_target;
-- 
1.9.1

[Qemu-devel] [PATCH 5/7] disas: arm-a64: Make printfer and stream variable

[Peter Crosthwaite]

In a normal disassembly flow, the printf and stream being used varies
from disas job to job. In particular it varies if mixing monitor_disas
and target_disas.

Make both the printfer function and target stream settable in the
QEMUDisassmbler class. Remove the stream_ initialisation from the
constructor as it will now runtime vary (and an initial value won't
mean very much).

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 disas/arm-a64.cc | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/disas/arm-a64.cc b/disas/arm-a64.cc
index e04f946..d725e75 100644
--- a/disas/arm-a64.cc
+++ b/disas/arm-a64.cc
@@ -35,16 +35,25 @@ static Disassembler *vixl_disasm = NULL;
  */
 class QEMUDisassembler : public Disassembler {
 public:
-    explicit QEMUDisassembler(FILE *stream) : stream_(stream) { }
+    explicit QEMUDisassembler() { }
     ~QEMUDisassembler() { }
 
+    void SetStream(FILE *stream) {
+        stream_ = stream;
+    }
+
+    void SetPrintf(int (*printf_fn)(FILE *, const char *, ...)) {
+        printf_ = printf_fn;
+    }
+
 protected:
     virtual void ProcessOutput(const Instruction *instr) {
-        fprintf(stream_, "%08" PRIx32 "      %s",
+        printf_(stream_, "%08" PRIx32 "      %s",
                 instr->InstructionBits(), GetOutput());
     }
 
 private:
+    int (*printf_)(FILE *, const char *, ...);
     FILE *stream_;
 };
 
@@ -53,9 +62,9 @@ static int vixl_is_initialized(void)
     return vixl_decoder != NULL;
 }
 
-static void vixl_init(FILE *f) {
+static void vixl_init() {
     vixl_decoder = new Decoder();
-    vixl_disasm = new QEMUDisassembler(f);
+    vixl_disasm = new QEMUDisassembler();
     vixl_decoder->AppendVisitor(vixl_disasm);
 }
 
@@ -78,9 +87,12 @@ int print_insn_arm_a64(uint64_t addr, disassemble_info *info)
     }
 
     if (!vixl_is_initialized()) {
-        vixl_init(info->stream);
+        vixl_init();
     }
 
+    ((QEMUDisassembler *)vixl_disasm)->SetPrintf(info->fprintf_func);
+    ((QEMUDisassembler *)vixl_disasm)->SetStream(info->stream);
+
     instrval = bytes[0] | bytes[1] << 8 | bytes[2] << 16 | bytes[3] << 24;
     instr = reinterpret_cast<const Instruction *>(&instrval);
     vixl_disasm->MapCodeAddress(addr, instr);
-- 
1.9.1

[Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Peter Crosthwaite]

Add the ARM specific disassembly flags setup, so ARM can be correctly
disassembled from the monitor.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 monitor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/monitor.c b/monitor.c
index d831d98..9d9f1e2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1217,6 +1217,17 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
         int flags;
         flags = 0;
         env = mon_get_cpu();
+#ifdef TARGET_ARM
+        if (env->thumb) {
+            flags |= 1;
+        }
+        if (env->bswap_code) {
+            flags |= 2;
+        }
+        if (env->aarch64) {
+            flags |= 4;
+        }
+#endif
 #ifdef TARGET_I386
         if (wsize == 2) {
             flags = 1;
-- 
1.9.1

[Qemu-devel] [PATCH 7/7] disas: arm: Use target_disas impl for monitor

[Peter Crosthwaite]

As it is more fully featured. It has multi-endian, thumb and AArch64
support whereas the existing monitor disas support only has vanilla
AA32 support.

E.G. Running an AA64 linux kernel the follow -d in_asm disas happens
(taget_disas()):

IN:
0x0000000040000000:  580000c0      ldr x0, pc+24 (addr 0x40000018)
0x0000000040000004:  aa1f03e1      mov x1, xzr

However before this patch, disasing the same from the monitor:

(qemu) xp/i 0x40000000
0x0000000040000000:  580000c0      stmdapl  r0, {r6, r7}

After this patch:
(qemu) xp/i 0x40000000
0x0000000040000000:  580000c0      ldr x0, pc+24 (addr 0x40000018)

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 disas.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/disas.c b/disas.c
index 498b05f..e1da40d 100644
--- a/disas.c
+++ b/disas.c
@@ -208,6 +208,27 @@ target_disas_set_info(int (**print_insn)(bfd_vma pc, disassemble_info *info),
         s->info.mach = bfd_mach_i386_i386;
     }
     *print_insn = print_insn_i386;
+#elif defined(TARGET_ARM)
+    if (flags & 4) {
+        /* We might not be compiled with the A64 disassembler
+        * because it needs a C++ compiler; in that case we will
+         * fall through to the default print_insn_od case.
+         */
+#if defined(CONFIG_ARM_A64_DIS)
+        *print_insn = print_insn_arm_a64;
+#endif
+    } else if (flags & 1) {
+        *print_insn = print_insn_thumb1;
+    } else {
+        *print_insn = print_insn_arm;
+    }
+    if (flags & 2) {
+#ifdef TARGET_WORDS_BIGENDIAN
+        s->info.endian = BFD_ENDIAN_LITTLE;
+#else
+        s->info.endian = BFD_ENDIAN_BIG;
+#endif
+    }
 #elif defined(TARGET_SPARC)
     *print_insn = print_insn_sparc;
 #ifdef TARGET_SPARC64
@@ -271,28 +292,7 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
     s.info.buffer_vma = code;
     s.info.buffer_length = size;
 
-#if defined(TARGET_ARM)
-    if (flags & 4) {
-        /* We might not be compiled with the A64 disassembler
-         * because it needs a C++ compiler; in that case we will
-         * fall through to the default print_insn_od case.
-         */
-#if defined(CONFIG_ARM_A64_DIS)
-        print_insn = print_insn_arm_a64;
-#endif
-    } else if (flags & 1) {
-        print_insn = print_insn_thumb1;
-    } else {
-        print_insn = print_insn_arm;
-    }
-    if (flags & 2) {
-#ifdef TARGET_WORDS_BIGENDIAN
-        s.info.endian = BFD_ENDIAN_LITTLE;
-#else
-        s.info.endian = BFD_ENDIAN_BIG;
-#endif
-    }
-#elif defined(TARGET_PPC)
+#if defined(TARGET_PPC)
     if ((flags >> 16) & 1) {
         s.info.endian = BFD_ENDIAN_LITTLE;
     }
@@ -475,9 +475,7 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
 
     s.info.buffer_vma = pc;
 
-#if defined(TARGET_ARM)
-    print_insn = print_insn_arm;
-#elif defined(TARGET_ALPHA)
+#if defined(TARGET_ALPHA)
     print_insn = print_insn_alpha;
 #elif defined(TARGET_PPC)
     if (flags & 0xFFFF) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 7/7] disas: arm: Use target_disas impl for monitor

[Claudio Fontana]

Hello Peter,

On 05.05.2015 06:45, Peter Crosthwaite wrote:
> As it is more fully featured. It has multi-endian, thumb and AArch64
> support whereas the existing monitor disas support only has vanilla
> AA32 support.
> 
> E.G. Running an AA64 linux kernel the follow -d in_asm disas happens
> (taget_disas()):
> 
> IN:
> 0x0000000040000000:  580000c0      ldr x0, pc+24 (addr 0x40000018)
> 0x0000000040000004:  aa1f03e1      mov x1, xzr
> 
> However before this patch, disasing the same from the monitor:
> 
> (qemu) xp/i 0x40000000
> 0x0000000040000000:  580000c0      stmdapl  r0, {r6, r7}
> 
> After this patch:
> (qemu) xp/i 0x40000000
> 0x0000000040000000:  580000c0      ldr x0, pc+24 (addr 0x40000018)
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>  disas.c | 48 +++++++++++++++++++++++-------------------------
>  1 file changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/disas.c b/disas.c
> index 498b05f..e1da40d 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -208,6 +208,27 @@ target_disas_set_info(int (**print_insn)(bfd_vma pc, disassemble_info *info),
>          s->info.mach = bfd_mach_i386_i386;
>      }
>      *print_insn = print_insn_i386;
> +#elif defined(TARGET_ARM)
> +    if (flags & 4) {
> +        /* We might not be compiled with the A64 disassembler
> +        * because it needs a C++ compiler; in that case we will

nit: misaligned * (add one space)

for all the rest, I didn't notice any problems.

Regarding the libvixl output, I still have a lot of unimplemented instructions..
is it possible to improve libvixl to dissect system instructions?
I guess this question is more for the libvixl project itself.

Looking at my disassembly I just tested I have:

0x00000000400d08a0:   d50342df      unimplemented (System)
0x00000000400d08a4:   d5033fdf      isb
0x00000000400d08a8:   d503207f      unimplemented (System)
0x00000000400d08ac:   d503207f      unimplemented (System)

Tested-by: Claudio Fontana <claudio.fontana@huawei.com>
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>

> +         * fall through to the default print_insn_od case.
> +         */
> +#if defined(CONFIG_ARM_A64_DIS)
> +        *print_insn = print_insn_arm_a64;
> +#endif
> +    } else if (flags & 1) {
> +        *print_insn = print_insn_thumb1;
> +    } else {
> +        *print_insn = print_insn_arm;
> +    }
> +    if (flags & 2) {
> +#ifdef TARGET_WORDS_BIGENDIAN
> +        s->info.endian = BFD_ENDIAN_LITTLE;
> +#else
> +        s->info.endian = BFD_ENDIAN_BIG;
> +#endif
> +    }
>  #elif defined(TARGET_SPARC)
>      *print_insn = print_insn_sparc;
>  #ifdef TARGET_SPARC64
> @@ -271,28 +292,7 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
>      s.info.buffer_vma = code;
>      s.info.buffer_length = size;
>  
> -#if defined(TARGET_ARM)
> -    if (flags & 4) {
> -        /* We might not be compiled with the A64 disassembler
> -         * because it needs a C++ compiler; in that case we will
> -         * fall through to the default print_insn_od case.
> -         */
> -#if defined(CONFIG_ARM_A64_DIS)
> -        print_insn = print_insn_arm_a64;
> -#endif
> -    } else if (flags & 1) {
> -        print_insn = print_insn_thumb1;
> -    } else {
> -        print_insn = print_insn_arm;
> -    }
> -    if (flags & 2) {
> -#ifdef TARGET_WORDS_BIGENDIAN
> -        s.info.endian = BFD_ENDIAN_LITTLE;
> -#else
> -        s.info.endian = BFD_ENDIAN_BIG;
> -#endif
> -    }
> -#elif defined(TARGET_PPC)
> +#if defined(TARGET_PPC)
>      if ((flags >> 16) & 1) {
>          s.info.endian = BFD_ENDIAN_LITTLE;
>      }
> @@ -475,9 +475,7 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>  
>      s.info.buffer_vma = pc;
>  
> -#if defined(TARGET_ARM)
> -    print_insn = print_insn_arm;
> -#elif defined(TARGET_ALPHA)
> +#if defined(TARGET_ALPHA)
>      print_insn = print_insn_alpha;
>  #elif defined(TARGET_PPC)
>      if (flags & 0xFFFF) {
> 


-- 
Claudio Fontana
Server Virtualization Architect
Huawei Technologies Duesseldorf GmbH
Riesstraße 25 - 80992 München

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Claudio Fontana]

On 05.05.2015 06:45, Peter Crosthwaite wrote:
> Add the ARM specific disassembly flags setup, so ARM can be correctly
> disassembled from the monitor.
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>  monitor.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/monitor.c b/monitor.c
> index d831d98..9d9f1e2 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1217,6 +1217,17 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>          int flags;
>          flags = 0;
>          env = mon_get_cpu();
> +#ifdef TARGET_ARM
> +        if (env->thumb) {
> +            flags |= 1;
> +        }
> +        if (env->bswap_code) {
> +            flags |= 2;
> +        }
> +        if (env->aarch64) {
> +            flags |= 4;
> +        }
> +#endif
>  #ifdef TARGET_I386
>          if (wsize == 2) {
>              flags = 1;
> 

Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
Tested-by: Claudio Fontana <claudio.fontana@huawei.com>

Re: [Qemu-devel] [PATCH 5/7] disas: arm-a64: Make printfer and stream variable

[Claudio Fontana]

On 05.05.2015 06:45, Peter Crosthwaite wrote:
> In a normal disassembly flow, the printf and stream being used varies
> from disas job to job. In particular it varies if mixing monitor_disas
> and target_disas.
> 
> Make both the printfer function and target stream settable in the
> QEMUDisassmbler class. Remove the stream_ initialisation from the
> constructor as it will now runtime vary (and an initial value won't
> mean very much).
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>  disas/arm-a64.cc | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/disas/arm-a64.cc b/disas/arm-a64.cc
> index e04f946..d725e75 100644
> --- a/disas/arm-a64.cc
> +++ b/disas/arm-a64.cc
> @@ -35,16 +35,25 @@ static Disassembler *vixl_disasm = NULL;
>   */
>  class QEMUDisassembler : public Disassembler {
>  public:
> -    explicit QEMUDisassembler(FILE *stream) : stream_(stream) { }
> +    explicit QEMUDisassembler() { }
>      ~QEMUDisassembler() { }
>  
> +    void SetStream(FILE *stream) {
> +        stream_ = stream;
> +    }
> +
> +    void SetPrintf(int (*printf_fn)(FILE *, const char *, ...)) {
> +        printf_ = printf_fn;
> +    }
> +
>  protected:
>      virtual void ProcessOutput(const Instruction *instr) {
> -        fprintf(stream_, "%08" PRIx32 "      %s",
> +        printf_(stream_, "%08" PRIx32 "      %s",
>                  instr->InstructionBits(), GetOutput());
>      }
>  
>  private:
> +    int (*printf_)(FILE *, const char *, ...);
>      FILE *stream_;
>  };
>  
> @@ -53,9 +62,9 @@ static int vixl_is_initialized(void)
>      return vixl_decoder != NULL;
>  }
>  
> -static void vixl_init(FILE *f) {
> +static void vixl_init() {
>      vixl_decoder = new Decoder();
> -    vixl_disasm = new QEMUDisassembler(f);
> +    vixl_disasm = new QEMUDisassembler();
>      vixl_decoder->AppendVisitor(vixl_disasm);
>  }
>  
> @@ -78,9 +87,12 @@ int print_insn_arm_a64(uint64_t addr, disassemble_info *info)
>      }
>  
>      if (!vixl_is_initialized()) {
> -        vixl_init(info->stream);
> +        vixl_init();
>      }
>  
> +    ((QEMUDisassembler *)vixl_disasm)->SetPrintf(info->fprintf_func);
> +    ((QEMUDisassembler *)vixl_disasm)->SetStream(info->stream);
> +
>      instrval = bytes[0] | bytes[1] << 8 | bytes[2] << 16 | bytes[3] << 24;
>      instr = reinterpret_cast<const Instruction *>(&instrval);
>      vixl_disasm->MapCodeAddress(addr, instr);
> 

Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
Tested-by: Claudio Fontana <claudio.fontana@huawei.com>

Re: [Qemu-devel] [PATCH 1/7] disas: Create factored out fn for monitor and target disas

[Claudio Fontana]

On 05.05.2015 06:44, Peter Crosthwaite wrote:
> The monitor_ and target_ disas function do mostly the same thing.
> One dissambles target instructions on behalf of the log, the other
> for the monitor command "xp/i" and friends.
> 
> There is a #if defined TARGET_FOO switch duplicated between both
> functions and arch-specific setup for disas is copied between the two.
> Factor out this duped code for arches that are consistent between
> monitor and target disas, that is I386, SPARC, M68K, MIPS, SH4, S390X,
> MOXIE and LM32.
> 
> No functional change.
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>  disas.c | 148 ++++++++++++++++++++++++++--------------------------------------
>  1 file changed, 61 insertions(+), 87 deletions(-)
> 
> diff --git a/disas.c b/disas.c
> index 44a019a..1c80567 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -187,6 +187,55 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
>      return print_insn_objdump(pc, info, "OBJD-T");
>  }
>  
> +static inline void
> +target_disas_set_info(int (**print_insn)(bfd_vma pc, disassemble_info *info),
> +                      CPUDebug *s, CPUArchState *env, int flags)
> +{
> +    s->env = env;
> +    s->info.print_address_func = generic_print_target_address;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    s->info.endian = BFD_ENDIAN_BIG;
> +#else
> +    s->info.endian = BFD_ENDIAN_LITTLE;
> +#endif
> +#if defined(TARGET_I386)
> +    if (flags == 2) {
> +        s->info.mach = bfd_mach_x86_64;
> +    } else if (flags == 1) {
> +        s->info.mach = bfd_mach_i386_i8086;
> +    } else {
> +        s->info.mach = bfd_mach_i386_i386;
> +    }
> +    *print_insn = print_insn_i386;
> +#elif defined(TARGET_SPARC)
> +    *print_insn = print_insn_sparc;
> +#ifdef TARGET_SPARC64
> +    s->info.mach = bfd_mach_sparc_v9b;
> +#endif
> +#elif defined(TARGET_M68K)
> +    *print_insn = print_insn_m68k;
> +#elif defined(TARGET_MIPS)
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    *print_insn = print_insn_big_mips;
> +#else
> +    *print_insn = print_insn_little_mips;
> +#endif
> +#elif defined(TARGET_SH4)
> +    s->info.mach = bfd_mach_sh4;
> +    *print_insn = print_insn_sh;
> +#elif defined(TARGET_S390X)
> +    s->info.mach = bfd_mach_s390_64;
> +    *print_insn = print_insn_s390;
> +#elif defined(TARGET_MOXIE)
> +    s->info.mach = bfd_arch_moxie;
> +    *print_insn = print_insn_moxie;
> +#elif defined(TARGET_LM32)
> +    s->info.mach = bfd_mach_lm32;
> +    *print_insn = print_insn_lm32;
> +#endif
> +}
> +
>  /* Disassemble this for me please... (debugging). 'flags' has the following
>     values:
>      i386 - 1 means 16 bit code, 2 means 64 bit code
> @@ -205,27 +254,13 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
>  
>      INIT_DISASSEMBLE_INFO(s.info, out, fprintf);
>  
> -    s.env = env;
> +    target_disas_set_info(&print_insn, &s, env, flags);
> +
>      s.info.read_memory_func = target_read_memory;
>      s.info.buffer_vma = code;
>      s.info.buffer_length = size;
> -    s.info.print_address_func = generic_print_target_address;
>  
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    s.info.endian = BFD_ENDIAN_BIG;
> -#else
> -    s.info.endian = BFD_ENDIAN_LITTLE;
> -#endif
> -#if defined(TARGET_I386)
> -    if (flags == 2) {
> -        s.info.mach = bfd_mach_x86_64;
> -    } else if (flags == 1) {
> -        s.info.mach = bfd_mach_i386_i8086;
> -    } else {
> -        s.info.mach = bfd_mach_i386_i386;
> -    }
> -    print_insn = print_insn_i386;
> -#elif defined(TARGET_ARM)
> +#if defined(TARGET_ARM)
>      if (flags & 4) {
>          /* We might not be compiled with the A64 disassembler
>           * because it needs a C++ compiler; in that case we will
> @@ -246,11 +281,6 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
>          s.info.endian = BFD_ENDIAN_BIG;
>  #endif
>      }
> -#elif defined(TARGET_SPARC)
> -    print_insn = print_insn_sparc;
> -#ifdef TARGET_SPARC64
> -    s.info.mach = bfd_mach_sparc_v9b;
> -#endif
>  #elif defined(TARGET_PPC)
>      if ((flags >> 16) & 1) {
>          s.info.endian = BFD_ENDIAN_LITTLE;
> @@ -267,17 +297,6 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
>      }
>      s.info.disassembler_options = (char *)"any";
>      print_insn = print_insn_ppc;
> -#elif defined(TARGET_M68K)
> -    print_insn = print_insn_m68k;
> -#elif defined(TARGET_MIPS)
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    print_insn = print_insn_big_mips;
> -#else
> -    print_insn = print_insn_little_mips;
> -#endif
> -#elif defined(TARGET_SH4)
> -    s.info.mach = bfd_mach_sh4;
> -    print_insn = print_insn_sh;
>  #elif defined(TARGET_ALPHA)
>      s.info.mach = bfd_mach_alpha_ev6;
>      print_insn = print_insn_alpha;
> @@ -289,18 +308,9 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
>          s.info.mach = bfd_mach_cris_v32;
>          print_insn = print_insn_crisv32;
>      }
> -#elif defined(TARGET_S390X)
> -    s.info.mach = bfd_mach_s390_64;
> -    print_insn = print_insn_s390;
>  #elif defined(TARGET_MICROBLAZE)
>      s.info.mach = bfd_arch_microblaze;
>      print_insn = print_insn_microblaze;
> -#elif defined(TARGET_MOXIE)
> -    s.info.mach = bfd_arch_moxie;
> -    print_insn = print_insn_moxie;
> -#elif defined(TARGET_LM32)
> -    s.info.mach = bfd_mach_lm32;
> -    print_insn = print_insn_lm32;
>  #endif
>      if (print_insn == NULL) {
>          print_insn = print_insn_od_target;
> @@ -452,10 +462,12 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>  {
>      int count, i;
>      CPUDebug s;
> -    int (*print_insn)(bfd_vma pc, disassemble_info *info);
> +    int (*print_insn)(bfd_vma pc, disassemble_info *info) = NULL;
>  
>      INIT_DISASSEMBLE_INFO(s.info, (FILE *)mon, monitor_fprintf);
>  
> +    target_disas_set_info(&print_insn, &s, env, flags);
> +
>      s.env = env;
>      monitor_disas_is_physical = is_physical;
>      s.info.read_memory_func = monitor_read_memory;
> @@ -463,29 +475,10 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>  
>      s.info.buffer_vma = pc;
>  
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    s.info.endian = BFD_ENDIAN_BIG;
> -#else
> -    s.info.endian = BFD_ENDIAN_LITTLE;
> -#endif
> -#if defined(TARGET_I386)
> -    if (flags == 2) {
> -        s.info.mach = bfd_mach_x86_64;
> -    } else if (flags == 1) {
> -        s.info.mach = bfd_mach_i386_i8086;
> -    } else {
> -        s.info.mach = bfd_mach_i386_i386;
> -    }
> -    print_insn = print_insn_i386;
> -#elif defined(TARGET_ARM)
> +#if defined(TARGET_ARM)
>      print_insn = print_insn_arm;
>  #elif defined(TARGET_ALPHA)
>      print_insn = print_insn_alpha;
> -#elif defined(TARGET_SPARC)
> -    print_insn = print_insn_sparc;
> -#ifdef TARGET_SPARC64
> -    s.info.mach = bfd_mach_sparc_v9b;
> -#endif
>  #elif defined(TARGET_PPC)
>      if (flags & 0xFFFF) {
>          /* If we have a precise definition of the instruction set, use it. */
> @@ -501,31 +494,12 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>          s.info.endian = BFD_ENDIAN_LITTLE;
>      }
>      print_insn = print_insn_ppc;
> -#elif defined(TARGET_M68K)
> -    print_insn = print_insn_m68k;
> -#elif defined(TARGET_MIPS)
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    print_insn = print_insn_big_mips;
> -#else
> -    print_insn = print_insn_little_mips;
> -#endif
> -#elif defined(TARGET_SH4)
> -    s.info.mach = bfd_mach_sh4;
> -    print_insn = print_insn_sh;
> -#elif defined(TARGET_S390X)
> -    s.info.mach = bfd_mach_s390_64;
> -    print_insn = print_insn_s390;
> -#elif defined(TARGET_MOXIE)
> -    s.info.mach = bfd_arch_moxie;
> -    print_insn = print_insn_moxie;
> -#elif defined(TARGET_LM32)
> -    s.info.mach = bfd_mach_lm32;
> -    print_insn = print_insn_lm32;
> -#else
> -    monitor_printf(mon, "0x" TARGET_FMT_lx
> -                   ": Asm output not supported on this arch\n", pc);
> -    return;
>  #endif
> +    if (!print_insn) {
> +        monitor_printf(mon, "0x" TARGET_FMT_lx
> +                       ": Asm output not supported on this arch\n", pc);
> +        return;
> +    }
>  
>      for(i = 0; i < nb_insn; i++) {
>  	monitor_printf(mon, "0x" TARGET_FMT_lx ":  ", pc);
> 

Note: tested only the ARMv8 path of this one.

Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>

Re: [Qemu-devel] [PATCH 7/7] disas: arm: Use target_disas impl for monitor

[Peter Crosthwaite]

On Tue, May 5, 2015 at 7:38 AM, Claudio Fontana
<claudio.fontana@huawei.com> wrote:
> Hello Peter,
>
> On 05.05.2015 06:45, Peter Crosthwaite wrote:
>> As it is more fully featured. It has multi-endian, thumb and AArch64
>> support whereas the existing monitor disas support only has vanilla
>> AA32 support.
>>
>> E.G. Running an AA64 linux kernel the follow -d in_asm disas happens
>> (taget_disas()):
>>
>> IN:
>> 0x0000000040000000:  580000c0      ldr x0, pc+24 (addr 0x40000018)
>> 0x0000000040000004:  aa1f03e1      mov x1, xzr
>>
>> However before this patch, disasing the same from the monitor:
>>
>> (qemu) xp/i 0x40000000
>> 0x0000000040000000:  580000c0      stmdapl  r0, {r6, r7}
>>
>> After this patch:
>> (qemu) xp/i 0x40000000
>> 0x0000000040000000:  580000c0      ldr x0, pc+24 (addr 0x40000018)
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>>  disas.c | 48 +++++++++++++++++++++++-------------------------
>>  1 file changed, 23 insertions(+), 25 deletions(-)
>>
>> diff --git a/disas.c b/disas.c
>> index 498b05f..e1da40d 100644
>> --- a/disas.c
>> +++ b/disas.c
>> @@ -208,6 +208,27 @@ target_disas_set_info(int (**print_insn)(bfd_vma pc, disassemble_info *info),
>>          s->info.mach = bfd_mach_i386_i386;
>>      }
>>      *print_insn = print_insn_i386;
>> +#elif defined(TARGET_ARM)
>> +    if (flags & 4) {
>> +        /* We might not be compiled with the A64 disassembler
>> +        * because it needs a C++ compiler; in that case we will
>
> nit: misaligned * (add one space)
>

Will fix.

> for all the rest, I didn't notice any problems.
>

Thanks for the test.

> Regarding the libvixl output, I still have a lot of unimplemented instructions..
> is it possible to improve libvixl to dissect system instructions?
> I guess this question is more for the libvixl project itself.
>

So my project (and coding time) is currently going is a different
direction. My main motivation here is to QOMify the disas code path to
remove the target specific compilation of the disaser. This series is
the minimal cleanup and functional changes before diving into the
bigger refactor.

Is the GSOC event happening again and should we list this as a project?

Regards,
Peter

> Looking at my disassembly I just tested I have:
>
> 0x00000000400d08a0:   d50342df      unimplemented (System)
> 0x00000000400d08a4:   d5033fdf      isb
> 0x00000000400d08a8:   d503207f      unimplemented (System)
> 0x00000000400d08ac:   d503207f      unimplemented (System)
>
> Tested-by: Claudio Fontana <claudio.fontana@huawei.com>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
>
>> +         * fall through to the default print_insn_od case.
>> +         */
>> +#if defined(CONFIG_ARM_A64_DIS)
>> +        *print_insn = print_insn_arm_a64;
>> +#endif
>> +    } else if (flags & 1) {
>> +        *print_insn = print_insn_thumb1;
>> +    } else {
>> +        *print_insn = print_insn_arm;
>> +    }
>> +    if (flags & 2) {
>> +#ifdef TARGET_WORDS_BIGENDIAN
>> +        s->info.endian = BFD_ENDIAN_LITTLE;
>> +#else
>> +        s->info.endian = BFD_ENDIAN_BIG;
>> +#endif
>> +    }
>>  #elif defined(TARGET_SPARC)
>>      *print_insn = print_insn_sparc;
>>  #ifdef TARGET_SPARC64
>> @@ -271,28 +292,7 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
>>      s.info.buffer_vma = code;
>>      s.info.buffer_length = size;
>>
>> -#if defined(TARGET_ARM)
>> -    if (flags & 4) {
>> -        /* We might not be compiled with the A64 disassembler
>> -         * because it needs a C++ compiler; in that case we will
>> -         * fall through to the default print_insn_od case.
>> -         */
>> -#if defined(CONFIG_ARM_A64_DIS)
>> -        print_insn = print_insn_arm_a64;
>> -#endif
>> -    } else if (flags & 1) {
>> -        print_insn = print_insn_thumb1;
>> -    } else {
>> -        print_insn = print_insn_arm;
>> -    }
>> -    if (flags & 2) {
>> -#ifdef TARGET_WORDS_BIGENDIAN
>> -        s.info.endian = BFD_ENDIAN_LITTLE;
>> -#else
>> -        s.info.endian = BFD_ENDIAN_BIG;
>> -#endif
>> -    }
>> -#elif defined(TARGET_PPC)
>> +#if defined(TARGET_PPC)
>>      if ((flags >> 16) & 1) {
>>          s.info.endian = BFD_ENDIAN_LITTLE;
>>      }
>> @@ -475,9 +475,7 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>>
>>      s.info.buffer_vma = pc;
>>
>> -#if defined(TARGET_ARM)
>> -    print_insn = print_insn_arm;
>> -#elif defined(TARGET_ALPHA)
>> +#if defined(TARGET_ALPHA)
>>      print_insn = print_insn_alpha;
>>  #elif defined(TARGET_PPC)
>>      if (flags & 0xFFFF) {
>>
>
>
> --
> Claudio Fontana
> Server Virtualization Architect
> Huawei Technologies Duesseldorf GmbH
> Riesstraße 25 - 80992 München
>
>

Re: [Qemu-devel] [PATCH 0/7] disas: Unify target_disas and monitor_disas

[Richard Henderson]

On 05/04/2015 09:44 PM, Peter Crosthwaite wrote:
> Alpha looks easier, but I have
> no test case (can anyone link me a random elf or kernel? Doesn't need
> to do anything meaningful other than enter execution of instructions).

If all you need is some executed instructions, the included bios image will do.


$ ./alpha-softmmu/qemu-system-alpha -nographic
PCI: 00:00:0 class 0300 id 1013:00b8
PCI:   region 0: 10000000
PCI:   region 1: 12000000
PCI: 00:01:0 class 0200 id 8086:100e
PCI:   region 0: 12020000
PCI:   region 1: 0000c000
PCI: 00:02:0 class 0101 id 1095:0646
PCI:   region 0: 0000c040
PCI:   region 1: 0000c048
PCI:   region 3: 0000c04c
>>>



r~

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Peter Maydell]

On 5 May 2015 at 05:45, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> Add the ARM specific disassembly flags setup, so ARM can be correctly
> disassembled from the monitor.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>  monitor.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index d831d98..9d9f1e2 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1217,6 +1217,17 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>          int flags;
>          flags = 0;
>          env = mon_get_cpu();
> +#ifdef TARGET_ARM
> +        if (env->thumb) {
> +            flags |= 1;
> +        }
> +        if (env->bswap_code) {
> +            flags |= 2;
> +        }
> +        if (env->aarch64) {
> +            flags |= 4;
> +        }
> +#endif

monitor.c has no business poking around in the CPU state
internals like this... You probably want a CPU method
get_disas_flags() or something.

-- PMM

Re: [Qemu-devel] [PATCH 5/7] disas: arm-a64: Make printfer and stream variable

[Richard Henderson]

On 05/04/2015 09:45 PM, Peter Crosthwaite wrote:
> -    explicit QEMUDisassembler(FILE *stream) : stream_(stream) { }
> +    explicit QEMUDisassembler() { }

The explicit is no longer helpful, and I'd still initialize stream_ to null.


r~

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Richard Henderson]

On 05/05/2015 10:19 AM, Peter Maydell wrote:
> On 5 May 2015 at 05:45, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
>> Add the ARM specific disassembly flags setup, so ARM can be correctly
>> disassembled from the monitor.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>>  monitor.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index d831d98..9d9f1e2 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1217,6 +1217,17 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>          int flags;
>>          flags = 0;
>>          env = mon_get_cpu();
>> +#ifdef TARGET_ARM
>> +        if (env->thumb) {
>> +            flags |= 1;
>> +        }
>> +        if (env->bswap_code) {
>> +            flags |= 2;
>> +        }
>> +        if (env->aarch64) {
>> +            flags |= 4;
>> +        }
>> +#endif
> 
> monitor.c has no business poking around in the CPU state
> internals like this... You probably want a CPU method
> get_disas_flags() or something.
> 
> -- PMM
> 

While this patch set does improve the current dismal state of affairs, I think
the ideal solution is a cpu method that takes care of all the disassembly info
setup.

Indeed, the flags setup becomes less obscure when, instead of

#ifdef TARGET_I386
        if (wsize == 2) {
            flags = 1;
        } else if (wsize == 4) {
            flags = 0;
        } else {
            /* as default we use the current CS size */
            flags = 0;
            if (env) {
#ifdef TARGET_X86_64
                if ((env->efer & MSR_EFER_LMA) &&
                    (env->segs[R_CS].flags & DESC_L_MASK))
                    flags = 2;
                else
#endif
                if (!(env->segs[R_CS].flags & DESC_B_MASK))
                    flags = 1;
            }
        }

in one place and

#if defined(TARGET_I386)
    if (flags == 2) {
        s.info.mach = bfd_mach_x86_64;
    } else if (flags == 1) {
        s.info.mach = bfd_mach_i386_i8086;
    } else {
        s.info.mach = bfd_mach_i386_i386;
    }
    print_insn = print_insn_i386;

in another, we merge the two so that we get

    s.info.mach = bfd_mach_i386_i8086;
    if (env->hflags & (1U << HF_CS32_SHIFT)) {
        s.info.mach = bfd_mach_i386_i386;
    }
#ifdef TARGET_X86_64
    if (env->hflags & (1U << HF_CS64_SHIFT)) {
        s.info.mach = bfd_mach_x86_64;
    }
#endif


I'm not sure what the right interface for this is, exactly.  But I'd think that
the CPUDebug structure would be initialized in the caller -- target or monitor
-- while the mach and whatnot get filled in by the cpu hook.  Maybe even have
the cpu hook return the print_insn function to use.



r~

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Peter Crosthwaite]

On Tue, May 5, 2015 at 10:43 AM, Richard Henderson <rth@twiddle.net> wrote:
> On 05/05/2015 10:19 AM, Peter Maydell wrote:
>> On 5 May 2015 at 05:45, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
>>> Add the ARM specific disassembly flags setup, so ARM can be correctly
>>> disassembled from the monitor.
>>>
>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>> ---
>>>  monitor.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index d831d98..9d9f1e2 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -1217,6 +1217,17 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>>          int flags;
>>>          flags = 0;
>>>          env = mon_get_cpu();
>>> +#ifdef TARGET_ARM
>>> +        if (env->thumb) {
>>> +            flags |= 1;
>>> +        }
>>> +        if (env->bswap_code) {
>>> +            flags |= 2;
>>> +        }
>>> +        if (env->aarch64) {
>>> +            flags |= 4;
>>> +        }
>>> +#endif
>>
>> monitor.c has no business poking around in the CPU state
>> internals like this... You probably want a CPU method
>> get_disas_flags() or something.
>>
>> -- PMM
>>
>
> While this patch set does improve the current dismal state of affairs, I think
> the ideal solution is a cpu method that takes care of all the disassembly info
> setup.
>

So I have made a start on this. The ARM, MB and CRIS in this patch
series is rather easy. Its X86 im having trouble with but your example
here looks like most of the work ...

> Indeed, the flags setup becomes less obscure when, instead of
>
> #ifdef TARGET_I386
>         if (wsize == 2) {
>             flags = 1;
>         } else if (wsize == 4) {
>             flags = 0;
>         } else {

So here the monitor is actually using the argument memory-dump size to
dictate the flags. Is this flawed and should we delete this wsize
if-else and rely on the CPU-state driven logic for correct disas info
selection? This wsize reliance seems unique to x86. I think we would
have to give this up in a QOMified approach.

>             /* as default we use the current CS size */
>             flags = 0;
>             if (env) {
> #ifdef TARGET_X86_64
>                 if ((env->efer & MSR_EFER_LMA) &&
>                     (env->segs[R_CS].flags & DESC_L_MASK))

This uses env->efer and segs to drive the flags...

>                     flags = 2;
>                 else
> #endif
>                 if (!(env->segs[R_CS].flags & DESC_B_MASK))
>                     flags = 1;
>             }
>         }
>
> in one place and
>
> #if defined(TARGET_I386)
>     if (flags == 2) {
>         s.info.mach = bfd_mach_x86_64;
>     } else if (flags == 1) {
>         s.info.mach = bfd_mach_i386_i8086;
>     } else {
>         s.info.mach = bfd_mach_i386_i386;
>     }
>     print_insn = print_insn_i386;
>
> in another, we merge the two so that we get
>
>     s.info.mach = bfd_mach_i386_i8086;
>     if (env->hflags & (1U << HF_CS32_SHIFT)) {

But your new implementation uses hflags. Are they the same state? I
couldnt find easy correltation between MSR_EFER_LMA and HF_CSXX_SHIFT
(although I do see that map to hflags HF_LMA?).

Is your code a functional change?

>         s.info.mach = bfd_mach_i386_i386;
>     }
> #ifdef TARGET_X86_64
>     if (env->hflags & (1U << HF_CS64_SHIFT)) {
>         s.info.mach = bfd_mach_x86_64;
>     }
> #endif
>
>
> I'm not sure what the right interface for this is, exactly.  But I'd think that
> the CPUDebug structure would be initialized in the caller -- target or monitor
> -- while the mach and whatnot get filled in by the cpu hook.  Maybe even have
> the cpu hook return the print_insn function to use.
>

I went for adding print_insn to disassembly_info and passing just that
to the hook. Patches soon! I might leave X86 out for the first spin.

Regards,
Peter

>
>
> r~
>

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Peter Crosthwaite]

On Tue, May 5, 2015 at 10:43 AM, Richard Henderson <rth@twiddle.net> wrote:
> On 05/05/2015 10:19 AM, Peter Maydell wrote:
>> On 5 May 2015 at 05:45, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
>>> Add the ARM specific disassembly flags setup, so ARM can be correctly
>>> disassembled from the monitor.
>>>
>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>> ---
>>>  monitor.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index d831d98..9d9f1e2 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -1217,6 +1217,17 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>>          int flags;
>>>          flags = 0;
>>>          env = mon_get_cpu();
>>> +#ifdef TARGET_ARM
>>> +        if (env->thumb) {
>>> +            flags |= 1;
>>> +        }
>>> +        if (env->bswap_code) {
>>> +            flags |= 2;
>>> +        }
>>> +        if (env->aarch64) {
>>> +            flags |= 4;
>>> +        }
>>> +#endif
>>
>> monitor.c has no business poking around in the CPU state
>> internals like this... You probably want a CPU method
>> get_disas_flags() or something.
>>
>> -- PMM
>>
>
> While this patch set does improve the current dismal state of affairs, I think
> the ideal solution is a cpu method that takes care of all the disassembly info
> setup.
>
> Indeed, the flags setup becomes less obscure when, instead of
>
> #ifdef TARGET_I386
>         if (wsize == 2) {
>             flags = 1;
>         } else if (wsize == 4) {
>             flags = 0;
>         } else {
>             /* as default we use the current CS size */
>             flags = 0;
>             if (env) {
> #ifdef TARGET_X86_64
>                 if ((env->efer & MSR_EFER_LMA) &&
>                     (env->segs[R_CS].flags & DESC_L_MASK))
>                     flags = 2;
>                 else
> #endif
>                 if (!(env->segs[R_CS].flags & DESC_B_MASK))
>                     flags = 1;
>             }
>         }
>
> in one place and
>
> #if defined(TARGET_I386)
>     if (flags == 2) {
>         s.info.mach = bfd_mach_x86_64;
>     } else if (flags == 1) {
>         s.info.mach = bfd_mach_i386_i8086;
>     } else {
>         s.info.mach = bfd_mach_i386_i386;
>     }
>     print_insn = print_insn_i386;
>
> in another, we merge the two so that we get
>
>     s.info.mach = bfd_mach_i386_i8086;
>     if (env->hflags & (1U << HF_CS32_SHIFT)) {
>         s.info.mach = bfd_mach_i386_i386;
>     }
> #ifdef TARGET_X86_64
>     if (env->hflags & (1U << HF_CS64_SHIFT)) {
>         s.info.mach = bfd_mach_x86_64;
>     }
> #endif
>
>
> I'm not sure what the right interface for this is, exactly.  But I'd think that
> the CPUDebug structure would be initialized in the caller -- target or monitor
> -- while the mach and whatnot get filled in by the cpu hook.  Maybe even have
> the cpu hook return the print_insn function to use.
>

So something else I just thought of and started worrying about, is the
the target_disas path is driving some of the flags from the disas
context, whereas a hook will only have access to the CPUState/env. Can
we rely on the env/CPUState always being up to date during
target_disas (which happens at translate time?) or will we need to go
field by field to make sure any env updates explicitly occur before
target_disas?

Regards,
Peter

>
>
> r~
>

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Richard Henderson]

On 05/05/2015 11:57 PM, Peter Crosthwaite wrote:
> So I have made a start on this. The ARM, MB and CRIS in this patch
> series is rather easy. Its X86 im having trouble with but your example
> here looks like most of the work ...
> 
>> Indeed, the flags setup becomes less obscure when, instead of
>>
>> #ifdef TARGET_I386
>>         if (wsize == 2) {
>>             flags = 1;
>>         } else if (wsize == 4) {
>>             flags = 0;
>>         } else {
> 
> So here the monitor is actually using the argument memory-dump size to
> dictate the flags. Is this flawed and should we delete this wsize
> if-else and rely on the CPU-state driven logic for correct disas info
> selection? This wsize reliance seems unique to x86. I think we would
> have to give this up in a QOMified approach.

Hmm.  I don't think that I've ever noticed the monitor disassembly could do
that.  If I were going to do that kind of debugging I certainly wouldn't use
the monitor -- I'd use gdb.  ;-)

If someone thinks we ought to keep that feature, speak now...

>>             /* as default we use the current CS size */
>>             flags = 0;
>>             if (env) {
>> #ifdef TARGET_X86_64
>>                 if ((env->efer & MSR_EFER_LMA) &&
>>                     (env->segs[R_CS].flags & DESC_L_MASK))
> 
> This uses env->efer and segs to drive the flags...
> 
>>                     flags = 2;
>>                 else
>> #endif
>>                 if (!(env->segs[R_CS].flags & DESC_B_MASK))
>>                     flags = 1;
>>             }
>>         }
>>
>> in one place and
>>
>> #if defined(TARGET_I386)
>>     if (flags == 2) {
>>         s.info.mach = bfd_mach_x86_64;
>>     } else if (flags == 1) {
>>         s.info.mach = bfd_mach_i386_i8086;
>>     } else {
>>         s.info.mach = bfd_mach_i386_i386;
>>     }
>>     print_insn = print_insn_i386;
>>
>> in another, we merge the two so that we get
>>
>>     s.info.mach = bfd_mach_i386_i8086;
>>     if (env->hflags & (1U << HF_CS32_SHIFT)) {
> 
> But your new implementation uses hflags. Are they the same state? I
> couldnt find easy correltation between MSR_EFER_LMA and HF_CSXX_SHIFT
> (although I do see that map to hflags HF_LMA?).
> 
> Is your code a functional change?

It's not intended to be.  Since I couldn't find where wsize was initialized, I
pulled the tests used by target-i386/translator.c, for dc->code32 and
dc->code64, since I knew where to find them right away.  ;-)

Without going back to the manuals, I don't know the difference between CS64 and
LMA; from the code it appears only the behaviour of sysret, which seems surprising.

> I went for adding print_insn to disassembly_info and passing just that
> to the hook. Patches soon! I might leave X86 out for the first spin.

Sounds good.


r~

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Richard Henderson]

On 05/06/2015 12:06 AM, Peter Crosthwaite wrote:
> Can
> we rely on the env/CPUState always being up to date during
> target_disas (which happens at translate time?) or will we need to go
> field by field to make sure any env updates explicitly occur before
> target_disas?

I *think* so, but it's a near thing.  The path goes

 tb_find_fast:
  cpu_get_tb_cpu_state, fill fill in flags for TB from current ENV state.
  tb_find_slow,
    tb_gen_code, using those same flags.

There's the edge case of re-translation, but I'm going to assert that cpu mode
changes ought not happen in that context.  Doing otherwise means that the
kernel has just switched modes, the translator has failed to end the TB, and
the new code has faulted immediately.

What I don't know is if we can, at the moment, canonicalize on TB flags.  If
the monitor were to use cpu_get_tb_cpu_state, I know it would work when using
TCG, but I don't know if we keep all the same data up-to-date in KVM or XEN modes.


r~

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Paolo Bonzini]

On 06/05/2015 16:12, Richard Henderson wrote:
>> Can
>> > we rely on the env/CPUState always being up to date during
>> > target_disas (which happens at translate time?) or will we need to go
>> > field by field to make sure any env updates explicitly occur before
>> > target_disas?
> I *think* so, but it's a near thing.  The path goes
> 
>  tb_find_fast:
>   cpu_get_tb_cpu_state, fill fill in flags for TB from current ENV state.
>   tb_find_slow,
>     tb_gen_code, using those same flags.
> 
> There's the edge case of re-translation, but I'm going to assert that cpu mode
> changes ought not happen in that context.  Doing otherwise means that the
> kernel has just switched modes, the translator has failed to end the TB, and
> the new code has faulted immediately.
> 
> What I don't know is if we can, at the moment, canonicalize on TB flags.  If
> the monitor were to use cpu_get_tb_cpu_state, I know it would work when using
> TCG, but I don't know if we keep all the same data up-to-date in KVM or XEN modes.

We do for KVM.

As far as I know, Xen doesn't have CPU state at all, not even in
fully-virtualized mode.  The CPU state is held (and serialized during
migration) by the hypervisor.

Paolo

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Stefano Stabellini]

On Wed, 6 May 2015, Paolo Bonzini wrote:
> On 06/05/2015 16:12, Richard Henderson wrote:
> >> Can
> >> > we rely on the env/CPUState always being up to date during
> >> > target_disas (which happens at translate time?) or will we need to go
> >> > field by field to make sure any env updates explicitly occur before
> >> > target_disas?
> > I *think* so, but it's a near thing.  The path goes
> > 
> >  tb_find_fast:
> >   cpu_get_tb_cpu_state, fill fill in flags for TB from current ENV state.
> >   tb_find_slow,
> >     tb_gen_code, using those same flags.
> > 
> > There's the edge case of re-translation, but I'm going to assert that cpu mode
> > changes ought not happen in that context.  Doing otherwise means that the
> > kernel has just switched modes, the translator has failed to end the TB, and
> > the new code has faulted immediately.
> > 
> > What I don't know is if we can, at the moment, canonicalize on TB flags.  If
> > the monitor were to use cpu_get_tb_cpu_state, I know it would work when using
> > TCG, but I don't know if we keep all the same data up-to-date in KVM or XEN modes.
> 
> We do for KVM.
> 
> As far as I know, Xen doesn't have CPU state at all, not even in
> fully-virtualized mode.  The CPU state is held (and serialized during
> migration) by the hypervisor.

Yes, that is correct, QEMU is only used as device emulator on Xen.

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Peter Maydell]

On 6 May 2015 at 15:12, Richard Henderson <rth@twiddle.net> wrote:
> On 05/06/2015 12:06 AM, Peter Crosthwaite wrote:
>> Can
>> we rely on the env/CPUState always being up to date during
>> target_disas (which happens at translate time?) or will we need to go
>> field by field to make sure any env updates explicitly occur before
>> target_disas?
>
> I *think* so, but it's a near thing.  The path goes
>
>  tb_find_fast:
>   cpu_get_tb_cpu_state, fill fill in flags for TB from current ENV state.
>   tb_find_slow,
>     tb_gen_code, using those same flags.
>
> There's the edge case of re-translation, but I'm going to assert that cpu mode
> changes ought not happen in that context.  Doing otherwise means that the
> kernel has just switched modes, the translator has failed to end the TB, and
> the new code has faulted immediately.

This is making the assumption that what the disassembler sees as
a "different mode" is the same as what the translator sees as a
"different mode" for which it needs to end the TB... This happens
to be true for ARM, at least, but I don't see any particular
reason why it is necessarily so for everything.

-- PMM

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Richard Henderson]

On 05/06/2015 08:44 AM, Peter Maydell wrote:
> On 6 May 2015 at 15:12, Richard Henderson <rth@twiddle.net> wrote:
>> On 05/06/2015 12:06 AM, Peter Crosthwaite wrote:
>>> Can
>>> we rely on the env/CPUState always being up to date during
>>> target_disas (which happens at translate time?) or will we need to go
>>> field by field to make sure any env updates explicitly occur before
>>> target_disas?
>>
>> I *think* so, but it's a near thing.  The path goes
>>
>>  tb_find_fast:
>>   cpu_get_tb_cpu_state, fill fill in flags for TB from current ENV state.
>>   tb_find_slow,
>>     tb_gen_code, using those same flags.
>>
>> There's the edge case of re-translation, but I'm going to assert that cpu mode
>> changes ought not happen in that context.  Doing otherwise means that the
>> kernel has just switched modes, the translator has failed to end the TB, and
>> the new code has faulted immediately.
> 
> This is making the assumption that what the disassembler sees as
> a "different mode" is the same as what the translator sees as a
> "different mode" for which it needs to end the TB... This happens
> to be true for ARM, at least, but I don't see any particular
> reason why it is necessarily so for everything.

I'm pretty sure it is, though.  Certainly true for x86 and mips.
I might call it a translator bug not to end the TB at a mode change.


r~

Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Peter Maydell]

On 6 May 2015 at 19:24, Richard Henderson <rth@twiddle.net> wrote:
> On 05/06/2015 08:44 AM, Peter Maydell wrote:
>> This is making the assumption that what the disassembler sees as
>> a "different mode" is the same as what the translator sees as a
>> "different mode" for which it needs to end the TB... This happens
>> to be true for ARM, at least, but I don't see any particular
>> reason why it is necessarily so for everything.
>
> I'm pretty sure it is, though.  Certainly true for x86 and mips.
> I might call it a translator bug not to end the TB at a mode change.

Why? The only reason we end the TB at mode changes for ARM/Thumb
is that the only way to do it in the architecture happens to
require a jump, I think.

I grant you it's probably a reasonable pragmatic requirement,
though.

-- PMM