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 <address@hidden> 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 <address@hidden>
> Signed-off-by: Tom Musta <address@hidden>
> ---
>
> 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 <address@hidden>

thanks
-- PMM