[Qemu-devel] [PATCH 0/7] target/arm: Fix various underdecodings

[Qemu-devel] [PATCH 0/7] target/arm: Fix various underdecodings

[Peter Maydell]

This patchset fixes the various cases of underdecoded instructions
that Laurent spotted and sent a bug report for. (The exception
is "missing default in disas_data_proc_1src", which got fixed in
commit 18de2813c35e359621a.)

thanks
-- PMM

Peter Maydell (7):
  target/arm/translate-a64: Don't underdecode system instructions
  target/arm/translate-a64: Don't underdecode PRFM
  target/arm/translate-a64: Don't underdecode SIMD ld/st multiple
  target/arm/translate-a64: Don't underdecode SIMD ld/st single
  target/arm/translate-a64: Don't underdecode add/sub extended register
  target/arm/translate-a64: Don't underdecode FP insns
  target/arm/translate-a64: Don't underdecode SDOT and UDOT

 target/arm/translate-a64.c | 53 +++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 7 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/7] target/arm/translate-a64: Don't underdecode system instructions

[Peter Maydell]

The "system instructions" and "system register move" subcategories
of "branches, exception generating and system instructions" for A64
only apply if bits [23:22] are zero; other values are currently
unallocated. Correctly UNDEF these unallocated encodings.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a64.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 4d28a27c3bd..e6df303e321 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2144,7 +2144,11 @@ static void disas_b_exc_sys(DisasContext *s, uint32_t insn)
         break;
     case 0x6a: /* Exception generation / System */
         if (insn & (1 << 24)) {
-            disas_system(s, insn);
+            if (extract32(insn, 22, 2) == 0) {
+                disas_system(s, insn);
+            } else {
+                unallocated_encoding(s);
+            }
         } else {
             disas_exc(s, insn);
         }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/7] target/arm/translate-a64: Don't underdecode system instructions

[Laurent Desnogues]

On Fri, Jan 25, 2019 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The "system instructions" and "system register move" subcategories
> of "branches, exception generating and system instructions" for A64
> only apply if bits [23:22] are zero; other values are currently
> unallocated. Correctly UNDEF these unallocated encodings.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
>  target/arm/translate-a64.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 4d28a27c3bd..e6df303e321 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -2144,7 +2144,11 @@ static void disas_b_exc_sys(DisasContext *s, uint32_t insn)
>          break;
>      case 0x6a: /* Exception generation / System */
>          if (insn & (1 << 24)) {
> -            disas_system(s, insn);
> +            if (extract32(insn, 22, 2) == 0) {
> +                disas_system(s, insn);
> +            } else {
> +                unallocated_encoding(s);
> +            }
>          } else {
>              disas_exc(s, insn);
>          }
> --
> 2.20.1
>

[Qemu-devel] [PATCH 2/7] target/arm/translate-a64: Don't underdecode PRFM

[Peter Maydell]

