[Qemu-devel] [PATCH v3 0/7] Unify and QOMify (target|monitor)_disas

[Qemu-devel] [PATCH v3 0/7] Unify and QOMify (target|monitor)_disas

[Peter Crosthwaite]

Depends on series: [PATCH v2 0/2] monitor+disas: Remove uses of ENV_GET_CPU

Intended for QOM queue.

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

The approach is to add a single QOM hook on the CPU level to setup the
disassembler (P1&2). The two stage flags system is removed. That is,
the old scheme, is for the translate/montitor code to pass in flags
that disas.c then interprets. Instead the entire job of setting up arch
specifics is outsourced to target-specific code (via the new QOM hook)
removing the need for the flags system. Both monitor_disas and
target_disas then calls this singly defined hook if it is available.

Three architectures (microblaze, cris and ARM) are patched
to use the new QOMification and at the same time, make the
monitor_disas consistent with target_disas. The #if defined TARGET_FOO
for each is removed from disas.c (bringing us closer to the exciting
goal of no #ifdef TARGET_FOO in system mode code).

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

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

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 (P3). P4 brings
ARM monitor disassembly online (via using the target_disas
implementation as the QOMified implementation).

Changed since v2 (RTH/PMM review):
Rebased on monitor+disas ENV_GET_CPU removal
Fixed minor comments (see indiv patches).

Changed since v1 (RTH review):
Use QOMified approach.
Remove flags system.
Limit scope to only the 3 converted arches
Addressed comments on CPP constructor changes

Peter Crosthwaite (7):
  disas: Add print_insn to disassemble info
  disas: QOMify target specific setup
  disas: arm-a64: Make printfer and stream variable
  disas: arm: QOMify target specific disas setup
  disas: microblaze: QOMify target specific disas setup
  disas: cris: Fix 0 buffer length case
  disas: cris: QOMify target specific disas setup

 disas.c                 | 119 ++++++++++++++++++------------------------------
 disas/arm-a64.cc        |  22 +++++++--
 disas/cris.c            |   6 +--
 include/disas/bfd.h     |   6 +++
 include/qom/cpu.h       |   4 ++
 target-arm/cpu.c        |  35 ++++++++++++++
 target-cris/cpu.c       |  16 +++++++
 target-microblaze/cpu.c |   8 ++++
 8 files changed, 133 insertions(+), 83 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v3 1/7] disas: Add print_insn to disassemble info

[Peter Crosthwaite]

Add the print_insn pointer to the disassemble info structure. This is
to prepare for QOMification support, where a QOM CPU hook function will
be responsible for setting the print_insn function. Add this function
to the existing struct to consolidate such that only the one struct
needs to be passed to the new QOM API.

Reviewed-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 disas.c             | 68 ++++++++++++++++++++++++++---------------------------
 include/disas/bfd.h |  6 +++++
 2 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/disas.c b/disas.c
index 576c6a4..363c3bf 100644
--- a/disas.c
+++ b/disas.c
@@ -201,7 +201,6 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
     target_ulong pc;
     int count;
     CPUDebug s;
-    int (*print_insn)(bfd_vma pc, disassemble_info *info) = NULL;
 
     INIT_DISASSEMBLE_INFO(s.info, out, fprintf);
 
@@ -224,7 +223,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
     } else {
         s.info.mach = bfd_mach_i386_i386;
     }
-    print_insn = print_insn_i386;
+    s.info.print_insn = print_insn_i386;
 #elif defined(TARGET_ARM)
     if (flags & 4) {
         /* We might not be compiled with the A64 disassembler
@@ -232,12 +231,12 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
          * fall through to the default print_insn_od case.
          */
 #if defined(CONFIG_ARM_A64_DIS)
-        print_insn = print_insn_arm_a64;
+        s.info.print_insn = print_insn_arm_a64;
 #endif
     } else if (flags & 1) {
-        print_insn = print_insn_thumb1;
+        s.info.print_insn = print_insn_thumb1;
     } else {
-        print_insn = print_insn_arm;
+        s.info.print_insn = print_insn_arm;
     }
     if (flags & 2) {
 #ifdef TARGET_WORDS_BIGENDIAN
@@ -247,7 +246,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
 #endif
     }
 #elif defined(TARGET_SPARC)
-    print_insn = print_insn_sparc;
+    s.info.print_insn = print_insn_sparc;
 #ifdef TARGET_SPARC64
     s.info.mach = bfd_mach_sparc_v9b;
 #endif
@@ -266,49 +265,49 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
 #endif
     }
     s.info.disassembler_options = (char *)"any";
-    print_insn = print_insn_ppc;
+    s.info.print_insn = print_insn_ppc;
 #elif defined(TARGET_M68K)
-    print_insn = print_insn_m68k;
+    s.info.print_insn = print_insn_m68k;
 #elif defined(TARGET_MIPS)
 #ifdef TARGET_WORDS_BIGENDIAN
-    print_insn = print_insn_big_mips;
+    s.info.print_insn = print_insn_big_mips;
 #else
-    print_insn = print_insn_little_mips;
+    s.info.print_insn = print_insn_little_mips;
 #endif
 #elif defined(TARGET_SH4)
     s.info.mach = bfd_mach_sh4;
-    print_insn = print_insn_sh;
+    s.info.print_insn = print_insn_sh;
 #elif defined(TARGET_ALPHA)
     s.info.mach = bfd_mach_alpha_ev6;
-    print_insn = print_insn_alpha;
+    s.info.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;
+        s.info.print_insn = print_insn_crisv10;
     } else {
         s.info.mach = bfd_mach_cris_v32;
-        print_insn = print_insn_crisv32;
+        s.info.print_insn = print_insn_crisv32;
     }
 #elif defined(TARGET_S390X)
     s.info.mach = bfd_mach_s390_64;
-    print_insn = print_insn_s390;
+    s.info.print_insn = print_insn_s390;
 #elif defined(TARGET_MICROBLAZE)
     s.info.mach = bfd_arch_microblaze;
-    print_insn = print_insn_microblaze;
+    s.info.print_insn = print_insn_microblaze;
 #elif defined(TARGET_MOXIE)
     s.info.mach = bfd_arch_moxie;
-    print_insn = print_insn_moxie;
+    s.info.print_insn = print_insn_moxie;
 #elif defined(TARGET_LM32)
     s.info.mach = bfd_mach_lm32;
-    print_insn = print_insn_lm32;
+    s.info.print_insn = print_insn_lm32;
 #endif
-    if (print_insn == NULL) {
-        print_insn = print_insn_od_target;
+    if (s.info.print_insn == NULL) {
+        s.info.print_insn = print_insn_od_target;
     }
 
     for (pc = code; size > 0; pc += count, size -= count) {
 	fprintf(out, "0x" TARGET_FMT_lx ":  ", pc);
-	count = print_insn(pc, &s.info);
+	count = s.info.print_insn(pc, &s.info);
 #if 0
         {
             int i;
@@ -452,7 +451,6 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
 {
     int count, i;
     CPUDebug s;
-    int (*print_insn)(bfd_vma pc, disassemble_info *info);
 
     INIT_DISASSEMBLE_INFO(s.info, (FILE *)mon, monitor_fprintf);
 
@@ -476,13 +474,13 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
     } else {
         s.info.mach = bfd_mach_i386_i386;
     }
-    print_insn = print_insn_i386;
+    s.info.print_insn = print_insn_i386;
 #elif defined(TARGET_ARM)
-    print_insn = print_insn_arm;
+    s.info.print_insn = print_insn_arm;
 #elif defined(TARGET_ALPHA)
-    print_insn = print_insn_alpha;
+    s.info.print_insn = print_insn_alpha;
 #elif defined(TARGET_SPARC)
-    print_insn = print_insn_sparc;
+    s.info.print_insn = print_insn_sparc;
 #ifdef TARGET_SPARC64
     s.info.mach = bfd_mach_sparc_v9b;
 #endif
@@ -500,27 +498,27 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
     if ((flags >> 16) & 1) {
         s.info.endian = BFD_ENDIAN_LITTLE;
     }
-    print_insn = print_insn_ppc;
+    s.info.print_insn = print_insn_ppc;
 #elif defined(TARGET_M68K)
-    print_insn = print_insn_m68k;
+    s.info.print_insn = print_insn_m68k;
 #elif defined(TARGET_MIPS)
 #ifdef TARGET_WORDS_BIGENDIAN
-    print_insn = print_insn_big_mips;
+    s.info.print_insn = print_insn_big_mips;
 #else
-    print_insn = print_insn_little_mips;
+    s.info.print_insn = print_insn_little_mips;
 #endif
 #elif defined(TARGET_SH4)
     s.info.mach = bfd_mach_sh4;
-    print_insn = print_insn_sh;
+    s.info.print_insn = print_insn_sh;
 #elif defined(TARGET_S390X)
     s.info.mach = bfd_mach_s390_64;
-    print_insn = print_insn_s390;
+    s.info.print_insn = print_insn_s390;
 #elif defined(TARGET_MOXIE)
     s.info.mach = bfd_arch_moxie;
-    print_insn = print_insn_moxie;
+    s.info.print_insn = print_insn_moxie;
 #elif defined(TARGET_LM32)
     s.info.mach = bfd_mach_lm32;
-    print_insn = print_insn_lm32;
+    s.info.print_insn = print_insn_lm32;
 #else
     monitor_printf(mon, "0x" TARGET_FMT_lx
                    ": Asm output not supported on this arch\n", pc);
@@ -529,7 +527,7 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
 
     for(i = 0; i < nb_insn; i++) {
 	monitor_printf(mon, "0x" TARGET_FMT_lx ":  ", pc);
-        count = print_insn(pc, &s.info);
+        count = s.info.print_insn(pc, &s.info);
 	monitor_printf(mon, "\n");
 	if (count < 0)
 	    break;
diff --git a/include/disas/bfd.h b/include/disas/bfd.h
index 8bd703c..a112e9c 100644
--- a/include/disas/bfd.h
+++ b/include/disas/bfd.h
@@ -313,6 +313,11 @@ typedef struct disassemble_info {
   void (*print_address_func)
     (bfd_vma addr, struct disassemble_info *info);
 
+    /* Function called to print an instruction. The function is architecture
+     * specific.
+     */
+    int (*print_insn)(bfd_vma addr, struct disassemble_info *info);
+
   /* Function called to determine if there is a symbol at the given ADDR.
      If there is, the function returns 1, otherwise it returns 0.
      This is used by ports which support an overlay manager where
@@ -463,6 +468,7 @@ int generic_symbol_at_address(bfd_vma, struct disassemble_info *);
   (INFO).read_memory_func = buffer_read_memory, \
   (INFO).memory_error_func = perror_memory, \
   (INFO).print_address_func = generic_print_address, \
+  (INFO).print_insn = NULL, \
   (INFO).symbol_at_address_func = generic_symbol_at_address, \
   (INFO).flags = 0, \
   (INFO).bytes_per_line = 0, \
-- 
1.9.1

[Qemu-devel] [PATCH v3 2/7] disas: QOMify target specific setup

[Peter Crosthwaite]

Add a QOM function hook for target-specific disassembly setup. This
allows removal of the #ifdeffery currently implementing target specific
disas setup from disas.c.

Reviewed-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 disas.c           | 22 ++++++++++++++++++----
 include/qom/cpu.h |  4 ++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/disas.c b/disas.c
index 363c3bf..ff5425d 100644
--- a/disas.c
+++ b/disas.c
@@ -1,5 +1,6 @@
 /* General "disassemble this chunk" code.  Used for debugging. */
 #include "config.h"
+#include "qemu-common.h"
 #include "disas/bfd.h"
 #include "elf.h"
 #include <errno.h>
@@ -198,6 +199,7 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
 void target_disas(FILE *out, CPUState *cpu, target_ulong code,
                   target_ulong size, int flags)
 {
+    CPUClass *cc = CPU_GET_CLASS(cpu);
     target_ulong pc;
     int count;
     CPUDebug s;
@@ -215,6 +217,11 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
 #else
     s.info.endian = BFD_ENDIAN_LITTLE;
 #endif
+
+    if (cc->disas_set_info) {
+        cc->disas_set_info(cpu, &s.info);
+    }
+
 #if defined(TARGET_I386)
     if (flags == 2) {
         s.info.mach = bfd_mach_x86_64;
@@ -449,6 +456,7 @@ monitor_fprintf(FILE *stream, const char *fmt, ...)
 void monitor_disas(Monitor *mon, CPUState *cpu,
                    target_ulong pc, int nb_insn, int is_physical, int flags)
 {
+    CPUClass *cc = CPU_GET_CLASS(cpu);
     int count, i;
     CPUDebug s;
 
@@ -466,6 +474,11 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
 #else
     s.info.endian = BFD_ENDIAN_LITTLE;
 #endif
+
+    if (cc->disas_set_info) {
+        cc->disas_set_info(cpu, &s.info);
+    }
+
 #if defined(TARGET_I386)
     if (flags == 2) {
         s.info.mach = bfd_mach_x86_64;
@@ -519,11 +532,12 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
 #elif defined(TARGET_LM32)
     s.info.mach = bfd_mach_lm32;
     s.info.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 (!s.info.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);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 39f0f19..363c928 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -23,6 +23,7 @@
 #include <signal.h>
 #include <setjmp.h>
 #include "hw/qdev-core.h"
+#include "disas/bfd.h"
 #include "exec/hwaddr.h"
 #include "exec/memattrs.h"
 #include "qemu/queue.h"
@@ -117,6 +118,7 @@ struct TranslationBlock;
  * @cpu_exec_enter: Callback for cpu_exec preparation.
  * @cpu_exec_exit: Callback for cpu_exec cleanup.
  * @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec.
+ * @disas_set_info: Setup architecture specific components of disassembly info
  *
  * Represents a CPU family or model.
  */
@@ -172,6 +174,8 @@ typedef struct CPUClass {
     void (*cpu_exec_enter)(CPUState *cpu);
     void (*cpu_exec_exit)(CPUState *cpu);
     bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
+
+    void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
 } CPUClass;
 
 #ifdef HOST_WORDS_BIGENDIAN
-- 
1.9.1

[Qemu-devel] [PATCH v3 3/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).

Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
Tested-by: Claudio Fontana <claudio.fontana@huawei.com>
Reviewed-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Changed since v2:
Changed styling of NULL variable construction
Changed since v1:
Drop explicit from constructor
Keep NULL stream_ initialiser
Initialise printf_ to NULL
---
 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..be5a733 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) { }
+    QEMUDisassembler() : stream_(NULL), printf_(NULL) { }
     ~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

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

[Peter Crosthwaite]

On Sun, May 24, 2015 at 3:47 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> 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).
>

This sentence is stale from a previous version. Removed.

> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> Tested-by: Claudio Fontana <claudio.fontana@huawei.com>
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> Changed since v2:
> Changed styling of NULL variable construction
> Changed since v1:
> Drop explicit from constructor
> Keep NULL stream_ initialiser
> Initialise printf_ to NULL
> ---
>  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..be5a733 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) { }
> +    QEMUDisassembler() : stream_(NULL), printf_(NULL) { }

Constructor args need to be reversed to avoid compile warning (doh).

Both fixed in v4.

Regards,
Peter

>      ~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 v3 4/7] disas: arm: QOMify target specific disas setup

[Peter Crosthwaite]

Move the target_disas() ARM specifics to the QOM disas_set_info hook
and delete the ARM specific code in disas.c.

This has the extra advantage of the more fully featured target_disas()
implementation now applying to monitor_disas().

Currently, target_disas() 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 following -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)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Changed since v2:
Replace env->aarch64 with is_a64(env) (PMM review)
Fix grammar error in commit message
---
 disas.c          | 32 --------------------------------
 target-arm/cpu.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/disas.c b/disas.c
index ff5425d..fde5029 100644
--- a/disas.c
+++ b/disas.c
@@ -151,14 +151,6 @@ bfd_vma bfd_getb16 (const bfd_byte *addr)
   return (bfd_vma) v;
 }
 
-#ifdef TARGET_ARM
-static int
-print_insn_thumb1(bfd_vma pc, disassemble_info *info)
-{
-  return print_insn_arm(pc | 1, info);
-}
-#endif
-
 static int print_insn_objdump(bfd_vma pc, disassemble_info *info,
                               const char *prefix)
 {
@@ -191,7 +183,6 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
 /* Disassemble this for me please... (debugging). 'flags' has the following
    values:
     i386 - 1 means 16 bit code, 2 means 64 bit code
-    arm  - bit 0 = thumb, bit 1 = reverse endian, bit 2 = A64
     ppc  - bits 0:15 specify (optionally) the machine instruction set;
            bit 16 indicates little endian.
     other targets - unused
@@ -231,27 +222,6 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
         s.info.mach = bfd_mach_i386_i386;
     }
     s.info.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)
-        s.info.print_insn = print_insn_arm_a64;
-#endif
-    } else if (flags & 1) {
-        s.info.print_insn = print_insn_thumb1;
-    } else {
-        s.info.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)
     s.info.print_insn = print_insn_sparc;
 #ifdef TARGET_SPARC64
@@ -488,8 +458,6 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
         s.info.mach = bfd_mach_i386_i386;
     }
     s.info.print_insn = print_insn_i386;
-#elif defined(TARGET_ARM)
-    s.info.print_insn = print_insn_arm;
 #elif defined(TARGET_ALPHA)
     s.info.print_insn = print_insn_alpha;
 #elif defined(TARGET_SPARC)
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 3ca3fa8..e39586b 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -362,6 +362,39 @@ static inline void unset_feature(CPUARMState *env, int feature)
     env->features &= ~(1ULL << feature);
 }
 
+static int
+print_insn_thumb1(bfd_vma pc, disassemble_info *info)
+{
+  return print_insn_arm(pc | 1, info);
+}
+
+static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
+{
+    ARMCPU *ac = ARM_CPU(cpu);
+    CPUARMState *env = &ac->env;
+
+    if (is_a64(env)) {
+        /* We might not be compiled with the A64 disassembler
+         * because it needs a C++ compiler. Leave print_insn
+         * unset in this case to use the caller default behaviour.
+         */
+#if defined(CONFIG_ARM_A64_DIS)
+        info->print_insn = print_insn_arm_a64;
+#endif
+    } else if (env->thumb) {
+        info->print_insn = print_insn_thumb1;
+    } else {
+        info->print_insn = print_insn_arm;
+    }
+    if (env->bswap_code) {
+#ifdef TARGET_WORDS_BIGENDIAN
+        info->endian = BFD_ENDIAN_LITTLE;
+#else
+        info->endian = BFD_ENDIAN_BIG;
+#endif
+    }
+}
+
 static void arm_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -1229,6 +1262,8 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_core_xml_file = "arm-core.xml";
     cc->gdb_stop_before_watchpoint = true;
     cc->debug_excp_handler = arm_debug_excp_handler;
+
+    cc->disas_set_info = arm_disas_set_info;
 }
 
 static void cpu_register(const ARMCPUInfo *info)
-- 
1.9.1

[Qemu-devel] [PATCH v3 5/7] disas: microblaze: QOMify target specific disas setup

[Peter Crosthwaite]

Move the target_disas() MB specifics to the QOM disas_set_info hook
and delete the MB specific code in disas.c.

This also now adds support for monitor disas to Microblaze.

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                 | 3 ---
 target-microblaze/cpu.c | 8 ++++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/disas.c b/disas.c
index fde5029..937e08b 100644
--- a/disas.c
+++ b/disas.c
@@ -268,9 +268,6 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
 #elif defined(TARGET_S390X)
     s.info.mach = bfd_mach_s390_64;
     s.info.print_insn = print_insn_s390;
-#elif defined(TARGET_MICROBLAZE)
-    s.info.mach = bfd_arch_microblaze;
-    s.info.print_insn = print_insn_microblaze;
 #elif defined(TARGET_MOXIE)
     s.info.mach = bfd_arch_moxie;
     s.info.print_insn = print_insn_moxie;
diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
index 67e3182..89b8363 100644
--- a/target-microblaze/cpu.c
+++ b/target-microblaze/cpu.c
@@ -111,6 +111,12 @@ static void mb_cpu_reset(CPUState *s)
 #endif
 }
 
+static void mb_disas_set_info(CPUState *cpu, disassemble_info *info)
+{
+    info->mach = bfd_arch_microblaze;
+    info->print_insn = print_insn_microblaze;
+}
+
 static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
@@ -183,6 +189,8 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
     dc->vmsd = &vmstate_mb_cpu;
     dc->props = mb_properties;
     cc->gdb_num_core_regs = 32 + 5;
+
+    cc->disas_set_info = mb_disas_set_info;
 }
 
 static const TypeInfo mb_cpu_type_info = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3 5/7] disas: microblaze: QOMify target specific disas setup

[Peter Crosthwaite]

Ping!

On Sun, May 24, 2015 at 3:47 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> Move the target_disas() MB specifics to the QOM disas_set_info hook
> and delete the MB specific code in disas.c.
>
> This also now adds support for monitor disas to Microblaze.
>
> 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                 | 3 ---
>  target-microblaze/cpu.c | 8 ++++++++
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/disas.c b/disas.c
> index fde5029..937e08b 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -268,9 +268,6 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
>  #elif defined(TARGET_S390X)
>      s.info.mach = bfd_mach_s390_64;
>      s.info.print_insn = print_insn_s390;
> -#elif defined(TARGET_MICROBLAZE)
> -    s.info.mach = bfd_arch_microblaze;
> -    s.info.print_insn = print_insn_microblaze;
>  #elif defined(TARGET_MOXIE)
>      s.info.mach = bfd_arch_moxie;
>      s.info.print_insn = print_insn_moxie;
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 67e3182..89b8363 100644
> --- a/target-microblaze/cpu.c
> +++ b/target-microblaze/cpu.c
> @@ -111,6 +111,12 @@ static void mb_cpu_reset(CPUState *s)
>  #endif
>  }
>
> +static void mb_disas_set_info(CPUState *cpu, disassemble_info *info)
> +{
> +    info->mach = bfd_arch_microblaze;
> +    info->print_insn = print_insn_microblaze;
> +}
> +
>  static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -183,6 +189,8 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
>      dc->vmsd = &vmstate_mb_cpu;
>      dc->props = mb_properties;
>      cc->gdb_num_core_regs = 32 + 5;
> +
> +    cc->disas_set_info = mb_disas_set_info;
>  }
>
>  static const TypeInfo mb_cpu_type_info = {
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH v3 5/7] disas: microblaze: QOMify target specific disas setup

[Edgar E. Iglesias]

Looks good:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


On Sun, May 24, 2015 at 03:47:18PM -0700, Peter Crosthwaite wrote:
> Move the target_disas() MB specifics to the QOM disas_set_info hook
> and delete the MB specific code in disas.c.
> 
> This also now adds support for monitor disas to Microblaze.
> 
> 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                 | 3 ---
>  target-microblaze/cpu.c | 8 ++++++++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/disas.c b/disas.c
> index fde5029..937e08b 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -268,9 +268,6 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
>  #elif defined(TARGET_S390X)
>      s.info.mach = bfd_mach_s390_64;
>      s.info.print_insn = print_insn_s390;
> -#elif defined(TARGET_MICROBLAZE)
> -    s.info.mach = bfd_arch_microblaze;
> -    s.info.print_insn = print_insn_microblaze;
>  #elif defined(TARGET_MOXIE)
>      s.info.mach = bfd_arch_moxie;
>      s.info.print_insn = print_insn_moxie;
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 67e3182..89b8363 100644
> --- a/target-microblaze/cpu.c
> +++ b/target-microblaze/cpu.c
> @@ -111,6 +111,12 @@ static void mb_cpu_reset(CPUState *s)
>  #endif
>  }
>  
> +static void mb_disas_set_info(CPUState *cpu, disassemble_info *info)
> +{
> +    info->mach = bfd_arch_microblaze;
> +    info->print_insn = print_insn_microblaze;
> +}
> +
>  static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -183,6 +189,8 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
>      dc->vmsd = &vmstate_mb_cpu;
>      dc->props = mb_properties;
>      cc->gdb_num_core_regs = 32 + 5;
> +
> +    cc->disas_set_info = mb_disas_set_info;
>  }
>  
>  static const TypeInfo mb_cpu_type_info = {
> -- 
> 1.9.1
> 
> 

[Qemu-devel] [PATCH v3 6/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

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

[Edgar E. Iglesias]

On Sun, May 24, 2015 at 03:47:19PM -0700, Peter Crosthwaite wrote:
> 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>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.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 v3 7/7] disas: cris: QOMify target specific disas setup

[Peter Crosthwaite]

Move the target_disas() cris specifics to the QOM disas_set_info hook
and delete the cris specific code in disas.c.

This also now adds support for monitor disas to cris.

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           |  8 --------
 target-cris/cpu.c | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/disas.c b/disas.c
index 937e08b..69a6066 100644
--- a/disas.c
+++ b/disas.c
@@ -257,14 +257,6 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
 #elif defined(TARGET_ALPHA)
     s.info.mach = bfd_mach_alpha_ev6;
     s.info.print_insn = print_insn_alpha;
-#elif defined(TARGET_CRIS)
-    if (flags != 32) {
-        s.info.mach = bfd_mach_cris_v0_v10;
-        s.info.print_insn = print_insn_crisv10;
-    } else {
-        s.info.mach = bfd_mach_cris_v32;
-        s.info.print_insn = print_insn_crisv32;
-    }
 #elif defined(TARGET_S390X)
     s.info.mach = bfd_mach_s390_64;
     s.info.print_insn = print_insn_s390;
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index 16cfba9..d555ea0 100644
--- a/target-cris/cpu.c
+++ b/target-cris/cpu.c
@@ -161,6 +161,20 @@ static void cris_cpu_set_irq(void *opaque, int irq, int level)
 }
 #endif
 
