On 7 December 2016 at 14:49, Julian Brown <jul...@codesourcery.com> wrote: > Thumb-1 code has some issues in BE32 mode (as currently implemented). In > short, since bytes are swapped within words at load time for BE32 > executables, this also swaps pairs of adjacent Thumb-1 instructions. > > This patch un-swaps those pairs of instructions again, both for execution, > and for disassembly. > > Signed-off-by: Julian Brown <jul...@codesourcery.com>
Thanks. This patch looks pretty good but I have a few tweaks to suggest below. (PS: this doesn't depend on patches 1-4, right?) > --- > disas.c | 1 + > include/disas/bfd.h | 7 +++++++ > target-arm/arm_ldst.h | 10 +++++++++- > target-arm/cpu.c | 32 ++++++++++++++++++++++++++++++++ > 4 files changed, 49 insertions(+), 1 deletion(-) > > diff --git a/disas.c b/disas.c > index 67f116a..506e56f 100644 > --- a/disas.c > +++ b/disas.c > @@ -190,6 +190,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong > code, > > s.cpu = cpu; > s.info.read_memory_func = target_read_memory; > + s.info.read_memory_inner_func = NULL; > s.info.buffer_vma = code; > s.info.buffer_length = size; > s.info.print_address_func = generic_print_address; > diff --git a/include/disas/bfd.h b/include/disas/bfd.h > index 8a3488c..5c1e1c5 100644 > --- a/include/disas/bfd.h > +++ b/include/disas/bfd.h > @@ -291,6 +291,7 @@ typedef struct disassemble_info { > The bottom 16 bits are for the internal use of the disassembler. */ > unsigned long flags; > #define INSN_HAS_RELOC 0x80000000 > +#define INSN_ARM_THUMB1_BE32 0x00010000 I think we should just call this INSN_ARM_BE32, because it doesn't actually matter whether we're disassembling Thumb1 or not, only whether the disassembler wants a 16-bit quantity. > PTR private_data; > > /* Function used to get bytes to disassemble. MEMADDR is the > @@ -302,6 +303,12 @@ typedef struct disassemble_info { > (bfd_vma memaddr, bfd_byte *myaddr, int length, > struct disassemble_info *info); > > + /* A place to stash the real read_memory_func if read_memory_func wants to > + do some funky address arithmetic or similar (e.g. for ARM BE32 mode). > */ > + int (*read_memory_inner_func) > + (bfd_vma memaddr, bfd_byte *myaddr, int length, > + struct disassemble_info *info); > + > /* Function which should be called if we get an error that we can't > recover from. STATUS is the errno value from read_memory_func and > MEMADDR is the address that we were trying to read. INFO is a > diff --git a/target-arm/arm_ldst.h b/target-arm/arm_ldst.h > index a76d89f..01587b3 100644 > --- a/target-arm/arm_ldst.h > +++ b/target-arm/arm_ldst.h > @@ -39,7 +39,15 @@ static inline uint32_t arm_ldl_code(CPUARMState *env, > target_ulong addr, > static inline uint16_t arm_lduw_code(CPUARMState *env, target_ulong addr, > bool sctlr_b) > { > - uint16_t insn = cpu_lduw_code(env, addr); > + uint16_t insn; > +#ifndef CONFIG_USER_ONLY > + /* In big-endian (BE32) mode, adjacent Thumb instructions have been > swapped > + within each word. Undo that now. */ > + if (sctlr_b) { > + addr ^= 2; > + } > +#endif > + insn = cpu_lduw_code(env, addr); > if (bswap_code(sctlr_b)) { > return bswap16(insn); > } > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 6afb0d9..6099d50 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -409,6 +409,30 @@ print_insn_thumb1(bfd_vma pc, disassemble_info *info) > return print_insn_arm(pc | 1, info); > } > > +static int arm_read_memory_func(bfd_vma memaddr, bfd_byte *myaddr, > + int length, struct disassemble_info *info) > +{ > + assert(info->read_memory_inner_func); > + > + if ((info->flags & INSN_ARM_THUMB1_BE32) != 0 && length == 2) { > + int status; > + unsigned char b[4]; > + assert(info->endian == BFD_ENDIAN_LITTLE); > + status = info->read_memory_inner_func(memaddr & ~3, (bfd_byte *)b, 4, > + info); > + if ((memaddr & 2) == 0) { > + myaddr[0] = b[2]; > + myaddr[1] = b[3]; > + } else { > + myaddr[0] = b[0]; > + myaddr[1] = b[1]; > + } Is there a reason why we need to read 4 bytes and then pick 2 of them, rather than just doing a 2 byte read from the adjusted address ? Might want to at least assert that length is either 2 or 4 if BE32 (we could handle length 1 but the disassembler doesn't do length 1 requests). > + return status; > + } else { > + return info->read_memory_inner_func(memaddr, myaddr, length, info); > + } > +} > + > static void arm_disas_set_info(CPUState *cpu, disassemble_info *info) > { > ARMCPU *ac = ARM_CPU(cpu); > @@ -424,6 +448,10 @@ static void arm_disas_set_info(CPUState *cpu, > disassemble_info *info) > #endif > } else if (env->thumb) { > info->print_insn = print_insn_thumb1; > + info->flags &= ~INSN_ARM_THUMB1_BE32; > + if (arm_sctlr_b(env)) { > + info->flags |= INSN_ARM_THUMB1_BE32; > + } We should set the BE32 flag if arm_sctlr_b(), regardless of whether env->thumb or not. > } else { > info->print_insn = print_insn_arm; > } > @@ -434,6 +462,10 @@ static void arm_disas_set_info(CPUState *cpu, > disassemble_info *info) > info->endian = BFD_ENDIAN_BIG; > #endif > } > + if (info->read_memory_inner_func == NULL) { > + info->read_memory_inner_func = info->read_memory_func; > + info->read_memory_func = arm_read_memory_func; > + } > } > > static void arm_cpu_initfn(Object *obj) thanks -- PMM