diff mbox series

[v2,35/68] target/arm: Convert CPS (privileged)

Message ID 20190819213755.26175-36-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 
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/translate.c       | 87 +++++++++++++++---------------------
 target/arm/a32-uncond.decode |  3 ++
 target/arm/t32.decode        |  3 ++
 3 files changed, 42 insertions(+), 51 deletions(-)

-- 
2.17.1

Comments

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

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  target/arm/translate.c       | 87 +++++++++++++++---------------------

>  target/arm/a32-uncond.decode |  3 ++

>  target/arm/t32.decode        |  3 ++

>  3 files changed, 42 insertions(+), 51 deletions(-)

> diff --git a/target/arm/t32.decode b/target/arm/t32.decode

> index 18c268e712..354ad77fe6 100644

> --- a/target/arm/t32.decode

> +++ b/target/arm/t32.decode

> @@ -44,6 +44,7 @@

>  &bfi             !extern rd rn lsb msb

>  &sat             !extern rd rn satimm imm sh

>  &pkh             !extern rd rn rm imm tb

> +&cps             !extern mode imod M A I F

>

>  # Data-processing (register)

>

> @@ -340,6 +341,8 @@ CLZ              1111 1010 1011 ---- 1111 .... 1000 ....      @rdm

>      SMC          1111 0111 1111 imm:4 1000 0000 0000 0000     &i

>      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


In T32 the CPS insn overlaps with the hint space (hint insns have
bits [10:8] all-zeroes, whereas all the valid CPS insns have either
M set or one of the imod bits set) -- why doesn't it need to be
in the same insn group as the hints? If we're going to put it
separated in the .decode file from the insns it overlaps with
I guess a comment to that effect would help so it doesn't get
inadvertently reordered with them.

CPS shouldn't exist at all for M-profile, but the legacy decoder
got this wrong too, so we should put that on the todo list for
fixing later (along, maybe, with UNDEFing on some of the
unpredictable combinations of M/imod/etc for A profile?).

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


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

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

>>

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> ---

>>  target/arm/translate.c       | 87 +++++++++++++++---------------------

>>  target/arm/a32-uncond.decode |  3 ++

>>  target/arm/t32.decode        |  3 ++

>>  3 files changed, 42 insertions(+), 51 deletions(-)

>> diff --git a/target/arm/t32.decode b/target/arm/t32.decode

>> index 18c268e712..354ad77fe6 100644

>> --- a/target/arm/t32.decode

>> +++ b/target/arm/t32.decode

>> @@ -44,6 +44,7 @@

>>  &bfi             !extern rd rn lsb msb

>>  &sat             !extern rd rn satimm imm sh

>>  &pkh             !extern rd rn rm imm tb

>> +&cps             !extern mode imod M A I F

>>

>>  # Data-processing (register)

>>

>> @@ -340,6 +341,8 @@ CLZ              1111 1010 1011 ---- 1111 .... 1000 ....      @rdm

>>      SMC          1111 0111 1111 imm:4 1000 0000 0000 0000     &i

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

> 

> In T32 the CPS insn overlaps with the hint space (hint insns have

> bits [10:8] all-zeroes, whereas all the valid CPS insns have either

> M set or one of the imod bits set) -- why doesn't it need to be

> in the same insn group as the hints? If we're going to put it

> separated in the .decode file from the insns it overlaps with

> I guess a comment to that effect would help so it doesn't get

> inadvertently reordered with them.


It is grouped.  It's not immediately visible in the patch because there are a
*lot* of insns that overlap with the hints and 3 lines of context are
insufficient to see that.

But the grouping is semi-visible in the indentation here.

> CPS shouldn't exist at all for M-profile, but the legacy decoder

> got this wrong too, so we should put that on the todo list for

> fixing later (along, maybe, with UNDEFing on some of the

> unpredictable combinations of M/imod/etc for A profile?).


Fixing m-profile is just as easy as as commenting.
I'll leave a TODO for the unpredictable combinations.


r~
Richard Henderson Aug. 25, 2019, 5:40 p.m. UTC | #3 
On 8/25/19 10:28 AM, Richard Henderson wrote:
>> CPS shouldn't exist at all for M-profile, but the legacy decoder

>> got this wrong too, so we should put that on the todo list for

>> fixing later (along, maybe, with UNDEFing on some of the

>> unpredictable combinations of M/imod/etc for A profile?).

> Fixing m-profile is just as easy as as commenting.

> I'll leave a TODO for the unpredictable combinations.


There's also a missing check for ARMv6 here.

That got added later, with the T16 decode.  I have just added the
following to the T16 commit message:

    target/arm: Convert T16, Change processor state

    Add a check for ARMv6 in trans_CPS.  We had this correct in
    the T16 path, but had previously forgotten the check on the
    A32 and T32 paths.


r~
Peter Maydell Aug. 25, 2019, 8:43 p.m. UTC | #4 
On Sun, 25 Aug 2019 at 18:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 8/25/19 9:20 AM, Peter Maydell wrote:

> > On Mon, 19 Aug 2019 at 22:38, Richard Henderson

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

> >>

> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> >> ---

> >>  target/arm/translate.c       | 87 +++++++++++++++---------------------

> >>  target/arm/a32-uncond.decode |  3 ++

> >>  target/arm/t32.decode        |  3 ++

> >>  3 files changed, 42 insertions(+), 51 deletions(-)

> >> diff --git a/target/arm/t32.decode b/target/arm/t32.decode

> >> index 18c268e712..354ad77fe6 100644

> >> --- a/target/arm/t32.decode

> >> +++ b/target/arm/t32.decode

> >> @@ -44,6 +44,7 @@

> >>  &bfi             !extern rd rn lsb msb

> >>  &sat             !extern rd rn satimm imm sh

> >>  &pkh             !extern rd rn rm imm tb

> >> +&cps             !extern mode imod M A I F

> >>

> >>  # Data-processing (register)

> >>

> >> @@ -340,6 +341,8 @@ CLZ              1111 1010 1011 ---- 1111 .... 1000 ....      @rdm

> >>      SMC          1111 0111 1111 imm:4 1000 0000 0000 0000     &i

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

> >

> > In T32 the CPS insn overlaps with the hint space (hint insns have

> > bits [10:8] all-zeroes, whereas all the valid CPS insns have either

> > M set or one of the imod bits set) -- why doesn't it need to be

> > in the same insn group as the hints? If we're going to put it

> > separated in the .decode file from the insns it overlaps with

> > I guess a comment to that effect would help so it doesn't get

> > inadvertently reordered with them.

>

> It is grouped.  It's not immediately visible in the patch because there are a

> *lot* of insns that overlap with the hints and 3 lines of context are

> insufficient to see that.

>

> But the grouping is semi-visible in the indentation here.


I'm still confused, I think. The hint space is
+  NOP            1111 0011 1010 1111 1000 0000 ---- ----
(plus the more specific hint insns before that pattern with
fixed values in the [7:0] bits).
CPS falls into that space; but you've placed it with
SMC and HVC which don't fall into the hint space, because
they have 0111 in bits [27:24], not 0011.

thanks
-- PMM
Richard Henderson Aug. 26, 2019, 1:10 a.m. UTC | #5 
On 8/25/19 1:43 PM, Peter Maydell wrote:
> I'm still confused, I think. The hint space is

> +  NOP            1111 0011 1010 1111 1000 0000 ---- ----

> (plus the more specific hint insns before that pattern with

> fixed values in the [7:0] bits).

> CPS falls into that space; but you've placed it with

> SMC and HVC which don't fall into the hint space, because

> they have 0111 in bits [27:24], not 0011.


Oops.  I see what you mean.


r~
Richard Henderson Aug. 26, 2019, 1:36 a.m. UTC | #6 
On 8/25/19 6:10 PM, Richard Henderson wrote:
> On 8/25/19 1:43 PM, Peter Maydell wrote:

>> I'm still confused, I think. The hint space is

>> +  NOP            1111 0011 1010 1111 1000 0000 ---- ----

>> (plus the more specific hint insns before that pattern with

>> fixed values in the [7:0] bits).

>> CPS falls into that space; but you've placed it with

>> SMC and HVC which don't fall into the hint space, because

>> they have 0111 in bits [27:24], not 0011.

> 

> Oops.  I see what you mean.


So, I've moved the line up immediately following the hint space, and added a
comment:

+    # If imod == '00' && M == '0' then SEE "Hint instructions", above.
+    CPS          1111 0011 1010 1111 1000 0 imod:2 M:1 A:1 I:1 F:1 mode:5 \
+                 &cps

The line *was* still within the same group (which is large), so it doesn't
actually make a difference to the decode, but I do agree it makes more sense in
the new position.


r~
diff mbox series

Patch

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6489bbc09c..928205d993 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10038,6 +10038,40 @@  static bool trans_SRS(DisasContext *s, arg_SRS *a)
     return true;
 }
 
+static bool trans_CPS(DisasContext *s, arg_CPS *a)
+{
+    uint32_t mask, val;
+
+    if (IS_USER(s)) {
+        /* Implemented as NOP in user mode.  */
+        return true;
+    }
+
+    mask = val = 0;
+    if (a->imod & 2) {
+        if (a->A) {
+            mask |= CPSR_A;
+        }
+        if (a->I) {
+            mask |= CPSR_I;
+        }
+        if (a->F) {
+            mask |= CPSR_F;
+        }
+        if (a->imod & 1) {
+            val |= mask;
+        }
+    }
+    if (a->M) {
+        mask |= CPSR_M;
+        val |= a->mode;
+    }
+    if (mask) {
+        gen_set_psr_im(s, mask, 0, val);
+    }
+    return true;
+}
+
 /*
  * Clear-Exclusive, Barriers
  */
@@ -10209,31 +10243,6 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
             ARCH(5TE);
         } else if ((insn & 0x0f000010) == 0x0e000010) {
             /* Additional coprocessor register transfer.  */
-        } else if ((insn & 0x0ff10020) == 0x01000000) {
-            uint32_t mask;
-            uint32_t val;
-            /* cps (privileged) */
-            if (IS_USER(s))
-                return;
-            mask = val = 0;
-            if (insn & (1 << 19)) {
-                if (insn & (1 << 8))
-                    mask |= CPSR_A;
-                if (insn & (1 << 7))
-                    mask |= CPSR_I;
-                if (insn & (1 << 6))
-                    mask |= CPSR_F;
-                if (insn & (1 << 18))
-                    val |= mask;
-            }
-            if (insn & (1 << 17)) {
-                mask |= CPSR_M;
-                val |= (insn & 0x1f);
-            }
-            if (mask) {
-                gen_set_psr_im(s, mask, 0, val);
-            }
-            return;
         }
         goto illegal_op;
     }
@@ -10342,7 +10351,6 @@  static bool thumb_insn_is_16bit(DisasContext *s, uint32_t pc, uint32_t insn)
 /* Translate a 32-bit thumb instruction. */
 static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
 {
-    uint32_t imm, offset;
     uint32_t rd, rn, rm, rs;
     TCGv_i32 tmp;
     TCGv_i32 addr;
@@ -10618,31 +10626,8 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                     case 0: /* msr cpsr, in decodetree  */
                     case 1: /* msr spsr, in decodetree  */
                         goto illegal_op;
-                    case 2: /* cps, nop-hint.  */
-                        /* nop hints in decodetree */
-                        /* Implemented as NOP in user mode.  */
-                        if (IS_USER(s))
-                            break;
-                        offset = 0;
-                        imm = 0;
-                        if (insn & (1 << 10)) {
-                            if (insn & (1 << 7))
-                                offset |= CPSR_A;
-                            if (insn & (1 << 6))
-                                offset |= CPSR_I;
-                            if (insn & (1 << 5))
-                                offset |= CPSR_F;
-                            if (insn & (1 << 9))
-                                imm = CPSR_A | CPSR_I | CPSR_F;
-                        }
-                        if (insn & (1 << 8)) {
-                            offset |= 0x1f;
-                            imm |= (insn & 0x1f);
-                        }
-                        if (offset) {
-                            gen_set_psr_im(s, offset, 0, imm);
-                        }
-                        break;
+                    case 2: /* cps, nop-hint, in decodetree */
+                        goto illegal_op;
                     case 3: /* Special control operations, in decodetree */
                     case 4: /* bxj, in decodetree */
                         goto illegal_op;
diff --git a/target/arm/a32-uncond.decode b/target/arm/a32-uncond.decode
index b077958cec..eb1c55b330 100644
--- a/target/arm/a32-uncond.decode
+++ b/target/arm/a32-uncond.decode
@@ -35,9 +35,12 @@  BLX_i            1111 101 . ........................          &i imm=%imm24h
 
 &rfe             rn w pu
 &srs             mode w pu
+&cps             mode imod M A I F
 
 RFE              1111 100 pu:2 0 w:1 1 rn:4 0000 1010 0000 0000   &rfe
 SRS              1111 110 pu:2 1 w:1 0 1101 0000 0101 000 mode:5  &srs
+CPS              1111 0001 0000 imod:2 M:1 0 0000 000 A:1 I:1 F:1 0 mode:5 \
+                 &cps
 
 # Clear-Exclusive, Barriers
 
diff --git a/target/arm/t32.decode b/target/arm/t32.decode
index 18c268e712..354ad77fe6 100644
--- a/target/arm/t32.decode
+++ b/target/arm/t32.decode
@@ -44,6 +44,7 @@ 
 &bfi             !extern rd rn lsb msb
 &sat             !extern rd rn satimm imm sh
 &pkh             !extern rd rn rm imm tb
+&cps             !extern mode imod M A I F
 
 # Data-processing (register)
 
@@ -340,6 +341,8 @@  CLZ              1111 1010 1011 ---- 1111 .... 1000 ....      @rdm
     SMC          1111 0111 1111 imm:4 1000 0000 0000 0000     &i
     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