Hi Zack, Cc'ing the maintainer of this file, Yoshinori:
$ ./scripts/get_maintainer.pl -f target/sh4/op_helper.c Yoshinori Sato <[email protected]> (reviewer:SH4 TCG CPUs) (https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer) On 4/4/24 18:39, Peter Maydell wrote:
On Thu, 4 Apr 2024 at 17:26, Zack Buhman <[email protected]> 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.pdfHi. 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 <[email protected]> thanks -- PMM
