Re: [Qemu-devel] [PATCH 1/2] target/arm: Implement ARMv8.0-SB

[Peter Maydell]

On Wed, 20 Feb 2019 at 23:50, Richard Henderson
<address@hidden> wrote:
>
> Signed-off-by: Richard Henderson <address@hidden>


> @@ -9192,6 +9192,17 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>                   */
>                  gen_goto_tb(s, 0, s->pc & ~1);
>                  return;
> +            case 7: /* sb */
> +                if (!dc_isar_feature(aa32_sb, s)) {
> +                    goto illegal_op;
> +                }
> +                /*
> +                 * TODO: There is no speculation barrier opcode
> +                 * for TCG; MB and end the TB instead.
> +                 */
> +                tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
> +                s->base.is_jmp = DISAS_TOO_MANY;

Why do we do the "end the TB" code differently here than we
do for the implementation of ISB in the case immediately
above ?

In the A32 encoding bits [3:0] are "(0)", so we should check that
they're 0 and UNDEF if not.


> +                return;
>              default:
>                  goto illegal_op;
>              }
> @@ -11810,6 +11821,17 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>                               */
>                              gen_goto_tb(s, 0, s->pc & ~1);
>                              break;
> +                        case 7: /* sb */
> +                            if (!dc_isar_feature(aa32_sb, s)) {
> +                                goto illegal_op;
> +                            }
> +                            /*
> +                             * TODO: There is no speculation barrier opcode
> +                             * for TCG; MB and end the TB instead.
> +                             */
> +                            tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
> +                            s->base.is_jmp = DISAS_TOO_MANY;

Similarly here: inconsistency about how we end the TB, and
not checking the [3:0] bits for being zero.

(We also I think are not fully decoding some of the other
sbz/sbo fields for insns in this group, but that's more of
an existing bug than a new one.)

> +                            break;
>                          default:
>                              goto illegal_op;
>                          }
> --
> 2.17.2

thanks
-- PMM