On 8 April 2014 20:26, Tom Musta <tommu...@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 <an...@samba.org>
> Signed-off-by: Tom Musta <tommu...@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.mayd...@linaro.org>
thanks
-- PMM
