On Thu, 4 Apr 2024 at 17:26, Zack Buhman <zack@buhman.org> wrote:
The saturation arithmetic logic in helper_macl is not correct.
I tested and verified this behavior on a SH7091, the general pattern
is a code sequence such as:
sets
mov.l _mach,r2
lds r2,mach
mov.l _macl,r2
lds r2,macl
mova _n,r0
mov r0,r1
mova _m,r0
mac.l @r0+,@r1+
_mach: .long 0x00007fff
_macl: .long 0x12345678
_m: .long 0x7fffffff
_n: .long 0x7fffffff
Test case 0: (no int64_t overflow)
given; prior to saturation mac.l:
mach = 0x00007fff macl = 0x12345678
@r0 = 0x7fffffff @r1 = 0x7fffffff
expected saturation mac.l result:
mach = 0x00007fff macl = 0xffffffff
qemu saturation mac.l result (prior to this commit):
mach = 0x00007ffe macl = 0x12345678
Test case 1: (no int64_t overflow)
given; prior to saturation mac.l:
mach = 0xffff8000 macl = 0x00000000
@r0 = 0xffffffff @r1 = 0x00000001
expected saturation mac.l result:
mach = 0xffff8000 macl = 0x00000000
qemu saturation mac.l result (prior to this commit):
mach = 0xffff7fff macl = 0xffffffff
Test case 2: (int64_t addition overflow)
given; prior to saturation mac.l:
mach = 0x80000000 macl = 0x00000000
@r0 = 0xffffffff @r1 = 0x00000001
expected saturation mac.l result:
mach = 0xffff8000 macl = 0x00000000
qemu saturation mac.l result (prior to this commit):
mach = 0xffff7fff macl = 0xffffffff
Test case 3: (int64_t addition overflow)
given; prior to saturation mac.l:
mach = 0x7fffffff macl = 0x00000000
@r0 = 0x7fffffff @r1 = 0x7fffffff
expected saturation mac.l result:
mach = 0x00007fff macl = 0xffffffff
qemu saturation mac.l result (prior to this commit):
mach = 0xfffffffe macl = 0x00000001
All of the above also matches the description of MAC.L as documented
in cd00147165-sh-4-32-bit-cpu-core-architecture-stmicroelectronics.pdf
Hi. I just noticed that you didn't include a signed-off-by line
in your commit message. We need these as they're how you say
that you're legally OK to contribute this code to QEMU and
you're happy for it to go into the project:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-include-a-signed-off-by-line
has links to what exactly this means, but basically the
requirement is that the last line of your commit message should be
"Signed-off-by: Your Name <your@email>"
In this case, if you just reply to this email with that, we
can pick it up and fix up the commit message when we apply the
patch.
---
target/sh4/op_helper.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/target/sh4/op_helper.c b/target/sh4/op_helper.c
index 4559d0d376..ee16524083 100644
--- a/target/sh4/op_helper.c
+++ b/target/sh4/op_helper.c
@@ -160,18 +160,29 @@ void helper_ocbi(CPUSH4State *env, uint32_t address)
void helper_macl(CPUSH4State *env, uint32_t arg0, uint32_t arg1)
{
- int64_t res;
-
- res = ((uint64_t) env->mach << 32) | env->macl;
- res += (int64_t) (int32_t) arg0 *(int64_t) (int32_t) arg1;
- env->mach = (res >> 32) & 0xffffffff;
- env->macl = res & 0xffffffff;
+ int32_t value0 = (int32_t)arg0;
+ int32_t value1 = (int32_t)arg1;
+ int64_t mul = ((int64_t)value0) * ((int64_t)value1);
+ int64_t mac = (((uint64_t)env->mach) << 32) | env->macl;
+ int64_t result;
+ bool overflow = sadd64_overflow(mac, mul, &result);
+ /* Perform 48-bit saturation arithmetic if the S flag is set */
if (env->sr & (1u << SR_S)) {
- if (res < 0)
- env->mach |= 0xffff0000;
- else
- env->mach &= 0x00007fff;
+ /*
+ * The sign bit of `mac + mul` may overflow. The MAC unit on
+ * real SH-4 hardware has equivalent carry/saturation logic:
+ */
+ const int64_t upper_bound = ((1ull << 47) - 1);
+ const int64_t lower_bound = -((1ull << 47) - 0);
+
+ if (overflow) {
+ result = (mac < 0) ? lower_bound : upper_bound;
+ } else {
+ result = MIN(MAX(result, lower_bound), upper_bound);
+ }
}
+ env->macl = result;
+ env->mach = result >> 32;
}
I haven't checked the sh4 docs but the change looks right, so
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM