Unify and QOMify (target|monitor)_disas

PATCH In review Peter Crosthwaite 4 participants 12 messages Jun 24, 2015 – Jul 6, 2015

Share & tools

Peter Crosthwaite

author

2015-06-24 03:57 UTC #0 lore raw

v4 cover Unify and QOMify (target|monitor)_disas

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 v3:
Minor commit message tweaks.
Fix CPP contructor argument order.

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

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-24 03:57 UTC #1 lore raw

PATCH v4 1/7 disas: Add print_insn to disassemble info

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

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

---
Changed since v3:
Add () to function name in commit message.

---
 disas.c             | 68 ++++++++++++++++++++++++++---------------------------
 include/disas/bfd.h |  6 +++++
 2 files changed, 39 insertions(+), 35 deletions(-)

disas.c +33 ~~-35~~

index 576c6a4..363c3bf 100644
@@ -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;

include/disas/bfd.h +6 -0

index 8bd703c..a112e9c 100644
@@ -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, \

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-24 03:57 UTC #2 lore raw

PATCH v4 2/7 disas: QOMify target specific setup

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

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

---
 disas.c           | 22 ++++++++++++++++++----
 include/qom/cpu.h |  4 ++++
 2 files changed, 22 insertions(+), 4 deletions(-)

disas.c +18 -4

index 363c3bf..ff5425d 100644
@@ -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);

include/qom/cpu.h +4 -0

index 39f0f19..363c928 100644
@@ -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

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-24 03:57 UTC #3 lore raw

PATCH v4 3/7 disas: arm-a64: Make printfer and stream variable

In reply to 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 printf() function and target stream settable in the QEMUDisassmbler class.

Reviewed-by Claudio Fontana <[email protected]>

Tested-by Claudio Fontana <[email protected]>

Reviewed-by Richard Henderson <[email protected]>

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

---
Changed since v3:
Remove stale commit message sentance.
Fix CPP constructor argument order
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(-)

disas/arm-a64.cc +17 -5

index e04f946..b0803f9 100644
@@ -35,16 +35,25 @@ static Disassembler *vixl_disasm = NULL;
  */
 class QEMUDisassembler : public Disassembler {
 public:
-    explicit QEMUDisassembler(FILE *stream) : stream_(stream) { }
+    QEMUDisassembler() : printf_(NULL), stream_(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);

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-24 03:57 UTC #4 lore raw

PATCH v4 4/7 disas: arm: QOMify target specific disas setup

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

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

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

disas.c +0 ~~-32~~

index ff5425d..fde5029 100644
@@ -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)

target-arm/cpu.c +35 -0

index 80669a6..6870ba5 100644
@@ -382,6 +382,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
+    }
+}
+
 #define ARM_CPUS_PER_CLUSTER 8
 
 static void arm_cpu_initfn(Object *obj)

@@ -1368,6 +1401,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)

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-24 03:57 UTC #5 lore raw

PATCH v4 5/7 disas: microblaze: QOMify target specific disas setup

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

Reviewed-by Edgar E. Iglesias <[email protected]>

---
 disas.c                 | 3 ---
 target-microblaze/cpu.c | 8 ++++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

disas.c +0 -3

index fde5029..937e08b 100644
@@ -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;

target-microblaze/cpu.c +8 -0

index c592bf7..36d3642 100644
@@ -119,6 +119,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);

@@ -256,6 +262,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 = {

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-24 03:57 UTC #6 lore raw

PATCH v4 6/7 disas: cris: Fix 0 buffer length case

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

Reviewed-by Edgar E. Iglesias <[email protected]>

---
 disas/cris.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

disas/cris.c +3 -3

index e6cff7a..1b76a09 100644
@@ -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.

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-24 03:57 UTC #7 lore raw

PATCH v4 7/7 disas: cris: QOMify target specific disas setup

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

Reviewed-by Edgar E. Iglesias <[email protected]>

---
 disas.c           |  8 --------
 target-cris/cpu.c | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 8 deletions(-)

disas.c +0 -8

index 937e08b..69a6066 100644
@@ -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;

target-cris/cpu.c +16 -0

index 16cfba9..d555ea0 100644
@@ -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 = {

Signature · 1 line

1.9.1

Peter Crosthwaite

2015-07-05 20:48 UTC #8 lore raw

In reply to Peter Crosthwaite

Ping!

If this or orphaned, I'd like to send the PULL on the H-freeze.

Regards,
Peter

Quoted 68 lines · Peter Crosthwaite

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 v3:
Minor commit message tweaks.
Fix CPP contructor argument order.
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

Richard Henderson

2015-07-06 11:44 UTC #9 lore raw

Quoted 3 lines · Peter Crosthwaite

Ping!
If this or orphaned, I'd like to send the PULL on the H-freeze.

Didn't all this get reviewed? I thought it did...

Quoted 74 lines

Regards,
Peter
Quoted 68 lines · Peter Crosthwaite
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 v3:
Minor commit message tweaks.
Fix CPP contructor argument order.
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

Andreas Färber

2015-07-06 11:48 UTC #10 lore raw

Am 06.07.2015 um 13:44 schrieb Richard Henderson:

Quoted 6 lines · Peter Crosthwaite

Quoted 3 lines
Ping!
If this or orphaned, I'd like to send the PULL on the H-freeze.
Didn't all this get reviewed? I thought it did...

If everything is reviewed I could take it for my CPU pull later today. Not sure if we'll run into a cpu.h conflict otherwise.

Signature · 3 lines

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

Peter Crosthwaite

2015-07-06 16:41 UTC #11 lore raw

Quoted 9 lines · Andreas Färber

Am 06.07.2015 um 13:44 schrieb Richard Henderson:
Quoted 6 lines · Peter Crosthwaite
Quoted 3 lines
Ping!
If this or orphaned, I'd like to send the PULL on the H-freeze.
Didn't all this get reviewed? I thought it did...
If everything is reviewed I could take it for my CPU pull later today.

It is reviewed:

$ ../patches/patches list 'from:"Peter Crosthwaite" status:reviewed'
Message-id: [email protected]
From: Peter Crosthwaite <[email protected]>
Date: 2015-06-24
Tags: v4
   [0/7] Unify and QOMify (target|monitor)_disas
   [1/7] disas: Add print_insn to disassemble info
   [2/7] disas: QOMify target specific setup
   [3/7] disas: arm-a64: Make printfer and stream variable
   [4/7] disas: arm: QOMify target specific disas setup
   [5/7] disas: microblaze: QOMify target specific disas setup
   [6/7] disas: cris: Fix 0 buffer length case
   [7/7] disas: cris: QOMify target specific disas setup

A merge via your CPU queue would be great.

Regards,
Peter

Quoted 9 lines

Not sure if we'll run into a cpu.h conflict otherwise.
af
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)