Re: [Qemu-devel] [PATCH v2 18/68] target/arm: Convert the rest of A32 Miscelaneous instructions

[Peter Maydell]

On Mon, 19 Aug 2019 at 22:38, Richard Henderson
<address@hidden> wrote:
>
> This fixes an exiting bug with the T5 encoding of SUBS PC, LR, #IMM,
> in that it may be executed from user mode as with any other encoding
> of SUBS, not as ERET.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate.c | 119 +++++++++++++----------------------------
>  target/arm/a32.decode  |   8 +++
>  target/arm/t32.decode  |   5 ++
>  3 files changed, 50 insertions(+), 82 deletions(-)


> +static bool trans_HVC(DisasContext *s, arg_HVC *a)
> +{
> +    if (!ENABLE_ARCH_7 || IS_USER(s) || arm_dc_feature(s, ARM_FEATURE_M)) {
> +        return false;
> +    }
> +    gen_hvc(s, a->imm);
> +    return true;
> +}

I was wondering about for these trans_ functions the
difference between returning 'false' and calling
unallocated_encoding() and then returning 'true'.
If I understand the decodetree right this will only
make a difference when the pattern is inside a {} group.
So for instance here we have

{
  [...]
  {
    HVC          1111 0111 1110 ....  1000 .... .... ....     \
                 &i imm=%imm16_16_0
    CPS          1111 0011 1010 1111 1000 0 imod:2 M:1 A:1 I:1 F:1 mode:5 \
                 &cps
    UDF          1111 0111 1111 ----  1010 ---- ---- ----
  }
  B_cond_thumb   1111 0. cond:4 ...... 10.0 ............      &ci imm=%imm21
}

which means that if the HVC returns 'false' we'll end up
trying the insn as a B_cond_thumb. In this case the
trans function for the B_cond_thumb pattern will correctly
return false itself for a->cond >= 0xe, so it makes no
difference. But maybe it would be more robust for a pattern
like HVC to be self-contained so it doesn't fall through
for cases that really do belong to it but happen to be
required to UNDEF (like IS_USER() == true) ?

OTOH I suppose you could say that when you're writing patterns
like the B_cond_thumb one you know you've underdecoded and must
catch all the theoretical overlaps by writing checks in the trans
function, so as long as you do that correctly you're fine.

I guess this isn't a request for a change, just wondering
if there is a general principle for how we should structure
this sort of thing. I didn't run into it with the VFP stuff
because none of the decode needed groups.

thanks
-- PMM