Re: [Qemu-devel] [PATCH] arm: Ensure LSB of BLX is set

[Peter Maydell]

On 6 July 2015 at 19:09,  <address@hidden> wrote:
> From: Meador Inge <address@hidden>
>
> This small patch adds a sanity check when disassembling
> the BLX instruction.  The use case came to light when
> doing toolchain development and a similar check was
> upstreamed for Binutils:
>
>   * https://sourceware.org/ml/binutils/2011-01/msg00077.html
>
> Patch by Nathan Sidwell.
>
> Signed-off-by: Meador Inge <address@hidden>
> ---
>  target-arm/translate.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 69ac18c..fedc8f3 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -9912,6 +9912,12 @@ static int disas_thumb2_insn(CPUARMState *env, 
> DisasContext *s, uint16_t insn_hw
>                      gen_jmp(s, offset);
>                  } else {
>                      /* blx */
> +                    /* The instruction must have bit zero unset, even
> +                       though it is part of the offset.  Real hardware
> +                       will abort, so we do too.  */

This comment isn't really correct -- bit zero in this encoding
(BLX T2) isn't part of the immediate, it's described
as a one bit field H which causes UNDEFINED if it's 1.
This is architecturally mandated, not just somewhere
we're following h/w on an IMPDEF or UNPREDICTABLE case.

> +                    if (insn & 1) {
> +                        goto illegal_op;
> +                    }

This check is happening too late -- we've already
done the write to R14. We need to UNDEF before that.

>                      offset &= ~(uint32_t)2;

The new check makes this masking unnecessary now, right?

>                      /* thumb2 bx, no need to check */
>                      gen_bx_im(s, offset);
> --

thanks
-- PMM