+static void cris_disas_set_info(CPUState *cpu, disassemble_info *info)
+{
+    CRISCPU *cc = CRIS_CPU(cpu);
+    CPUCRISState *env = &cc->env;
+
+    if (env->pregs[PR_VR] != 32) {
+        info->mach = bfd_mach_cris_v0_v10;
+        info->print_insn = print_insn_crisv10;
+    } else {
+        info->mach = bfd_mach_cris_v32;
+        info->print_insn = print_insn_crisv32;
+    }
+}
+
 static void cris_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -292,6 +306,8 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
 
     cc->gdb_num_core_regs = 49;
     cc->gdb_stop_before_watchpoint = true;
+
+    cc->disas_set_info = cris_disas_set_info;
 }
 
 static const TypeInfo cris_cpu_type_info = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3 7/7] disas: cris: QOMify target specific disas setup

[Edgar E. Iglesias]

On Sun, May 24, 2015 at 03:47:20PM -0700, Peter Crosthwaite wrote:
> Move the target_disas() cris specifics to the QOM disas_set_info hook
> and delete the cris specific code in disas.c.
> 
> This also now adds support for monitor disas to cris.
> 
> 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>

Thanks Peter

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  disas.c           |  8 --------
>  target-cris/cpu.c | 16 ++++++++++++++++
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/disas.c b/disas.c
> index 937e08b..69a6066 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -257,14 +257,6 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
>  #elif defined(TARGET_ALPHA)
>      s.info.mach = bfd_mach_alpha_ev6;
>      s.info.print_insn = print_insn_alpha;
> -#elif defined(TARGET_CRIS)
> -    if (flags != 32) {
> -        s.info.mach = bfd_mach_cris_v0_v10;
> -        s.info.print_insn = print_insn_crisv10;
> -    } else {
> -        s.info.mach = bfd_mach_cris_v32;
> -        s.info.print_insn = print_insn_crisv32;
> -    }
>  #elif defined(TARGET_S390X)
>      s.info.mach = bfd_mach_s390_64;
>      s.info.print_insn = print_insn_s390;
> diff --git a/target-cris/cpu.c b/target-cris/cpu.c
> index 16cfba9..d555ea0 100644
> --- a/target-cris/cpu.c
> +++ b/target-cris/cpu.c
> @@ -161,6 +161,20 @@ static void cris_cpu_set_irq(void *opaque, int irq, int level)
>  }
>  #endif
>  
> +static void cris_disas_set_info(CPUState *cpu, disassemble_info *info)
> +{
> +    CRISCPU *cc = CRIS_CPU(cpu);
> +    CPUCRISState *env = &cc->env;
> +
> +    if (env->pregs[PR_VR] != 32) {
> +        info->mach = bfd_mach_cris_v0_v10;
> +        info->print_insn = print_insn_crisv10;
> +    } else {
> +        info->mach = bfd_mach_cris_v32;
> +        info->print_insn = print_insn_crisv32;
> +    }
> +}
> +
>  static void cris_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -292,6 +306,8 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
>  
>      cc->gdb_num_core_regs = 49;
>      cc->gdb_stop_before_watchpoint = true;
> +
> +    cc->disas_set_info = cris_disas_set_info;
>  }
>  
>  static const TypeInfo cris_cpu_type_info = {
> -- 
> 1.9.1
> 
>