The PRFM prefetch insn in the load/store with imm9 encodings
requires idx field 0b00; we were underdecoding this by
only checking !is_unpriv (which is equivalent to idx != 2).
Correctly UNDEF the unallocated encodings where idx == 0b01
and 0b11 as well as 0b10.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index e6df303e321..8e081758e03 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2803,7 +2803,7 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
     } else {
         if (size == 3 && opc == 2) {
             /* PRFM - prefetch */
-            if (is_unpriv) {
+            if (idx != 0) {
                 unallocated_encoding(s);
                 return;
             }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/7] target/arm/translate-a64: Don't underdecode PRFM

[Laurent Desnogues]

On Fri, Jan 25, 2019 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The PRFM prefetch insn in the load/store with imm9 encodings
> requires idx field 0b00; we were underdecoding this by
> only checking !is_unpriv (which is equivalent to idx != 2).
> Correctly UNDEF the unallocated encodings where idx == 0b01
> and 0b11 as well as 0b10.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
>  target/arm/translate-a64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index e6df303e321..8e081758e03 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -2803,7 +2803,7 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
>      } else {
>          if (size == 3 && opc == 2) {
>              /* PRFM - prefetch */
> -            if (is_unpriv) {
> +            if (idx != 0) {
>                  unallocated_encoding(s);
>                  return;
>              }
> --
> 2.20.1
>

[Qemu-devel] [PATCH 3/7] target/arm/translate-a64: Don't underdecode SIMD ld/st multiple

[Peter Maydell]

In the AdvSIMD load/store multiple structures encodings,
the non-post-indexed case should have zeroes in [20:16]
(which is the Rm field for the post-indexed case).
Correctly UNDEF the currently unallocated encodings which
have non-zeroes in those bits.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a64.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 8e081758e03..c1f0cad7691 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -3249,6 +3249,7 @@ static void disas_ldst_multiple_struct(DisasContext *s, uint32_t insn)
 {
     int rt = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
+    int rm = extract32(insn, 16, 5);
     int size = extract32(insn, 10, 2);
     int opcode = extract32(insn, 12, 4);
     bool is_store = !extract32(insn, 22, 1);
@@ -3268,6 +3269,11 @@ static void disas_ldst_multiple_struct(DisasContext *s, uint32_t insn)
         return;
     }
 
+    if (!is_postidx && rm != 0) {
+        unallocated_encoding(s);
+        return;
+    }
+
     /* From the shared decode logic */
     switch (opcode) {
     case 0x0:
@@ -3367,7 +3373,6 @@ static void disas_ldst_multiple_struct(DisasContext *s, uint32_t insn)
     }
 
     if (is_postidx) {
-        int rm = extract32(insn, 16, 5);
         if (rm == 31) {
             tcg_gen_mov_i64(tcg_rn, tcg_addr);
         } else {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 3/7] target/arm/translate-a64: Don't underdecode SIMD ld/st multiple

[Laurent Desnogues]

On Fri, Jan 25, 2019 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the AdvSIMD load/store multiple structures encodings,
> the non-post-indexed case should have zeroes in [20:16]
> (which is the Rm field for the post-indexed case).
> Correctly UNDEF the currently unallocated encodings which
> have non-zeroes in those bits.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
>  target/arm/translate-a64.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 8e081758e03..c1f0cad7691 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -3249,6 +3249,7 @@ static void disas_ldst_multiple_struct(DisasContext *s, uint32_t insn)
>  {
>      int rt = extract32(insn, 0, 5);
>      int rn = extract32(insn, 5, 5);
> +    int rm = extract32(insn, 16, 5);
>      int size = extract32(insn, 10, 2);
>      int opcode = extract32(insn, 12, 4);
>      bool is_store = !extract32(insn, 22, 1);
> @@ -3268,6 +3269,11 @@ static void disas_ldst_multiple_struct(DisasContext *s, uint32_t insn)
>          return;
>      }
>
> +    if (!is_postidx && rm != 0) {
> +        unallocated_encoding(s);
> +        return;
> +    }
> +
>      /* From the shared decode logic */
>      switch (opcode) {
>      case 0x0:
> @@ -3367,7 +3373,6 @@ static void disas_ldst_multiple_struct(DisasContext *s, uint32_t insn)
>      }
>
>      if (is_postidx) {
> -        int rm = extract32(insn, 16, 5);
>          if (rm == 31) {
>              tcg_gen_mov_i64(tcg_rn, tcg_addr);
>          } else {
> --
> 2.20.1
>

[Qemu-devel] [PATCH 4/7] target/arm/translate-a64: Don't underdecode SIMD ld/st single

[Peter Maydell]

In the AdvSIMD load/store single structure encodings, the
non-post-indexed case should have zeroes in [20:16] (which is the
Rm field for the post-indexed case). Bit 31 must also be zero
(a check we got right in ldst_multiple but not here). Correctly
UNDEF these unallocated encodings.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a64.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index c1f0cad7691..2cade64ed25 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -3409,6 +3409,7 @@ static void disas_ldst_single_struct(DisasContext *s, uint32_t insn)
 {
     int rt = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
+    int rm = extract32(insn, 16, 5);
     int size = extract32(insn, 10, 2);
     int S = extract32(insn, 12, 1);
     int opc = extract32(insn, 13, 3);
@@ -3424,6 +3425,15 @@ static void disas_ldst_single_struct(DisasContext *s, uint32_t insn)
     int ebytes, xs;
     TCGv_i64 tcg_addr, tcg_rn, tcg_ebytes;
 
+    if (extract32(insn, 31, 1)) {
+        unallocated_encoding(s);
+        return;
+    }
+    if (!is_postidx && rm != 0) {
+        unallocated_encoding(s);
+        return;
+    }
+
     switch (scale) {
     case 3:
         if (!is_load || S) {
@@ -3501,7 +3511,6 @@ static void disas_ldst_single_struct(DisasContext *s, uint32_t insn)
     }
 
     if (is_postidx) {
-        int rm = extract32(insn, 16, 5);
         if (rm == 31) {
             tcg_gen_mov_i64(tcg_rn, tcg_addr);
         } else {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 4/7] target/arm/translate-a64: Don't underdecode SIMD ld/st single

[Laurent Desnogues]

On Fri, Jan 25, 2019 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the AdvSIMD load/store single structure encodings, the
> non-post-indexed case should have zeroes in [20:16] (which is the
> Rm field for the post-indexed case). Bit 31 must also be zero
> (a check we got right in ldst_multiple but not here). Correctly
> UNDEF these unallocated encodings.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
>  target/arm/translate-a64.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index c1f0cad7691..2cade64ed25 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -3409,6 +3409,7 @@ static void disas_ldst_single_struct(DisasContext *s, uint32_t insn)
>  {
>      int rt = extract32(insn, 0, 5);
>      int rn = extract32(insn, 5, 5);
> +    int rm = extract32(insn, 16, 5);
>      int size = extract32(insn, 10, 2);
>      int S = extract32(insn, 12, 1);
>      int opc = extract32(insn, 13, 3);
> @@ -3424,6 +3425,15 @@ static void disas_ldst_single_struct(DisasContext *s, uint32_t insn)
>      int ebytes, xs;
>      TCGv_i64 tcg_addr, tcg_rn, tcg_ebytes;
>
> +    if (extract32(insn, 31, 1)) {
> +        unallocated_encoding(s);
> +        return;
> +    }
> +    if (!is_postidx && rm != 0) {
> +        unallocated_encoding(s);
> +        return;
> +    }
> +
>      switch (scale) {
>      case 3:
>          if (!is_load || S) {
> @@ -3501,7 +3511,6 @@ static void disas_ldst_single_struct(DisasContext *s, uint32_t insn)
>      }
>
>      if (is_postidx) {
> -        int rm = extract32(insn, 16, 5);
>          if (rm == 31) {
>              tcg_gen_mov_i64(tcg_rn, tcg_addr);
>          } else {
> --
> 2.20.1
>

[Qemu-devel] [PATCH 5/7] target/arm/translate-a64: Don't underdecode add/sub extended register

[Peter Maydell]

In the "add/subtract (extended register)" encoding group, the "opt"
field in bits [23:22] must be zero. Correctly UNDEF the unallocated
encodings where this field is not zero.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a64.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 2cade64ed25..efd2f6490b5 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -4204,12 +4204,13 @@ static void disas_add_sub_ext_reg(DisasContext *s, uint32_t insn)
     bool setflags = extract32(insn, 29, 1);
     bool sub_op = extract32(insn, 30, 1);
     bool sf = extract32(insn, 31, 1);
+    bool opt = extract32(insn, 22, 2);
 
     TCGv_i64 tcg_rm, tcg_rn; /* temps */
     TCGv_i64 tcg_rd;
     TCGv_i64 tcg_result;
 
-    if (imm3 > 4) {
+    if (imm3 > 4 || opt != 0) {
         unallocated_encoding(s);
         return;
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 5/7] target/arm/translate-a64: Don't underdecode add/sub extended register

[Laurent Desnogues]

On Fri, Jan 25, 2019 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the "add/subtract (extended register)" encoding group, the "opt"
> field in bits [23:22] must be zero. Correctly UNDEF the unallocated
> encodings where this field is not zero.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate-a64.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 2cade64ed25..efd2f6490b5 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -4204,12 +4204,13 @@ static void disas_add_sub_ext_reg(DisasContext *s, uint32_t insn)
>      bool setflags = extract32(insn, 29, 1);
>      bool sub_op = extract32(insn, 30, 1);
>      bool sf = extract32(insn, 31, 1);
> +    bool opt = extract32(insn, 22, 2);

I'd prefer an int to a bool.

Otherwise:

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

>      TCGv_i64 tcg_rm, tcg_rn; /* temps */
>      TCGv_i64 tcg_rd;
>      TCGv_i64 tcg_result;
>
> -    if (imm3 > 4) {
> +    if (imm3 > 4 || opt != 0) {
>          unallocated_encoding(s);
>          return;
>      }
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH 5/7] target/arm/translate-a64: Don't underdecode add/sub extended register

[Peter Maydell]

On Mon, 28 Jan 2019 at 11:16, Laurent Desnogues
<laurent.desnogues@gmail.com> wrote:
>
> On Fri, Jan 25, 2019 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > In the "add/subtract (extended register)" encoding group, the "opt"
> > field in bits [23:22] must be zero. Correctly UNDEF the unallocated
> > encodings where this field is not zero.
> >
> > Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  target/arm/translate-a64.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> > index 2cade64ed25..efd2f6490b5 100644
> > --- a/target/arm/translate-a64.c
> > +++ b/target/arm/translate-a64.c
> > @@ -4204,12 +4204,13 @@ static void disas_add_sub_ext_reg(DisasContext *s, uint32_t insn)
> >      bool setflags = extract32(insn, 29, 1);
> >      bool sub_op = extract32(insn, 30, 1);
> >      bool sf = extract32(insn, 31, 1);
> > +    bool opt = extract32(insn, 22, 2);
>
> I'd prefer an int to a bool.

Oops, yes. I think I got mixed up with all the other bool
flags here, but it's a 2 bit field so definitely should be int.

thanks
-- PMM

[Qemu-devel] [PATCH 6/7] target/arm/translate-a64: Don't underdecode FP insns

[Peter Maydell]

In the encoding groups
 * floating-point data-processing (1 source)
 * floating-point data-processing (2 source)
 * floating-point data-processing (3 source)
 * floating-point immediate
 * floating-point compare
 * floating-ponit conditional compare
 * floating-point conditional select


bit 31 is M and bit 29 is S (and bit 30 is 0, already checked at
this point in the decode). None of these groups allocate any
encoding for M=1 or S=1. We checked this in disas_fp_compare(),
disas_fp_ccomp() and disas_fp_csel(), but missed it in disas_fp_1src(),
disas_fp_2src(), disas_fp_3src() and disas_fp_imm().

We also missed that in the fp immediate encoding the imm5 field
must be all zeroes.

Correctly UNDEF the unallocated encodings here.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a64.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index efd2f6490b5..474d9bfb5f0 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -5636,11 +5636,17 @@ static void handle_fp_fcvt(DisasContext *s, int opcode,
  */
 static void disas_fp_1src(DisasContext *s, uint32_t insn)
 {
+    int mos = extract32(insn, 29, 3);
     int type = extract32(insn, 22, 2);
     int opcode = extract32(insn, 15, 6);
     int rn = extract32(insn, 5, 5);
     int rd = extract32(insn, 0, 5);
 
+    if (mos) {
+        unallocated_encoding(s);
+        return;
+    }
+
     switch (opcode) {
     case 0x4: case 0x5: case 0x7:
     {
@@ -5867,13 +5873,14 @@ static void handle_fp_2src_half(DisasContext *s, int opcode,
  */
 static void disas_fp_2src(DisasContext *s, uint32_t insn)
 {
+    int mos = extract32(insn, 29, 3);
     int type = extract32(insn, 22, 2);
     int rd = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
     int rm = extract32(insn, 16, 5);
     int opcode = extract32(insn, 12, 4);
 
-    if (opcode > 8) {
+    if (opcode > 8 || mos) {
         unallocated_encoding(s);
         return;
     }
@@ -6028,6 +6035,7 @@ static void handle_fp_3src_half(DisasContext *s, bool o0, bool o1,
  */
 static void disas_fp_3src(DisasContext *s, uint32_t insn)
 {
+    int mos = extract32(insn, 29, 3);
     int type = extract32(insn, 22, 2);
     int rd = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
@@ -6036,6 +6044,11 @@ static void disas_fp_3src(DisasContext *s, uint32_t insn)
     bool o0 = extract32(insn, 15, 1);
     bool o1 = extract32(insn, 21, 1);
 
+    if (mos) {
+        unallocated_encoding(s);
+        return;
+    }
+
     switch (type) {
     case 0:
         if (!fp_access_check(s)) {
@@ -6105,12 +6118,19 @@ uint64_t vfp_expand_imm(int size, uint8_t imm8)
 static void disas_fp_imm(DisasContext *s, uint32_t insn)
 {
     int rd = extract32(insn, 0, 5);
+    int imm5 = extract32(insn, 5, 5);
     int imm8 = extract32(insn, 13, 8);
     int type = extract32(insn, 22, 2);
+    int mos = extract32(insn, 29, 3);
     uint64_t imm;
     TCGv_i64 tcg_res;
     TCGMemOp sz;
 
+    if (mos || imm5) {
+        unallocated_encoding(s);
+        return;
+    }
+
     switch (type) {
     case 0:
         sz = MO_32;
-- 
2.20.1

Re: [Qemu-devel] [PATCH 6/7] target/arm/translate-a64: Don't underdecode FP insns

[Laurent Desnogues]

On Fri, Jan 25, 2019 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the encoding groups
>  * floating-point data-processing (1 source)
>  * floating-point data-processing (2 source)
>  * floating-point data-processing (3 source)
>  * floating-point immediate
>  * floating-point compare
>  * floating-ponit conditional compare
>  * floating-point conditional select
>
>
> bit 31 is M and bit 29 is S (and bit 30 is 0, already checked at
> this point in the decode). None of these groups allocate any
> encoding for M=1 or S=1. We checked this in disas_fp_compare(),
> disas_fp_ccomp() and disas_fp_csel(), but missed it in disas_fp_1src(),
> disas_fp_2src(), disas_fp_3src() and disas_fp_imm().
>
> We also missed that in the fp immediate encoding the imm5 field
> must be all zeroes.
>
> Correctly UNDEF the unallocated encodings here.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
>  target/arm/translate-a64.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index efd2f6490b5..474d9bfb5f0 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -5636,11 +5636,17 @@ static void handle_fp_fcvt(DisasContext *s, int opcode,
>   */
>  static void disas_fp_1src(DisasContext *s, uint32_t insn)
>  {
> +    int mos = extract32(insn, 29, 3);
>      int type = extract32(insn, 22, 2);
>      int opcode = extract32(insn, 15, 6);
>      int rn = extract32(insn, 5, 5);
>      int rd = extract32(insn, 0, 5);
>
> +    if (mos) {
> +        unallocated_encoding(s);
> +        return;
> +    }
> +
>      switch (opcode) {
>      case 0x4: case 0x5: case 0x7:
>      {
> @@ -5867,13 +5873,14 @@ static void handle_fp_2src_half(DisasContext *s, int opcode,
>   */
>  static void disas_fp_2src(DisasContext *s, uint32_t insn)
>  {
> +    int mos = extract32(insn, 29, 3);
>      int type = extract32(insn, 22, 2);
>      int rd = extract32(insn, 0, 5);
>      int rn = extract32(insn, 5, 5);
>      int rm = extract32(insn, 16, 5);
>      int opcode = extract32(insn, 12, 4);
>
> -    if (opcode > 8) {
> +    if (opcode > 8 || mos) {
>          unallocated_encoding(s);
>          return;
>      }
> @@ -6028,6 +6035,7 @@ static void handle_fp_3src_half(DisasContext *s, bool o0, bool o1,
>   */
>  static void disas_fp_3src(DisasContext *s, uint32_t insn)
>  {
> +    int mos = extract32(insn, 29, 3);
>      int type = extract32(insn, 22, 2);
>      int rd = extract32(insn, 0, 5);
>      int rn = extract32(insn, 5, 5);
> @@ -6036,6 +6044,11 @@ static void disas_fp_3src(DisasContext *s, uint32_t insn)
>      bool o0 = extract32(insn, 15, 1);
>      bool o1 = extract32(insn, 21, 1);
>
> +    if (mos) {
> +        unallocated_encoding(s);
> +        return;
> +    }
> +
>      switch (type) {
>      case 0:
>          if (!fp_access_check(s)) {
> @@ -6105,12 +6118,19 @@ uint64_t vfp_expand_imm(int size, uint8_t imm8)
>  static void disas_fp_imm(DisasContext *s, uint32_t insn)
>  {
>      int rd = extract32(insn, 0, 5);
> +    int imm5 = extract32(insn, 5, 5);
>      int imm8 = extract32(insn, 13, 8);
>      int type = extract32(insn, 22, 2);
> +    int mos = extract32(insn, 29, 3);
>      uint64_t imm;
>      TCGv_i64 tcg_res;
>      TCGMemOp sz;
>
> +    if (mos || imm5) {
> +        unallocated_encoding(s);
> +        return;
> +    }
> +
>      switch (type) {
>      case 0:
>          sz = MO_32;
> --
> 2.20.1
>

[Qemu-devel] [PATCH 7/7] target/arm/translate-a64: Don't underdecode SDOT and UDOT

[Peter Maydell]

In the AdvSIMD scalar x indexed element and vector x indexed element
encoding group, the SDOT and UDOT instructions are vector only,
and their opcode is unallocated in the scalar group. Correctly
UNDEF this unallocated encoding.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 474d9bfb5f0..30bc2412fc0 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -12641,7 +12641,7 @@ static void disas_simd_indexed(DisasContext *s, uint32_t insn)
         break;
     case 0x0e: /* SDOT */
     case 0x1e: /* UDOT */
-        if (size != MO_32 || !dc_isar_feature(aa64_dp, s)) {
+        if (is_scalar || size != MO_32 || !dc_isar_feature(aa64_dp, s)) {
             unallocated_encoding(s);
             return;
         }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 7/7] target/arm/translate-a64: Don't underdecode SDOT and UDOT

[Laurent Desnogues]

On Fri, Jan 25, 2019 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the AdvSIMD scalar x indexed element and vector x indexed element
> encoding group, the SDOT and UDOT instructions are vector only,
> and their opcode is unallocated in the scalar group. Correctly
> UNDEF this unallocated encoding.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
>  target/arm/translate-a64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 474d9bfb5f0..30bc2412fc0 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -12641,7 +12641,7 @@ static void disas_simd_indexed(DisasContext *s, uint32_t insn)
>          break;
>      case 0x0e: /* SDOT */
>      case 0x1e: /* UDOT */
> -        if (size != MO_32 || !dc_isar_feature(aa64_dp, s)) {
> +        if (is_scalar || size != MO_32 || !dc_isar_feature(aa64_dp, s)) {
>              unallocated_encoding(s);
>              return;
>          }
> --
> 2.20.1
>