[PATCH for-4.2 0/2] target/arm: two fixes for ldrex/strex

[PATCH for-4.2 0/2] target/arm: two fixes for ldrex/strex

[Richard Henderson]

During this cycle I added checks for UNPREDICTABLE behavior,
but didn't quite get it all right.


r~


Richard Henderson (2):
  target/arm: Do not reject rt == rt2 for strexd
  target/arm: Relax r13 restriction for ldrex/strex for v8.0

 target/arm/translate.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH 1/2] target/arm: Do not reject rt == rt2 for strexd

[Richard Henderson]

There was too much cut and paste between ldrexd and strexd,
as ldrexd does prohibit two output registers the same.

Fixes: af288228995
Reported-by: Michael Goffioul <michael.goffioul@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 2ea9da7637..b285b23858 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8934,7 +8934,7 @@ static bool op_strex(DisasContext *s, arg_STREX *a, MemOp mop, bool rel)
         || (s->thumb && (a->rd == 13 || a->rt == 13))
         || (mop == MO_64
             && (a->rt2 == 15
-                || a->rd == a->rt2 || a->rt == a->rt2
+                || a->rd == a->rt2
                 || (s->thumb && a->rt2 == 13)))) {
         unallocated_encoding(s);
         return true;
-- 
2.17.1

[PATCH 2/2] target/arm: Relax r13 restriction for ldrex/strex for v8.0

[Richard Henderson]

Armv8-A removes UNPREDICTABLE for R13 for these cases.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index b285b23858..3db8103966 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8931,11 +8931,11 @@ static bool op_strex(DisasContext *s, arg_STREX *a, MemOp mop, bool rel)
     /* We UNDEF for these UNPREDICTABLE cases.  */
     if (a->rd == 15 || a->rn == 15 || a->rt == 15
         || a->rd == a->rn || a->rd == a->rt
-        || (s->thumb && (a->rd == 13 || a->rt == 13))
+        || (!ENABLE_ARCH_8 && s->thumb && (a->rd == 13 || a->rt == 13))
         || (mop == MO_64
             && (a->rt2 == 15
                 || a->rd == a->rt2
-                || (s->thumb && a->rt2 == 13)))) {
+                || (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13)))) {
         unallocated_encoding(s);
         return true;
     }
@@ -9087,10 +9087,10 @@ static bool op_ldrex(DisasContext *s, arg_LDREX *a, MemOp mop, bool acq)
 
     /* We UNDEF for these UNPREDICTABLE cases.  */
     if (a->rn == 15 || a->rt == 15
-        || (s->thumb && a->rt == 13)
+        || (!ENABLE_ARCH_8 && s->thumb && a->rt == 13)
         || (mop == MO_64
             && (a->rt2 == 15 || a->rt == a->rt2
-                || (s->thumb && a->rt2 == 13)))) {
+                || (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13)))) {
         unallocated_encoding(s);
         return true;
     }
-- 
2.17.1

Re: [PATCH 2/2] target/arm: Relax r13 restriction for ldrex/strex for v8.0

[Peter Maydell]

On Sun, 17 Nov 2019 at 09:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Armv8-A removes UNPREDICTABLE for R13 for these cases.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index b285b23858..3db8103966 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8931,11 +8931,11 @@ static bool op_strex(DisasContext *s, arg_STREX *a, MemOp mop, bool rel)
>      /* We UNDEF for these UNPREDICTABLE cases.  */
>      if (a->rd == 15 || a->rn == 15 || a->rt == 15
>          || a->rd == a->rn || a->rd == a->rt
> -        || (s->thumb && (a->rd == 13 || a->rt == 13))
> +        || (!ENABLE_ARCH_8 && s->thumb && (a->rd == 13 || a->rt == 13))
>          || (mop == MO_64
>              && (a->rt2 == 15
>                  || a->rd == a->rt2
> -                || (s->thumb && a->rt2 == 13)))) {
> +                || (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13)))) {
>          unallocated_encoding(s);
>          return true;
>      }
> @@ -9087,10 +9087,10 @@ static bool op_ldrex(DisasContext *s, arg_LDREX *a, MemOp mop, bool acq)
>
>      /* We UNDEF for these UNPREDICTABLE cases.  */
>      if (a->rn == 15 || a->rt == 15
> -        || (s->thumb && a->rt == 13)
> +        || (!ENABLE_ARCH_8 && s->thumb && a->rt == 13)
>          || (mop == MO_64
>              && (a->rt2 == 15 || a->rt == a->rt2
> -                || (s->thumb && a->rt2 == 13)))) {
> +                || (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13)))) {
>          unallocated_encoding(s);
>          return true;
>      }

These cases for r13 are indeed no longer UNPREDICTABLE in
v8A, but they are still marked as UNPREDICTABLE for v8M...

thanks
-- PMM

Re: [PATCH 2/2] target/arm: Relax r13 restriction for ldrex/strex for v8.0

[Richard Henderson]

On 11/18/19 2:10 PM, Peter Maydell wrote:
>>      /* We UNDEF for these UNPREDICTABLE cases.  */
>>      if (a->rn == 15 || a->rt == 15
>> -        || (s->thumb && a->rt == 13)
>> +        || (!ENABLE_ARCH_8 && s->thumb && a->rt == 13)
>>          || (mop == MO_64
>>              && (a->rt2 == 15 || a->rt == a->rt2
>> -                || (s->thumb && a->rt2 == 13)))) {
>> +                || (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13)))) {
>>          unallocated_encoding(s);
>>          return true;
>>      }
> 
> These cases for r13 are indeed no longer UNPREDICTABLE in
> v8A, but they are still marked as UNPREDICTABLE for v8M...

Ho hum.  I knew I should have looked at that doc as well...


r~

Re: [PATCH 1/2] target/arm: Do not reject rt == rt2 for strexd

[Peter Maydell]

On Sun, 17 Nov 2019 at 09:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> There was too much cut and paste between ldrexd and strexd,
> as ldrexd does prohibit two output registers the same.
>
> Fixes: af288228995
> Reported-by: Michael Goffioul <michael.goffioul@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 2ea9da7637..b285b23858 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8934,7 +8934,7 @@ static bool op_strex(DisasContext *s, arg_STREX *a, MemOp mop, bool rel)
>          || (s->thumb && (a->rd == 13 || a->rt == 13))
>          || (mop == MO_64
>              && (a->rt2 == 15
> -                || a->rd == a->rt2 || a->rt == a->rt2
> +                || a->rd == a->rt2
>                  || (s->thumb && a->rt2 == 13)))) {
>          unallocated_encoding(s);
>          return true;
> --
> 2.17.1


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

thanks
-- PMM

Re: [PATCH 2/2] target/arm: Relax r13 restriction for ldrex/strex for v8.0

[Peter Maydell]

On Mon, 18 Nov 2019 at 13:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/18/19 2:10 PM, Peter Maydell wrote:
> >>      /* We UNDEF for these UNPREDICTABLE cases.  */
> >>      if (a->rn == 15 || a->rt == 15
> >> -        || (s->thumb && a->rt == 13)
> >> +        || (!ENABLE_ARCH_8 && s->thumb && a->rt == 13)
> >>          || (mop == MO_64
> >>              && (a->rt2 == 15 || a->rt == a->rt2
> >> -                || (s->thumb && a->rt2 == 13)))) {
> >> +                || (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13)))) {
> >>          unallocated_encoding(s);
> >>          return true;
> >>      }
> >
> > These cases for r13 are indeed no longer UNPREDICTABLE in
> > v8A, but they are still marked as UNPREDICTABLE for v8M...
>
> Ho hum.  I knew I should have looked at that doc as well...

I would like to get this in for rc2 tomorrow, so I propose
to squash in changes to give the following result (basically
turning the ENABLE_ARCH_8 checks into checks on a new bool 'v8a'):


diff --git a/target/arm/translate.c b/target/arm/translate.c
index b285b23858e..4d5d4bd8886 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8927,15 +8927,17 @@ static bool trans_SWPB(DisasContext *s, arg_SWP *a)
 static bool op_strex(DisasContext *s, arg_STREX *a, MemOp mop, bool rel)
 {
     TCGv_i32 addr;
+    /* Some cases stopped being UNPREDICTABLE in v8A (but not v8M) */
+    bool v8a = ENABLE_ARCH_8 && !arm_dc_feature(s, ARM_FEATURE_M);

     /* We UNDEF for these UNPREDICTABLE cases.  */
     if (a->rd == 15 || a->rn == 15 || a->rt == 15
         || a->rd == a->rn || a->rd == a->rt
-        || (s->thumb && (a->rd == 13 || a->rt == 13))
+        || (!v8a && s->thumb && (a->rd == 13 || a->rt == 13))
         || (mop == MO_64
             && (a->rt2 == 15
                 || a->rd == a->rt2
-                || (s->thumb && a->rt2 == 13)))) {
+                || (!v8a && s->thumb && a->rt2 == 13)))) {
         unallocated_encoding(s);
         return true;
     }
@@ -9084,13 +9086,15 @@ static bool trans_STLH(DisasContext *s, arg_STL *a)
 static bool op_ldrex(DisasContext *s, arg_LDREX *a, MemOp mop, bool acq)
 {
     TCGv_i32 addr;
+    /* Some cases stopped being UNPREDICTABLE in v8A (but not v8M) */
+    bool v8a = ENABLE_ARCH_8 && !arm_dc_feature(s, ARM_FEATURE_M);

     /* We UNDEF for these UNPREDICTABLE cases.  */
     if (a->rn == 15 || a->rt == 15
-        || (s->thumb && a->rt == 13)
+        || (!v8a && s->thumb && a->rt == 13)
         || (mop == MO_64
             && (a->rt2 == 15 || a->rt == a->rt2
-                || (s->thumb && a->rt2 == 13)))) {
+                || (!v8a && s->thumb && a->rt2 == 13)))) {
         unallocated_encoding(s);
         return true;
     }

