Re: [Qemu-devel] [PATCH v2 41/68] target/arm: Convert TT

[Peter Maydell]

On Mon, 19 Aug 2019 at 22:38, Richard Henderson
<address@hidden> wrote:
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate.c | 87 +++++++++++++-----------------------------
>  target/arm/t32.decode  |  5 ++-
>  2 files changed, 31 insertions(+), 61 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 9a8864e8ff..d1078ca1ec 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8454,6 +8454,30 @@ static bool trans_SG(DisasContext *s, arg_SG *a)
>      return true;
>  }
>
> +static bool trans_TT(DisasContext *s, arg_TT *a)
> +{
> +    TCGv_i32 addr, tmp;
> +
> +    if (!arm_dc_feature(s, ARM_FEATURE_M) ||
> +        !arm_dc_feature(s, ARM_FEATURE_V8)) {
> +        return false;
> +    }
> +    if (a->rd == 13 || a->rd == 15 || a->rn == 15) {
> +        /* We UNDEF for these UNPREDICTABLE cases */
> +        return false;
> +    }
> +    if (a->A && !s->v8m_secure) {
> +        return false;
> +    }
> +
> +    addr = load_reg(s, a->rn);
> +    tmp = tcg_const_i32((a->A << 1) | a->T);
> +    gen_helper_v7m_tt(tmp, cpu_env, addr, tmp);
> +    tcg_temp_free_i32(addr);
> +    store_reg(s, a->rd, tmp);
> +    return true;
> +}

> diff --git a/target/arm/t32.decode b/target/arm/t32.decode
> index ce46650446..bb875f77b0 100644
> --- a/target/arm/t32.decode
> +++ b/target/arm/t32.decode
> @@ -506,7 +506,10 @@ STRD_ri_t32      1110 1001 .110 .... .... .... ........  
>   @ldstd_ri8 w=1 p=1
>  @ldrex_d         .... .... .... rn:4 rt:4 rt2:4 .... .... \
>                   &ldrex imm=0
>
> -STREX            1110 1000 0100 .... .... .... .... ....      @strex_i
> +{
> +  TT             1110 1000 0100 rn:4 1111 rd:4 A:1 T:1 000000
> +  STREX          1110 1000 0100 .... .... .... .... ....      @strex_i
> +}

This patch turns out to have a bug -- there are cases in
trans_TT which are supposed to UNDEF. We return 'false' in
those cases, which means we fall through to trans_STREX(),
which doesn't have any checks in it to make them actually
UNDEF, so we end up generating code for some kind of STREX.
(Interestingly, I wrote the notes in the other email I
sent earlier about returning false vs unallocated_encoding+true
before I found and diagnosed the cause of this bug.)

thanks
-- PMM