* [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV
@ 2013-05-08 11:17 Petar Jovanovic
2013-05-08 12:41 ` [Qemu-devel] " Michael Tokarev
2013-05-08 16:50 ` Aurelien Jarno
0 siblings, 2 replies; 4+ messages in thread
From: Petar Jovanovic @ 2013-05-08 11:17 UTC (permalink / raw)
To: qemu-trivial; +Cc: petar.jovanovic
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(-)
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; \
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);
#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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV
2013-05-08 11:17 [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV Petar Jovanovic
@ 2013-05-08 12:41 ` Michael Tokarev
2013-05-08 16:50 ` Aurelien Jarno
1 sibling, 0 replies; 4+ messages in thread
From: Michael Tokarev @ 2013-05-08 12:41 UTC (permalink / raw)
To: Petar Jovanovic; +Cc: qemu-trivial, petar.jovanovic, qemu-devel
08.05.2013 15:17, 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.
This isn't really a good fit for a trivial patch. Cc'ing qemu-devel
and mips "odd-fixes" maintainer instead.
Thanks!
/mjt
> 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(-)
>
> 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; \
> 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);
> #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;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV
@ 2013-05-08 12:41 ` Michael Tokarev
0 siblings, 0 replies; 4+ messages in thread
From: Michael Tokarev @ 2013-05-08 12:41 UTC (permalink / raw)
To: Petar Jovanovic; +Cc: qemu-trivial, petar.jovanovic, Aurelien Jarno, qemu-devel
08.05.2013 15:17, 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.
This isn't really a good fit for a trivial patch. Cc'ing qemu-devel
and mips "odd-fixes" maintainer instead.
Thanks!
/mjt
> 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(-)
>
> 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; \
> 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);
> #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;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV
2013-05-08 11:17 [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV Petar Jovanovic
2013-05-08 12:41 ` [Qemu-devel] " Michael Tokarev
@ 2013-05-08 16:50 ` Aurelien Jarno
1 sibling, 0 replies; 4+ messages in thread
From: Aurelien Jarno @ 2013-05-08 16:50 UTC (permalink / raw)
To: Petar Jovanovic; +Cc: qemu-trivial, petar.jovanovic
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-08 16:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-08 11:17 [Qemu-trivial] [PATCH] target-mips: fix incorrect behaviour for INSV Petar Jovanovic
2013-05-08 12:41 ` Michael Tokarev
2013-05-08 12:41 ` [Qemu-devel] " Michael Tokarev
2013-05-08 16:50 ` Aurelien Jarno
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.