Coverity reported (CID 1450831) an array overrun in
gen_mxu_D16MAX_D16MIN():

  1103     } else if (unlikely((XRb == 0) || (XRa == 0))) {
  ....
  1112         if (opc == OPC_MXU_D16MAX) {
  1113             tcg_gen_smax_i32(mxu_gpr[XRa - 1], t0, t1);
  1114         } else {
  1115             tcg_gen_smin_i32(mxu_gpr[XRa - 1], t0, t1);
  1116         }

>>> Overrunning array "mxu_gpr" of 15 8-byte elements at element
    index 4294967295 (byte offset 34359738367) using index "XRa - 1U"
    (which evaluates to 4294967295).

This happens because the code is confused about which of XRa, XRb and
XRc is the output, and which are the inputs.  XRa is the output, but
most of the conditions separating out different special cases are
written as if XRc is the output, with the result that we can end up
in the code path that assumes XRa is non-0 even when it is zero.

Fix the erroneous code, bringing it in to line with the structure
used in functions like gen_mxu_S32MAX_S32MIN() and
gen_mxu_Q8MAX_Q8MIN().

Fixes: CID 1450831
Fixes: bb84cbf38505bd1d8
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
NB: tested with 'make check' and 'make check-acceptance' only, which
almost certainly don't exercise this code path.

 target/mips/mxu_translate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/mips/mxu_translate.c b/target/mips/mxu_translate.c
index afc008eeeef..fb0a811af6c 100644
--- a/target/mips/mxu_translate.c
+++ b/target/mips/mxu_translate.c
@@ -1095,12 +1095,12 @@ static void gen_mxu_D16MAX_D16MIN(DisasContext *ctx)
 
     if (unlikely(pad != 0)) {
         /* opcode padding incorrect -> do nothing */
-    } else if (unlikely(XRc == 0)) {
+    } else if (unlikely(XRa == 0)) {
         /* destination is zero register -> do nothing */
-    } else if (unlikely((XRb == 0) && (XRa == 0))) {
+    } else if (unlikely((XRb == 0) && (XRc == 0))) {
         /* both operands zero registers -> just set destination to zero */
-        tcg_gen_movi_i32(mxu_gpr[XRc - 1], 0);
-    } else if (unlikely((XRb == 0) || (XRa == 0))) {
+        tcg_gen_movi_i32(mxu_gpr[XRa - 1], 0);
+    } else if (unlikely((XRb == 0) || (XRc == 0))) {
         /* exactly one operand is zero register - find which one is not...*/
         uint32_t XRx = XRb ? XRb : XRc;
         /* ...and do half-word-wise max/min with one operand 0 */
-- 
2.20.1

+Craig

On 3/16/21 2:13 PM, Peter Maydell wrote:
> Coverity reported (CID 1450831) an array overrun in
> gen_mxu_D16MAX_D16MIN():
> 
>   1103     } else if (unlikely((XRb == 0) || (XRa == 0))) {
>   ....
>   1112         if (opc == OPC_MXU_D16MAX) {
>   1113             tcg_gen_smax_i32(mxu_gpr[XRa - 1], t0, t1);
>   1114         } else {
>   1115             tcg_gen_smin_i32(mxu_gpr[XRa - 1], t0, t1);
>   1116         }
> 
>>>> Overrunning array "mxu_gpr" of 15 8-byte elements at element
>     index 4294967295 (byte offset 34359738367) using index "XRa - 1U"
>     (which evaluates to 4294967295).
> 
> This happens because the code is confused about which of XRa, XRb and
> XRc is the output, and which are the inputs.  XRa is the output, but
> most of the conditions separating out different special cases are
> written as if XRc is the output, with the result that we can end up
> in the code path that assumes XRa is non-0 even when it is zero.
> 
> Fix the erroneous code, bringing it in to line with the structure
> used in functions like gen_mxu_S32MAX_S32MIN() and
> gen_mxu_Q8MAX_Q8MIN().
> 
> Fixes: CID 1450831
> Fixes: bb84cbf38505bd1d8
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> NB: tested with 'make check' and 'make check-acceptance' only, which
> almost certainly don't exercise this code path.

Not it isn't indeed.

I'm adding testing where users show interest.

For Loongson SIMD I have 2 images, Fedora and Debian, and watch
a video stream with ffmpeg. I have in my TODO to add a tcg plugin
to check the instruction coverage. Maybe it already exists, I haven't
checked.
Why I'd love to get this integration testing in mainstream, I gave up
because of the artifact storage problem (it doesn't make sense to
rebuild a full OS to test a single package to test an ISA).

I don't have anything for XBurst. Craig was running some
testing 2 years ago, but I have no idea what it was.

>  target/mips/mxu_translate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/mips/mxu_translate.c b/target/mips/mxu_translate.c
> index afc008eeeef..fb0a811af6c 100644
> --- a/target/mips/mxu_translate.c
> +++ b/target/mips/mxu_translate.c
> @@ -1095,12 +1095,12 @@ static void gen_mxu_D16MAX_D16MIN(DisasContext *ctx)
>  
>      if (unlikely(pad != 0)) {
>          /* opcode padding incorrect -> do nothing */
> -    } else if (unlikely(XRc == 0)) {
> +    } else if (unlikely(XRa == 0)) {
>          /* destination is zero register -> do nothing */
> -    } else if (unlikely((XRb == 0) && (XRa == 0))) {
> +    } else if (unlikely((XRb == 0) && (XRc == 0))) {
>          /* both operands zero registers -> just set destination to zero */
> -        tcg_gen_movi_i32(mxu_gpr[XRc - 1], 0);
> -    } else if (unlikely((XRb == 0) || (XRa == 0))) {
> +        tcg_gen_movi_i32(mxu_gpr[XRa - 1], 0);
> +    } else if (unlikely((XRb == 0) || (XRc == 0))) {
>          /* exactly one operand is zero register - find which one is not...*/
>          uint32_t XRx = XRb ? XRb : XRc;
>          /* ...and do half-word-wise max/min with one operand 0 */
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

On 3/16/21 2:13 PM, Peter Maydell wrote:
> Coverity reported (CID 1450831) an array overrun in
> gen_mxu_D16MAX_D16MIN():
> 
>   1103     } else if (unlikely((XRb == 0) || (XRa == 0))) {
>   ....
>   1112         if (opc == OPC_MXU_D16MAX) {
>   1113             tcg_gen_smax_i32(mxu_gpr[XRa - 1], t0, t1);
>   1114         } else {
>   1115             tcg_gen_smin_i32(mxu_gpr[XRa - 1], t0, t1);
>   1116         }
> 
>>>> Overrunning array "mxu_gpr" of 15 8-byte elements at element
>     index 4294967295 (byte offset 34359738367) using index "XRa - 1U"
>     (which evaluates to 4294967295).
> 
> This happens because the code is confused about which of XRa, XRb and
> XRc is the output, and which are the inputs.  XRa is the output, but
> most of the conditions separating out different special cases are
> written as if XRc is the output, with the result that we can end up
> in the code path that assumes XRa is non-0 even when it is zero.
> 
> Fix the erroneous code, bringing it in to line with the structure
> used in functions like gen_mxu_S32MAX_S32MIN() and
> gen_mxu_Q8MAX_Q8MIN().
> 
> Fixes: CID 1450831
> Fixes: bb84cbf38505bd1d8
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> NB: tested with 'make check' and 'make check-acceptance' only, which
> almost certainly don't exercise this code path.
> 
>  target/mips/mxu_translate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Thanks, applied to mips-fixes.