Re: [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV

[Aurelien Jarno <aurelien@aurel32.net>]

On Wed, May 08, 2013 at 01:17:40PM +0200, Petar Jovanovic wrote:
> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
> 
> Corner case for INSV instruction when size=32 has not been correctly
> implemented. The mask for size should be one bit wider, and preparing the
> filter variable should be aware of this case too.
> 
> The test for INSV has been extended to include the case that triggers the
> bug.
> 
> Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
> ---
>  target-mips/dsp_helper.c         |    4 ++--
>  tests/tcg/mips/mips32-dsp/insv.c |   13 +++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)

Thanks for the patch. I have applied it, as it is correct and we are in
the freeze period. However please find a few comments below that might
need a follow-up patch.

> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index 805247d..9212789 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -2921,7 +2921,7 @@ target_ulong helper_##name(CPUMIPSState *env, target_ulong rs,  \
>          return rt;                                              \
>      }                                                           \
>                                                                  \
> -    filter = ((int32_t)0x01 << size) - 1;                       \
> +    filter = ((int64_t)0x01 << size) - 1;                       \

maybe using (1LL << size) would make the code easier to read.

>      filter = filter << pos;                                     \
>      temprs = (rs << pos) & filter;                              \
>      temprt = rt & ~filter;                                      \
> @@ -2930,7 +2930,7 @@ target_ulong helper_##name(CPUMIPSState *env, target_ulong rs,  \
>      return (target_long)(ret_type)temp;                         \
>  }
>  
> -BIT_INSV(insv, 0x1F, 0x1F, int32_t);
> +BIT_INSV(insv, 0x1F, 0x3F, int32_t);
>  #ifdef TARGET_MIPS64
>  BIT_INSV(dinsv, 0x7F, 0x3F, target_long);

This means that the sizefilter argument is constant, so it probably can
be removed.

>  #endif
> diff --git a/tests/tcg/mips/mips32-dsp/insv.c b/tests/tcg/mips/mips32-dsp/insv.c
> index 243b007..9d67469 100644
> --- a/tests/tcg/mips/mips32-dsp/insv.c
> +++ b/tests/tcg/mips/mips32-dsp/insv.c
> @@ -19,5 +19,18 @@ int main()
>          );
>      assert(rt == result);
>  
> +    dsp    = 0x1000;
> +    rt     = 0xF0F0F0F0;
> +    rs     = 0xA5A5A5A5;
> +    result = 0xA5A5A5A5;
> +
> +    __asm
> +        ("wrdsp %2\n\t"
> +         "insv  %0, %1\n\t"
> +         : "+r"(rt)
> +         : "r"(rs), "r"(dsp)
> +        );
> +    assert(rt == result);
> +
>      return 0;
>  }
> -- 
> 1.7.9.5
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net