qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 5/6] Fix Thumb-1 BE32 execution and disassemb


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v2 5/6] Fix Thumb-1 BE32 execution and disassembly.
Date: Thu, 5 Jan 2017 18:18:55 +0000

On 7 December 2016 at 14:49, Julian Brown <address@hidden> 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 <address@hidden>

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]