For all, rm == 15 is invalid.
Prior to v8, thumb with rm == 13 is invalid.
For PLDW, rn == 15 is invalid.

Fixes a RISU mismatch for the HINTSPACE pattern in t32.risu
compared to a neoverse-n1 host.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/tcg/a32-uncond.decode |  8 +++--
 target/arm/tcg/t32.decode        |  7 ++--
 target/arm/tcg/translate.c       | 57 ++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/target/arm/tcg/a32-uncond.decode b/target/arm/tcg/a32-uncond.decode
index 2339de2e94..e1b1780d37 100644
--- a/target/arm/tcg/a32-uncond.decode
+++ b/target/arm/tcg/a32-uncond.decode
@@ -24,7 +24,9 @@
 
 &empty           !extern
 &i               !extern imm
+&r               !extern rm
 &setend          E
+&nm              rn rm
 
 # Branch with Link and Exchange
 
@@ -61,9 +63,9 @@ PLD              1111 0101 -101 ---- 1111 ---- ---- ----    # (imm, lit) 5te
 PLDW             1111 0101 -001 ---- 1111 ---- ---- ----    # (imm, lit) 7mp
 PLI              1111 0100 -101 ---- 1111 ---- ---- ----    # (imm, lit) 7
 
-PLD              1111 0111 -101 ---- 1111 ----- -- 0 ----   # (register) 5te
-PLDW             1111 0111 -001 ---- 1111 ----- -- 0 ----   # (register) 7mp
-PLI              1111 0110 -101 ---- 1111 ----- -- 0 ----   # (register) 7
+PLD_rr           1111 0111 -101 ---- 1111 ----- -- 0 rm:4   &r
+PLDW_rr          1111 0111 -001 rn:4 1111 ----- -- 0 rm:4   &nm
+PLI_rr           1111 0110 -101 ---- 1111 ----- -- 0 rm:4   &r
 
 # Unallocated memory hints
 #
diff --git a/target/arm/tcg/t32.decode b/target/arm/tcg/t32.decode
index d327178829..1ec12442a4 100644
--- a/target/arm/tcg/t32.decode
+++ b/target/arm/tcg/t32.decode
@@ -28,6 +28,7 @@
 &rrr_rot         !extern rd rn rm rot
 &rrr             !extern rd rn rm
 &rr              !extern rd rm
+&nm              !extern rn rm
 &ri              !extern rd imm
 &r               !extern rm
 &i               !extern imm
@@ -472,7 +473,7 @@ STR_ri           1111 1000 1100 .... .... ............        @ldst_ri_pos
   }
   LDRBT_ri       1111 1000 0001 .... .... 1110 ........       @ldst_ri_unp
   {
-    PLD          1111 1000 0001 ---- 1111 000000 -- ----      # (register)
+    PLD_rr       1111 1000 0001 ---- 1111 000000 -- rm:4      &r
     LDRB_rr      1111 1000 0001 .... .... 000000 .. ....      @ldst_rr
   }
 }
@@ -492,7 +493,7 @@ STR_ri           1111 1000 1100 .... .... ............        @ldst_ri_pos
   }
   LDRHT_ri       1111 1000 0011 .... .... 1110 ........       @ldst_ri_unp
   {
-    PLDW         1111 1000 0011 ---- 1111 000000 -- ----      # (register)
+    PLDW_rr      1111 1000 0011 rn:4 1111 000000 -- rm:4      &nm
     LDRH_rr      1111 1000 0011 .... .... 000000 .. ....      @ldst_rr
   }
 }
@@ -520,7 +521,7 @@ STR_ri           1111 1000 1100 .... .... ............        @ldst_ri_pos
   }
   LDRSBT_ri      1111 1001 0001 .... .... 1110 ........       @ldst_ri_unp
   {
-    PLI          1111 1001 0001 ---- 1111 000000 -- ----      # (register)
+    PLI_rr       1111 1001 0001 ---- 1111 000000 -- rm:4      &r
     LDRSB_rr     1111 1001 0001 .... .... 000000 .. ....      @ldst_rr
   }
 }
diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index 187eacffd9..7c09153b6e 100644
--- a/target/arm/tcg/translate.c
+++ b/target/arm/tcg/translate.c
@@ -8775,6 +8775,63 @@ static bool trans_PLI(DisasContext *s, arg_PLI *a)
     return ENABLE_ARCH_7;
 }
 
