[Qemu-devel] [V3 PATCH 0/2] QEMU Monitor Instruction Disassembly Incorrect for PPC LE

[Qemu-devel] [V3 PATCH 0/2] QEMU Monitor Instruction Disassembly Incorrect for PPC LE

[Tom Musta]

Fix disassembly in the QEMU monitor for Little Endian codes.  Also fix the comment
and tighten up a flag check in the closely related disassembler code for
tracing.

V2: Fixed target_disas comment and bit decoding.
V3: Make monitor_disas flag documentation refer to target_disas documentation.

Tom Musta (2):
  target-ppc: Fix target_disas
  monitor: QEMU Monitor Instruction Disassembly Incorrect for PowerPC
    LE Mode

 disas.c   |   19 +++++++++++++++----
 monitor.c |    4 ++++
 2 files changed, 19 insertions(+), 4 deletions(-)

[Qemu-devel] [V3 PATCH 1/2] target-ppc: Fix target_disas

[Tom Musta]

Inspect only bit 16 for the Little Endian test.  Correct comment preceding
the target_disas() function.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---

V2: new

 disas.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/disas.c b/disas.c
index 79e6944..a427d18 100644
--- a/disas.c
+++ b/disas.c
@@ -191,7 +191,8 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
    values:
     i386 - 1 means 16 bit code, 2 means 64 bit code
     arm  - bit 0 = thumb, bit 1 = reverse endian, bit 2 = A64
-    ppc  - nonzero means little endian
+    ppc  - bits 0:15 specify (optionally) the machine instruction set;
+           bit 16 indicates little endian.
     other targets - unused
  */
 void target_disas(FILE *out, CPUArchState *env, target_ulong code,
@@ -251,7 +252,7 @@ void target_disas(FILE *out, CPUArchState *env, target_ulong code,
     s.info.mach = bfd_mach_sparc_v9b;
 #endif
 #elif defined(TARGET_PPC)
-    if (flags >> 16) {
+    if ((flags >> 16) & 1) {
         s.info.endian = BFD_ENDIAN_LITTLE;
     }
     if (flags & 0xFFFF) {
-- 
1.7.1

Re: [Qemu-devel] [V3 PATCH 1/2] target-ppc: Fix target_disas

[Peter Maydell]

On 8 April 2014 20:26, Tom Musta <tommusta@gmail.com> wrote:
> Inspect only bit 16 for the Little Endian test.  Correct comment preceding
> the target_disas() function.
>
> Signed-off-by: Tom Musta <tommusta@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

[Qemu-devel] [V3 PATCH 2/2] monitor: QEMU Monitor Instruction Disassembly Incorrect for PowerPC LE Mode

[Tom Musta]

The monitor support for disassembling instructions does not honor the MSR[LE]
bit for PowerPC processors.

This change enhances the monitor_disas() routine by supporting a flag bit
for Little Endian mode.  Bit 16 is used since that bit was used in the
analagous guest disassembly routine target_disas().

Also, to be consistent with target_disas(), the disassembler bfd_mach field
can be passed in the flags argument.

Reported-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Tom Musta <tommusta@gmail.com>
---

V2: Look specifically at bit 16 for LE.  Support machine configuration in flags.

V3: Changed documentation of 'flags' argument to simply refer to the target_disas
  description (per Peter Maydell's review).

The bug can be easily observed by dumping the first few instructions of an
interrupt vector (0x300 is the Data Storage Interrupt handler in PPC)

  (qemu) xp/8i 0x300
  0x0000000000000300:  .long 0x60
  0x0000000000000304:  lhzu    r18,-19843(r3)
  0x0000000000000308:  .long 0x60
  0x000000000000030c:  lhzu    r18,-20099(r2)
  0x0000000000000310:  lwz     r0,11769(0)
  0x0000000000000314:  lhzu    r23,8317(r2)
  0x0000000000000318:  .long 0x7813427c
  0x000000000000031c:  lbz     r0,19961(0)

With the patch applied, the disassembly now looks correct:

  (qemu) xp/8i 0x300
  0x0000000000000300:  nop
  0x0000000000000304:  mtsprg  2,r13
  0x0000000000000308:  nop
  0x000000000000030c:  mfsprg  r13,1
  0x0000000000000310:  std     r9,128(r13)
  0x0000000000000314:  mfspr   r9,896
  0x0000000000000318:  mr      r2,r2
  0x000000000000031c:  std     r10,136(r13)
 disas.c   |   14 ++++++++++++--
 monitor.c |    4 ++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/disas.c b/disas.c
index a427d18..f300102 100644
--- a/disas.c
+++ b/disas.c
@@ -445,6 +445,8 @@ monitor_fprintf(FILE *stream, const char *fmt, ...)
     return 0;
 }
 
+/* Disassembler for the monitor.  See target_disas for a description of 'flags'.
+ */
 void monitor_disas(Monitor *mon, CPUArchState *env,
                    target_ulong pc, int nb_insn, int is_physical, int flags)
 {
@@ -485,11 +487,19 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
     s.info.mach = bfd_mach_sparc_v9b;
 #endif
 #elif defined(TARGET_PPC)
+    if (flags & 0xFFFF) {
+        /* If we have a precise definitions of the instructions set, use it */
+        s.info.mach = flags & 0xFFFF;
+    } else {
 #ifdef TARGET_PPC64
-    s.info.mach = bfd_mach_ppc64;
+        s.info.mach = bfd_mach_ppc64;
 #else
-    s.info.mach = bfd_mach_ppc;
+        s.info.mach = bfd_mach_ppc;
 #endif
+    }
+    if ((flags >> 16) & 1) {
+        s.info.endian = BFD_ENDIAN_LITTLE;
+    }
     print_insn = print_insn_ppc;
 #elif defined(TARGET_M68K)
     print_insn = print_insn_m68k;
diff --git a/monitor.c b/monitor.c
index 342e83b..3ae561b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1309,6 +1309,10 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
             }
         }
 #endif
+#ifdef TARGET_PPC
+        flags = msr_le << 16;
+        flags |= env->bfd_mach;
+#endif
         monitor_disas(mon, env, addr, count, is_physical, flags);
         return;
     }
-- 
1.7.1

Re: [Qemu-devel] [V3 PATCH 2/2] monitor: QEMU Monitor Instruction Disassembly Incorrect for PowerPC LE Mode

[Peter Maydell]

On 8 April 2014 20:26, Tom Musta <tommusta@gmail.com> wrote:
> The monitor support for disassembling instructions does not honor the MSR[LE]
> bit for PowerPC processors.
>
> This change enhances the monitor_disas() routine by supporting a flag bit
> for Little Endian mode.  Bit 16 is used since that bit was used in the
> analagous guest disassembly routine target_disas().
>
> Also, to be consistent with target_disas(), the disassembler bfd_mach field
> can be passed in the flags argument.
>
> Reported-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Tom Musta <tommusta@gmail.com>
> ---
>
> V2: Look specifically at bit 16 for LE.  Support machine configuration in flags.
>
> V3: Changed documentation of 'flags' argument to simply refer to the target_disas
>   description (per Peter Maydell's review).
>
> The bug can be easily observed by dumping the first few instructions of an
> interrupt vector (0x300 is the Data Storage Interrupt handler in PPC)
>
>   (qemu) xp/8i 0x300
>   0x0000000000000300:  .long 0x60
>   0x0000000000000304:  lhzu    r18,-19843(r3)
>   0x0000000000000308:  .long 0x60
>   0x000000000000030c:  lhzu    r18,-20099(r2)
>   0x0000000000000310:  lwz     r0,11769(0)
>   0x0000000000000314:  lhzu    r23,8317(r2)
>   0x0000000000000318:  .long 0x7813427c
>   0x000000000000031c:  lbz     r0,19961(0)
>
> With the patch applied, the disassembly now looks correct:
>
>   (qemu) xp/8i 0x300
>   0x0000000000000300:  nop
>   0x0000000000000304:  mtsprg  2,r13
>   0x0000000000000308:  nop
>   0x000000000000030c:  mfsprg  r13,1
>   0x0000000000000310:  std     r9,128(r13)
>   0x0000000000000314:  mfspr   r9,896
>   0x0000000000000318:  mr      r2,r2
>   0x000000000000031c:  std     r10,136(r13)
>  disas.c   |   14 ++++++++++++--
>  monitor.c |    4 ++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/disas.c b/disas.c
> index a427d18..f300102 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -445,6 +445,8 @@ monitor_fprintf(FILE *stream, const char *fmt, ...)
>      return 0;
>  }
>
> +/* Disassembler for the monitor.  See target_disas for a description of 'flags'.
> + */

You could just put that */ on the previous line, right? Or is it too
long if you do?

>  void monitor_disas(Monitor *mon, CPUArchState *env,
>                     target_ulong pc, int nb_insn, int is_physical, int flags)
>  {
> @@ -485,11 +487,19 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>      s.info.mach = bfd_mach_sparc_v9b;
>  #endif
>  #elif defined(TARGET_PPC)
> +    if (flags & 0xFFFF) {
> +        /* If we have a precise definitions of the instructions set, use it */

"definition", "instruction".

Otherwise:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM