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

[Peter Maydell]

On 7 April 2014 15:50, 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().

It seems a touch dubious to assume the area of memory
being dissassembled is necessarily the same endianness
the CPU happens to be currently...

It would be nice to fix the comment at the top of
target_disas() which currently claims something totally
different and wrong for the PPC flags semantics.

> Reported-by: Anton Blanchard <address@hidden>
> Signed-off-by: Tom Musta <address@hidden>
>
> ---
>
> 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   |    3 +++
>  monitor.c |    3 +++
>  2 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/disas.c b/disas.c
> index 79e6944..c7804b2 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -489,6 +489,9 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>  #else
>      s.info.mach = bfd_mach_ppc;
>  #endif
> +    if (flags >> 16) {
> +        s.info.endian = BFD_ENDIAN_LITTLE;
> +    }

If you do this then this code will need to change
if you ever add another flag; better to just
test bit 16. (The code in target_disas() for PPC
also gets this wrong.)

Should monitor_disas() support pulling the info.mach
from the flags the way target_disas() does?

thanks
-- PMM