+static bool prefetch_check_m(DisasContext *s, int rm)
+{
+    switch (rm) {
+    case 13:
+        /* SP allowed in v8 or with A1 encoding; rejected with T1. */
+        return ENABLE_ARCH_8 || !s->thumb;
+    case 15:
+        /* PC always rejected. */
+        return false;
+    default:
+        return true;
+    }
+}
+
+static bool trans_PLD_rr(DisasContext *s, arg_PLD_rr *a)
+{
+    if (!ENABLE_ARCH_5TE) {
+        return false;
+    }
+    /* We cannot return false, because that leads to LDRB for thumb. */
+    if (!prefetch_check_m(s, a->rm)) {
+        unallocated_encoding(s);
+    }
+    return true;
+}
+
+static bool trans_PLDW_rr(DisasContext *s, arg_PLDW_rr *a)
+{
+    if (!arm_dc_feature(s, ARM_FEATURE_V7MP)) {
+        return false;
+    }
+    /*
+     * For A1, rn == 15 is UNPREDICTABLE.
+     * For T1, rn == 15 is PLD (literal).
+     */
+    if (a->rn == 15) {
+        return false;
+    }
+    /* We cannot return false, because that leads to LDRH for thumb. */
+    if (!prefetch_check_m(s, a->rm)) {
+        unallocated_encoding(s);
+    }
+    return true;
+}
+
+static bool trans_PLI_rr(DisasContext *s, arg_PLI_rr *a)
+{
+    if (!ENABLE_ARCH_7) {
+        return false;
+    }
+    /* We cannot return false, because that leads to LDRSB for thumb. */
+    if (!prefetch_check_m(s, a->rm)) {
+        unallocated_encoding(s);
+    }
+    return true;
+}
+
 /*
  * If-then
  */
-- 
2.34.1

On Sat, 25 May 2024 at 00:25, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> For all, rm == 15 is invalid.
> Prior to v8, thumb with rm == 13 is invalid.
> For PLDW, rn == 15 is invalid.

> Fixes a RISU mismatch for the HINTSPACE pattern in t32.risu
> compared to a neoverse-n1 host.

These are UNPREDICTABLE cases, not invalid. In general
we don't try to match a specific implementation's
UNPREDICTABLE choices.

I think we're better off avoiding the mismatch by improving
the risu patterns to avoid the UNPREDICTABLE cases.

thanks
-- PMM

On 5/28/24 06:18, Peter Maydell wrote:
> On Sat, 25 May 2024 at 00:25, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> For all, rm == 15 is invalid.
>> Prior to v8, thumb with rm == 13 is invalid.
>> For PLDW, rn == 15 is invalid.
> 
>> Fixes a RISU mismatch for the HINTSPACE pattern in t32.risu
>> compared to a neoverse-n1 host.
> 
> These are UNPREDICTABLE cases, not invalid. In general
> we don't try to match a specific implementation's
> UNPREDICTABLE choices.
> 
> I think we're better off avoiding the mismatch by improving
> the risu patterns to avoid the UNPREDICTABLE cases.

We do plenty of other treatments of UNPREDICTABLE as UNDEF (e.g. STREX).  Why is this case 
any different?


r~

On Tue, 28 May 2024 at 18:36, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/28/24 06:18, Peter Maydell wrote:
> > On Sat, 25 May 2024 at 00:25, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> For all, rm == 15 is invalid.
> >> Prior to v8, thumb with rm == 13 is invalid.
> >> For PLDW, rn == 15 is invalid.
> >
> >> Fixes a RISU mismatch for the HINTSPACE pattern in t32.risu
> >> compared to a neoverse-n1 host.
> >
> > These are UNPREDICTABLE cases, not invalid. In general
> > we don't try to match a specific implementation's
> > UNPREDICTABLE choices.
> >
> > I think we're better off avoiding the mismatch by improving
> > the risu patterns to avoid the UNPREDICTABLE cases.
>
> We do plenty of other treatments of UNPREDICTABLE as UNDEF (e.g. STREX).  Why is this case
> any different?

It just seems like a lot of effort to go to. Sometimes we
UNDEF for UNPREDICTABLEs, but quite often we say "the
behaviour we get for free is fine, so no need to write
extra code".

In this particular case, also:
 * we'd need to go back and cross-check against older
   architecture manuals and also look at whether M-profile
   and A-profile are doing the same thing here
 * the "v8 loosens the UNPREDICTABLE case for m = 13 T1
   encoding" looks suspiciously to me like a "nobody ever
   actually made this do anything except behave like the A1
   encoding, so make T1 and A1 the same" kind of relaxation

Basically, it would take me probably 15+ minutes to review
the changes against the various versions of the docs and
I don't think it's worth the effort :-)

thanks
-- PMM

On 5/29/24 06:32, Peter Maydell wrote:
>> We do plenty of other treatments of UNPREDICTABLE as UNDEF (e.g. STREX).  Why is this case
>> any different?
> 
> It just seems like a lot of effort to go to. Sometimes we
> UNDEF for UNPREDICTABLEs, but quite often we say "the
> behaviour we get for free is fine, so no need to write
> extra code".
> 
> In this particular case, also:
>   * we'd need to go back and cross-check against older
>     architecture manuals and also look at whether M-profile
>     and A-profile are doing the same thing here
>   * the "v8 loosens the UNPREDICTABLE case for m = 13 T1
>     encoding" looks suspiciously to me like a "nobody ever
>     actually made this do anything except behave like the A1
>     encoding, so make T1 and A1 the same" kind of relaxation
> 
> Basically, it would take me probably 15+ minutes to review
> the changes against the various versions of the docs and
> I don't think it's worth the effort :-)

Fair enough.  I've sent a patch to update thumb.risu to avoid the problematic patterns.


r~