disas: Unify target_disas and monitor_disas

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 <[email protected]>

---
 disas.c | 148 ++++++++++++++++++++++++++--------------------------------------
 1 file changed, 61 insertions(+), 87 deletions(-)

disas.c +56 -86

index 44a019a..1c80567 100644
@@ -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);

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 <[email protected]>

---
 disas/arm-a64.cc | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

disas/arm-a64.cc +13 -4

index e04f946..d725e75 100644
@@ -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()) {

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

Signed-off-by Peter Crosthwaite <[email protected]>

---
 monitor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

monitor.c +4 -0

index d831d98..9d9f1e2 100644
@@ -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;
+        }

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

Signed-off-by Peter Crosthwaite <[email protected]>

---
 monitor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

monitor.c +1 -0

index d831d98..9d9f1e2 100644
@@ -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

Quoted 17 lines · Peter Crosthwaite

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

Signed-off-by Peter Crosthwaite <[email protected]>

---
 monitor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

monitor.c +1 -0

index d831d98..9d9f1e2 100644
@@ -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

monitor.c static void memory_dump(Monitor *mon, int count, int format, int wsize, · line 1230

Quoted 10 lines

monitor.c +10 -0

+        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

Quoted 33 lines · Peter Maydell

Quoted 17 lines · Peter Crosthwaite

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

Signed-off-by Peter Crosthwaite <[email protected]>

---
 monitor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

monitor.c +1 -0

index d831d98..9d9f1e2 100644
@@ -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

monitor.c static void memory_dump(Monitor *mon, int count, int format, int wsize, · line 1230

Quoted 10 lines

monitor.c +10 -0

+        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.

Quoted 2 lines

-- 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~

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 ...

Quoted 8 lines

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.

Quoted 6 lines

            /* 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...

Quoted 22 lines

                    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

Quoted 2 lines

    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?

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.

Quoted 33 lines · Peter Maydell

Quoted 17 lines · Peter Crosthwaite

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

Signed-off-by Peter Crosthwaite <[email protected]>

---
 monitor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

monitor.c +1 -0

index d831d98..9d9f1e2 100644
@@ -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

monitor.c static void memory_dump(Monitor *mon, int count, int format, int wsize, · line 1230

Quoted 10 lines

monitor.c +10 -0

+        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.

Quoted 2 lines

-- 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~

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?

Quoted 5 lines

Can

Quoted 4 lines

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.

Quoted 12 lines · Richard Henderson

Quoted 5 lines

Can

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 5 lines · Peter Crosthwaite

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.

Quoted 14 lines · Richard Henderson

Quoted 5 lines · Peter Crosthwaite

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.

Quoted 4 lines

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.

Quoted 5 lines · Peter Maydell

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.

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 <[email protected]>

---
 disas.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

disas.c +0 -0

index 498b05f..e1da40d 100644

disas.c +19 -25

+         * 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) {

Hello Peter,

Quoted 25 lines · 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 <[email protected]>

---
 disas.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

disas.c target_disas_set_info(int (**print_insn)(bfd_vma pc, disassemble_info *info), · line 214

Quoted 12 lines

disas.c +4 -0

index 498b05f..e1da40d 100644
@@ -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 <[email protected]>

Reviewed-by Claudio Fontana <[email protected]>

Quoted 52 lines

disas.c +18 -22

+         * 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;
     }

disas.c void monitor_disas(Monitor *mon, CPUArchState *env, · line 481

Quoted 10 lines

disas.c +1 -3

@@ -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

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).

Quoted 1 line

Quoted 1 line