Re: [Qemu-devel] [PATCH v2 62/68] target/arm: Convert T16, Miscellaneous 16-bit instructions

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 19 Aug 2019 at 22:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

> diff --git a/target/arm/t16.decode b/target/arm/t16.decode
> index 98d60952a1..4ecbabd364 100644
> --- a/target/arm/t16.decode
> +++ b/target/arm/t16.decode
> @@ -210,20 +210,33 @@ REVSH           1011 1010 11 ... ...            @rdm
>
>  # Hints
>
> +%it_cond        5:3 !function=times_2
> +
>  {
> -  YIELD         1011 1111 0001 0000
> -  WFE           1011 1111 0010 0000
> -  WFI           1011 1111 0011 0000
> +  {
> +    YIELD       1011 1111 0001 0000
> +    WFE         1011 1111 0010 0000
> +    WFI         1011 1111 0011 0000
>
> -  # TODO: Implement SEV, SEVL; may help SMP performance.
> -  # SEV         1011 1111 0100 0000
> -  # SEVL        1011 1111 0101 0000
> +    # TODO: Implement SEV, SEVL; may help SMP performance.
> +    # SEV       1011 1111 0100 0000
> +    # SEVL      1011 1111 0101 0000
>
> -  # The canonical nop has the second nibble as 0000, but the whole of the
> -  # rest of the space is a reserved hint, behaves as nop.
> -  NOP           1011 1111 ---- 0000
> +    # The canonical nop has the second nibble as 0000, but the whole of the
> +    # rest of the space is a reserved hint, behaves as nop.
> +    NOP         1011 1111 ---- 0000
> +  }
> +  IT            1011 1111 ... imm:5             &ci cond=%it_cond

This is correct (same behaviour as the old decoder, but
it looks a bit odd here because it's not the same as
the fields defined by the architecture (in particular the
'cond' field is not the same set of bits as the 'firstcond'
field). We could maybe comment it:

  # Bits 7:0 in IT are architecturally simply the
  # new PSTATE.IT bits (despite the instruction description
  # splitting them into 'firstcond' and 'mask' fields).
  # In QEMU during translation we track the IT bits using
  # the DisasContext fields condexec_cond and condexec_mask,
  # so here we massage the bits from the insn into the form
  # that that optimization requires.

(Or equivalently we could just pass a single 8 bit immediate
to the trans_IT function and split it out there, I dunno.)

>  }
>
> +# Miscellaneous 16-bit instructions
> +
> +%imm6_9_3       9:1 3:5 !function=times_2

Would it be worth adding support to the decodetree script
for letting you specify fixed bits in this kind of field-decode,
so we could write '9:1 3:5 0' rather than having to specify
a multiply-by-2 function to put the 0 bit in ? Or is it
not likely to be common enough to be worth bothering with?
(Not something for this series, anyway.)

> +
> +HLT             1011 1010 10 imm:6              &i
> +BKPT            1011 1110 imm:8                 &i
> +CBZ             1011 nz:1 0.1 ..... rn:3        imm=%imm6_9_3
> +
>  # Push and Pop
>
>  %push_list      0:9 !function=t16_push_list
> --

In any case
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM