[PATCH 0/2] target/arm: Check Neon VLD1/VST1 stride bits are zero

[PATCH 0/2] target/arm: Check Neon VLD1/VST1 stride bits are zero

[Peter Maydell]

In the Neon VLD*/VST* "load/store single N-element structure to/from
one lane" instructions the encodings include bits to specify a
"stride" value, which specifies the separation between the Neon
registers which hold the different elements of the structure.  For
VLD1/VST1 there is only a single element and thus only one Neon
register is involved.  This means "stride" is not meaningful, and the
architecture requires that the bits that would encode it must be zero
(which is to say, must encode a stride value of 1).  We weren't
making this encoding check, so would incorrectly treat some
instruction patterns as being a VLD1/VST1 when they should UNDEF. 
(https://gitlab.com/qemu-project/qemu/-/issues/890)

Patch 1 fixes that bug.  Patch 2 is a minor cleanup of the align bits
check for VLD3/VST3 -- we had this logically correct (all the align
bits must be zero) but wrote it in a confusing way.

Richard: I tested this against your simple test case in the bug
report; if you could run it through your risu tests as well that
would be great.

thanks
-- PMM

Peter Maydell (2):
  target/arm/translate-neon: UNDEF if VLD1/VST1 stride bits are non-zero
  target/arm/translate-neon: Simplify align field check for VLD3

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

-- 
2.25.1

[PATCH 1/2] target/arm/translate-neon: UNDEF if VLD1/VST1 stride bits are non-zero

[Peter Maydell]

For VLD1/VST1 (single element to one lane) we are only accessing one
register, and so the 'stride' is meaningless.  The bits that would
specify stride (insn bit [4] for size=1, bit [6] for size=2) are
specified to be zero in the encoding (which would correspond to a
stride of 1 for VLD2/VLD3/VLD4 etc), and we must UNDEF if they are
not.

We failed to make this check, which meant that we would incorrectly
handle some instruction patterns as loads or stores instead of
UNDEFing them. Enforce that stride == 1 for the nregs == 1 case.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/890
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-neon.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/translate-neon.c b/target/arm/translate-neon.c
index 3854dd35163..072fdc1e6ee 100644
--- a/target/arm/translate-neon.c
+++ b/target/arm/translate-neon.c
@@ -657,6 +657,9 @@ static bool trans_VLDST_single(DisasContext *s, arg_VLDST_single *a)
     /* Catch the UNDEF cases. This is unavoidably a bit messy. */
     switch (nregs) {
     case 1:
+        if (a->stride != 1) {
+            return false;
+        }
         if (((a->align & (1 << a->size)) != 0) ||
             (a->size == 2 && (a->align == 1 || a->align == 2))) {
             return false;
-- 
2.25.1

Re: [PATCH 1/2] target/arm/translate-neon: UNDEF if VLD1/VST1 stride bits are non-zero

[Peter Maydell]

On Thu, 3 Mar 2022 at 11:37, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> For VLD1/VST1 (single element to one lane) we are only accessing one
> register, and so the 'stride' is meaningless.  The bits that would
> specify stride (insn bit [4] for size=1, bit [6] for size=2) are

This should say "bit [5] for size=1".

> specified to be zero in the encoding (which would correspond to a
> stride of 1 for VLD2/VLD3/VLD4 etc), and we must UNDEF if they are
> not.

-- PMM

Re: [PATCH 1/2] target/arm/translate-neon: UNDEF if VLD1/VST1 stride bits are non-zero

[Richard Henderson]

On 3/3/22 01:37, Peter Maydell wrote:
> For VLD1/VST1 (single element to one lane) we are only accessing one
> register, and so the 'stride' is meaningless.  The bits that would
> specify stride (insn bit [4] for size=1, bit [6] for size=2) are
> specified to be zero in the encoding (which would correspond to a
> stride of 1 for VLD2/VLD3/VLD4 etc), and we must UNDEF if they are
> not.
> 
> We failed to make this check, which meant that we would incorrectly
> handle some instruction patterns as loads or stores instead of
> UNDEFing them. Enforce that stride == 1 for the nregs == 1 case.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/890
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/translate-neon.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/target/arm/translate-neon.c b/target/arm/translate-neon.c
> index 3854dd35163..072fdc1e6ee 100644
> --- a/target/arm/translate-neon.c
> +++ b/target/arm/translate-neon.c
> @@ -657,6 +657,9 @@ static bool trans_VLDST_single(DisasContext *s, arg_VLDST_single *a)
>       /* Catch the UNDEF cases. This is unavoidably a bit messy. */
>       switch (nregs) {
>       case 1:
> +        if (a->stride != 1) {
> +            return false;
> +        }
>           if (((a->align & (1 << a->size)) != 0) ||
>               (a->size == 2 && (a->align == 1 || a->align == 2))) {
>               return false;

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 2/2] target/arm/translate-neon: Simplify align field check for VLD3

[Peter Maydell]

For VLD3 (single 3-element structure to one lane), there is no
alignment specification and the alignment bits in the instruction
must be zero.  This is bit [4] for the size=0 and size=1 cases, and
bits [5:4] for the size=2 case.  We do this check correctly in
VLDST_single(), but we write it a bit oddly: in the 'case 3' code we
check for bit 0 of a->align (bit [4] of the insn), and then we fall
through to the 'case 2' code which checks bit 1 of a->align (bit [5]
of the insn) in the size 2 case.  Replace this with just checking "is
a->align non-zero" for VLD3, which lets us drop the fall-through and
put the cases in this switch in numerical order.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-neon.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/arm/translate-neon.c b/target/arm/translate-neon.c
index 072fdc1e6ee..384604c0095 100644
--- a/target/arm/translate-neon.c
+++ b/target/arm/translate-neon.c
@@ -665,16 +665,16 @@ static bool trans_VLDST_single(DisasContext *s, arg_VLDST_single *a)
             return false;
         }
         break;
-    case 3:
-        if ((a->align & 1) != 0) {
-            return false;
-        }
-        /* fall through */
     case 2:
         if (a->size == 2 && (a->align & 2) != 0) {
             return false;
         }
         break;
+    case 3:
+        if (a->align != 0) {
+            return false;
+        }
+        break;
     case 4:
         if (a->size == 2 && a->align == 3) {
             return false;
-- 
2.25.1

Re: [PATCH 2/2] target/arm/translate-neon: Simplify align field check for VLD3

[Richard Henderson]

On 3/3/22 01:37, Peter Maydell wrote:
> For VLD3 (single 3-element structure to one lane), there is no
> alignment specification and the alignment bits in the instruction
> must be zero.  This is bit [4] for the size=0 and size=1 cases, and
> bits [5:4] for the size=2 case.  We do this check correctly in
> VLDST_single(), but we write it a bit oddly: in the 'case 3' code we
> check for bit 0 of a->align (bit [4] of the insn), and then we fall
> through to the 'case 2' code which checks bit 1 of a->align (bit [5]
> of the insn) in the size 2 case.  Replace this with just checking "is
> a->align non-zero" for VLD3, which lets us drop the fall-through and
> put the cases in this switch in numerical order.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/translate-neon.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Richard Henderson <richard.henderson@linaro.org>

r~