diff mbox series

[v2,18/68] target/arm: Convert the rest of A32 Miscelaneous instructions

Message ID 20190819213755.26175-19-richard.henderson@linaro.org
State Superseded
Headers show
Series target/arm: Convert aa32 base isa to decodetree | expand

Commit Message

Richard Henderson Aug. 19, 2019, 9:37 p.m. UTC
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 <richard.henderson@linaro.org>

---
 target/arm/translate.c | 119 +++++++++++++----------------------------
 target/arm/a32.decode  |   8 +++
 target/arm/t32.decode  |   5 ++
 3 files changed, 50 insertions(+), 82 deletions(-)

-- 
2.17.1

Comments

Peter Maydell Aug. 23, 2019, 12:03 p.m. UTC | #1
On Mon, 19 Aug 2019 at 22:38, Richard Henderson
<richard.henderson@linaro.org> wrote:

In subject, typo: "Miscellaneous".

> This fixes an exiting bug with the T5 encoding of SUBS PC, LR, #IMM,


"existing"

> in that it may be executed from user mode as with any other encoding

> of SUBS, not as ERET.


Should this paragraph be in the commit message for the previous
patch? This change doesn't touch SUBS/ERET.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


thanks
-- PMM
Richard Henderson Aug. 23, 2019, 2:33 p.m. UTC | #2
On 8/23/19 5:03 AM, Peter Maydell wrote:
> On Mon, 19 Aug 2019 at 22:38, Richard Henderson

> <richard.henderson@linaro.org> wrote:

> 

> In subject, typo: "Miscellaneous".

> 

>> This fixes an exiting bug with the T5 encoding of SUBS PC, LR, #IMM,

> 

> "existing"

> 

>> in that it may be executed from user mode as with any other encoding

>> of SUBS, not as ERET.

> 

> Should this paragraph be in the commit message for the previous

> patch? This change doesn't touch SUBS/ERET.

> 

> Otherwise

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


Indeed that text should not be here.

And also, as you might be able to tell from the text of the previous patch, it
isn't an existing bug.  It took me a while to see that, and that's why I think
that passing pre-v7m through the usual SUBS path is clearer.


r~
Peter Maydell Aug. 27, 2019, 10:32 a.m. UTC | #3
On Mon, 19 Aug 2019 at 22:38, Richard Henderson
<richard.henderson@linaro.org> 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 <richard.henderson@linaro.org>

> ---

>  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
Richard Henderson Aug. 27, 2019, 8:01 p.m. UTC | #4
On 8/27/19 3:32 AM, Peter Maydell wrote:
>> +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.


Correct.

> 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.


Correct.

> 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) ?


I agree this should be the rule.

E.g. for this IS_USER case, we have successfully decoded HVC and so should not
return false.  The fact that HVC should raise SIGILL if IS_USER should not be
confused with decoding HVC.

Other constraints, such as rd != 15 or imod != 0, should continue to return
false so that a (potential) grouped insn can match.

> 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.


Yes.


r~
Richard Henderson Aug. 27, 2019, 10:29 p.m. UTC | #5
On 8/27/19 1:01 PM, Richard Henderson wrote:
> Other constraints, such as rd != 15 or imod != 0, should continue to return

> false so that a (potential) grouped insn can match.


Eh.  This is not the answer that the TT example suggests.

So far we are able to order the grouped insns such that
decoding directives like

    if t == 15 then SEE "TT";

are respected.  Since we do not generally do a very good
job of diagnosing all of the UNPREDICTABLE behavior, we
should not rely on getting all of it, e.g. by requiring
that if TT diagnoses some UNPRED that STREX also diagnoses
similar UNPRED.

I'm going to walk through the patch set and fix these.


r~
diff mbox series

Patch

diff --git a/target/arm/translate.c b/target/arm/translate.c
index cb7b35489f..cb6296dc12 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8491,6 +8491,39 @@  static bool trans_ERET(DisasContext *s, arg_ERET *a)
     return true;
 }
 
+static bool trans_HLT(DisasContext *s, arg_HLT *a)
+{
+    gen_hlt(s, a->imm);
+    return true;
+}
+
+static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
+{
+    if (!ENABLE_ARCH_5) {
+        return false;
+    }
+    gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
+    return true;
+}
+
+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;
+}
+
+static bool trans_SMC(DisasContext *s, arg_SMC *a)
+{
+    if (!ENABLE_ARCH_6K || IS_USER(s) || arm_dc_feature(s, ARM_FEATURE_M)) {
+        return false;
+    }
+    gen_smc(s);
+    return true;
+}
+
 /*
  * Legacy decoder.
  */
@@ -8771,68 +8804,8 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
     } else if ((insn & 0x0f900000) == 0x01000000
                && (insn & 0x00000090) != 0x00000090) {
         /* miscellaneous instructions */
-        op1 = (insn >> 21) & 3;
-        sh = (insn >> 4) & 0xf;
-        rm = insn & 0xf;
-        switch (sh) {
-        case 0x0:
-            /* MSR/MRS (banked/register) */
-            /* All done in decodetree.  Illegal ops already signalled.  */
-            g_assert_not_reached();
-        case 0x1: /* bx, clz */
-        case 0x2: /* bxj */
-        case 0x3: /* blx */
-        case 0x4: /* crc32 */
-            /* All done in decodetree.  Illegal ops reach here.  */
-            goto illegal_op;
-        case 0x5: /* Saturating addition and subtraction.  */
-        case 0x6: /* ERET */
-            /* All done in decodetree.  Reach here for illegal ops.  */
-            goto illegal_op;
-        case 7:
-        {
-            int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
-            switch (op1) {
-            case 0:
-                /* HLT */
-                gen_hlt(s, imm16);
-                break;
-            case 1:
-                /* bkpt */
-                ARCH(5);
-                gen_exception_bkpt_insn(s, syn_aa32_bkpt(imm16, false));
-                break;
-            case 2:
-                /* Hypervisor call (v7) */
-                ARCH(7);
-                if (IS_USER(s)) {
-                    goto illegal_op;
-                }
-                gen_hvc(s, imm16);
-                break;
-            case 3:
-                /* Secure monitor call (v6+) */
-                ARCH(6K);
-                if (IS_USER(s)) {
-                    goto illegal_op;
-                }
-                gen_smc(s);
-                break;
-            default:
-                g_assert_not_reached();
-            }
-            break;
-        }
-        case 0x8:
-        case 0xa:
-        case 0xc:
-        case 0xe:
-            /* Halfword multiply and multiply accumulate.  */
-            /* All done in decodetree.  Reach here for illegal ops.  */
-            goto illegal_op;
-        default:
-            goto illegal_op;
-        }
+        /* All done in decodetree.  Illegal ops reach here.  */
+        goto illegal_op;
     } else if (((insn & 0x0e000000) == 0 &&
                 (insn & 0x00000090) != 0x90) ||
                ((insn & 0x0e000000) == (1 << 25))) {
@@ -10493,26 +10466,8 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                     goto illegal_op;
 
                 if (insn & (1 << 26)) {
-                    if (arm_dc_feature(s, ARM_FEATURE_M)) {
-                        goto illegal_op;
-                    }
-                    if (!(insn & (1 << 20))) {
-                        /* Hypervisor call (v7) */
-                        int imm16 = extract32(insn, 16, 4) << 12
-                            | extract32(insn, 0, 12);
-                        ARCH(7);
-                        if (IS_USER(s)) {
-                            goto illegal_op;
-                        }
-                        gen_hvc(s, imm16);
-                    } else {
-                        /* Secure monitor call (v6+) */
-                        ARCH(6K);
-                        if (IS_USER(s)) {
-                            goto illegal_op;
-                        }
-                        gen_smc(s);
-                    }
+                    /* hvc, smc, in decodetree */
+                    goto illegal_op;
                 } else {
                     op = (insn >> 20) & 7;
                     switch (op) {
diff --git a/target/arm/a32.decode b/target/arm/a32.decode
index 52a66dd1d5..c7f156be6d 100644
--- a/target/arm/a32.decode
+++ b/target/arm/a32.decode
@@ -31,6 +31,7 @@ 
 &rrr             rd rn rm
 &rr              rd rm
 &r               rm
+&i               imm
 &msr_reg         rn r mask
 &mrs_reg         rd r
 &msr_bank        rn r sysm
@@ -196,9 +197,11 @@  CRC32CW          .... 0001 0100 .... .... 0010 0100 ....      @rndm
 # Miscellaneous instructions
 
 %sysm            8:1 16:4
+%imm16_8_0       8:12 0:4
 
 @rm              ---- .... .... .... .... .... .... rm:4      &r
 @rdm             ---- .... .... .... rd:4 .... .... rm:4      &rr
+@i16             ---- .... .... .... .... .... .... ....      &i imm=%imm16_8_0
 
 MRS_bank         ---- 0001 0 r:1 00 .... rd:4 001. 0000 0000  &mrs_bank %sysm
 MSR_bank         ---- 0001 0 r:1 10 .... 1111 001. 0000 rn:4  &msr_bank %sysm
@@ -213,3 +216,8 @@  BLX_r            .... 0001 0010 1111 1111 1111 0011 ....      @rm
 CLZ              .... 0001 0110 1111 .... 1111 0001 ....      @rdm
 
 ERET             ---- 0001 0110 0000 0000 0000 0110 1110
+
+HLT              .... 0001 0000 .... .... .... 0111 ....      @i16
+BKPT             .... 0001 0010 .... .... .... 0111 ....      @i16
+HVC              .... 0001 0100 .... .... .... 0111 ....      @i16
+SMC              ---- 0001 0110 0000 0000 0000 0111 imm:4     &i
diff --git a/target/arm/t32.decode b/target/arm/t32.decode
index 6236d28b99..5116c6165a 100644
--- a/target/arm/t32.decode
+++ b/target/arm/t32.decode
@@ -28,6 +28,7 @@ 
 &rrr             !extern rd rn rm
 &rr              !extern rd rm
 &r               !extern rm
+&i               !extern imm
 &msr_reg         !extern rn r mask
 &mrs_reg         !extern rd r
 &msr_bank        !extern rn r sysm
@@ -189,6 +190,7 @@  CLZ              1111 1010 1011 ---- 1111 .... 1000 ....      @rdm
 
 %msr_sysm        4:1 8:4
 %mrs_sysm        4:1 16:4
+%imm16_16_0      16:4 0:12
 
 {
   {
@@ -226,4 +228,7 @@  CLZ              1111 1010 1011 ---- 1111 .... 1000 ....      @rdm
     SUB_rri      1111 0011 1101 1110 1000 1111 imm:8 \
                  &s_rri_rot rot=0 s=1 rd=15 rn=14
   }
+  SMC            1111 0111 1111 imm:4 1000 0000 0000 0000     &i
+  HVC            1111 0111 1110 ....  1000 .... .... ....     \
+                 &i imm=%imm16_16_0
 }