[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV |
Date: |
Wed, 8 May 2013 18:50:07 +0200 |
User-agent: |
Mutt/1.5.20 (2009-06-14) |
On Wed, May 08, 2013 at 01:17:40PM +0200, Petar Jovanovic wrote:
> From: Petar Jovanovic <address@hidden>
>
> 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 <address@hidden>
> ---
> 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
address@hidden http://www.aurel32.net