thanks
-- PMM

Re: [PATCH 2/2] target/arm: Relax r13 restriction for ldrex/strex for v8.0

[Richard Henderson]

On 11/18/19 6:53 PM, Peter Maydell wrote:
> On Mon, 18 Nov 2019 at 13:16, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 11/18/19 2:10 PM, Peter Maydell wrote:
>>>>      /* We UNDEF for these UNPREDICTABLE cases.  */
>>>>      if (a->rn == 15 || a->rt == 15
>>>> -        || (s->thumb && a->rt == 13)
>>>> +        || (!ENABLE_ARCH_8 && s->thumb && a->rt == 13)
>>>>          || (mop == MO_64
>>>>              && (a->rt2 == 15 || a->rt == a->rt2
>>>> -                || (s->thumb && a->rt2 == 13)))) {
>>>> +                || (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13)))) {
>>>>          unallocated_encoding(s);
>>>>          return true;
>>>>      }
>>>
>>> These cases for r13 are indeed no longer UNPREDICTABLE in
>>> v8A, but they are still marked as UNPREDICTABLE for v8M...
>>
>> Ho hum.  I knew I should have looked at that doc as well...
> 
> I would like to get this in for rc2 tomorrow, so I propose
> to squash in changes to give the following result (basically
> turning the ENABLE_ARCH_8 checks into checks on a new bool 'v8a'):
> 
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index b285b23858e..4d5d4bd8886 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8927,15 +8927,17 @@ static bool trans_SWPB(DisasContext *s, arg_SWP *a)
>  static bool op_strex(DisasContext *s, arg_STREX *a, MemOp mop, bool rel)
>  {
>      TCGv_i32 addr;
> +    /* Some cases stopped being UNPREDICTABLE in v8A (but not v8M) */
> +    bool v8a = ENABLE_ARCH_8 && !arm_dc_feature(s, ARM_FEATURE_M);

Sorry, I wrote the patch but got distracted with other bugs without getting
around to posting.  I had solved this with a new ENABLE_ARCH_8A, but this
version works for me as well.

Thanks,

r~

Re: [PATCH 2/2] target/arm: Relax r13 restriction for ldrex/strex for v8.0

[Peter Maydell]

On Mon, 18 Nov 2019 at 20:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/18/19 6:53 PM, Peter Maydell wrote:
> > On Mon, 18 Nov 2019 at 13:16, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> On 11/18/19 2:10 PM, Peter Maydell wrote:
> >>>>      /* We UNDEF for these UNPREDICTABLE cases.  */
> >>>>      if (a->rn == 15 || a->rt == 15
> >>>> -        || (s->thumb && a->rt == 13)
> >>>> +        || (!ENABLE_ARCH_8 && s->thumb && a->rt == 13)
> >>>>          || (mop == MO_64
> >>>>              && (a->rt2 == 15 || a->rt == a->rt2
> >>>> -                || (s->thumb && a->rt2 == 13)))) {
> >>>> +                || (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13)))) {
> >>>>          unallocated_encoding(s);
> >>>>          return true;
> >>>>      }
> >>>
> >>> These cases for r13 are indeed no longer UNPREDICTABLE in
> >>> v8A, but they are still marked as UNPREDICTABLE for v8M...
> >>
> >> Ho hum.  I knew I should have looked at that doc as well...
> >
> > I would like to get this in for rc2 tomorrow, so I propose
> > to squash in changes to give the following result (basically
> > turning the ENABLE_ARCH_8 checks into checks on a new bool 'v8a'):
> >
> >
> > diff --git a/target/arm/translate.c b/target/arm/translate.c
> > index b285b23858e..4d5d4bd8886 100644
> > --- a/target/arm/translate.c
> > +++ b/target/arm/translate.c
> > @@ -8927,15 +8927,17 @@ static bool trans_SWPB(DisasContext *s, arg_SWP *a)
> >  static bool op_strex(DisasContext *s, arg_STREX *a, MemOp mop, bool rel)
> >  {
> >      TCGv_i32 addr;
> > +    /* Some cases stopped being UNPREDICTABLE in v8A (but not v8M) */
> > +    bool v8a = ENABLE_ARCH_8 && !arm_dc_feature(s, ARM_FEATURE_M);
>
> Sorry, I wrote the patch but got distracted with other bugs without getting
> around to posting.  I had solved this with a new ENABLE_ARCH_8A, but this
> version works for me as well.

At some point we should decide whether we prefer these ENABLE
macros or just to open-code arm_dc_feature() calls, because the
current mix is a bit odd... (for code I've written I've tended to the
arm_dc_feature() approach).

thanks
-- PMM