| Message ID | b8d0b7c81437065b81416c00781e42ea09e2053c.1460625412.git.atar4qemu@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 14/04/16 10:29, Artyom Tarasenko wrote: > Fix register offset calculation when regwptr is used. > > Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > --- > target-sparc/ldst_helper.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c > index 2b0221c..a383074 100644 > --- a/target-sparc/ldst_helper.c > +++ b/target-sparc/ldst_helper.c > @@ -2059,11 +2059,11 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd) > bswap64s(&env->gregs[rd + 1]); > } > } else { > - env->regwptr[rd] = cpu_ldq_nucleus(env, addr); > - env->regwptr[rd + 1] = cpu_ldq_nucleus(env, addr + 8); > + env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr); > + env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8); > if (asi == 0x2c) { > - bswap64s(&env->regwptr[rd]); > - bswap64s(&env->regwptr[rd + 1]); > + bswap64s(&env->regwptr[rd - 8]); > + bswap64s(&env->regwptr[rd + 1 - 8]); > } > } > break; > @@ -2076,8 +2076,8 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd) > env->gregs[rd] = helper_ld_asi(env, addr, asi, 4, 0); > env->gregs[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0); > } else { > - env->regwptr[rd] = helper_ld_asi(env, addr, asi, 4, 0); > - env->regwptr[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0); > + env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0); > + env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4, asi, 4, 0); > } > break; > } > So it seems that without this patch any ldda on a non-global register is placing the result into the wrong register which is fairly nasty. The patch does apply, but only with fuzz, and checkpatch.pl reports several warnings with spacing/line endings. If you can rebase to master and resend then you can add my Reviewed-by and I'm okay with this for 2.6 - register corruption is not good. ATB, Mark.
On Thu, Apr 14, 2016 at 3:42 PM, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 14/04/16 10:29, Artyom Tarasenko wrote: > >> Fix register offset calculation when regwptr is used. >> >> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >> --- >> target-sparc/ldst_helper.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c >> index 2b0221c..a383074 100644 >> --- a/target-sparc/ldst_helper.c >> +++ b/target-sparc/ldst_helper.c >> @@ -2059,11 +2059,11 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd) >> bswap64s(&env->gregs[rd + 1]); >> } >> } else { >> - env->regwptr[rd] = cpu_ldq_nucleus(env, addr); >> - env->regwptr[rd + 1] = cpu_ldq_nucleus(env, addr + 8); >> + env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr); >> + env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8); >> if (asi == 0x2c) { >> - bswap64s(&env->regwptr[rd]); >> - bswap64s(&env->regwptr[rd + 1]); >> + bswap64s(&env->regwptr[rd - 8]); >> + bswap64s(&env->regwptr[rd + 1 - 8]); >> } >> } >> break; >> @@ -2076,8 +2076,8 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd) >> env->gregs[rd] = helper_ld_asi(env, addr, asi, 4, 0); >> env->gregs[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0); >> } else { >> - env->regwptr[rd] = helper_ld_asi(env, addr, asi, 4, 0); >> - env->regwptr[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0); >> + env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0); >> + env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4, asi, 4, 0); >> } >> break; >> } >> > > So it seems that without this patch any ldda on a non-global register is > placing the result into the wrong register which is fairly nasty. > > The patch does apply, but only with fuzz, and checkpatch.pl reports > several warnings with spacing/line endings. If you can rebase to master > and resend then you can add my Reviewed-by and I'm okay with this for > 2.6 - register corruption is not good. Can you please show the output of your checkpatch.pl report? I get: $ scripts/checkpatch.pl 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch total: 0 errors, 0 warnings, 25 lines checked 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch has no obvious style problems and is ready for Submission.
On 14/04/16 15:04, Artyom Tarasenko wrote: > Can you please show the output of your checkpatch.pl report? I get: > > $ scripts/checkpatch.pl > 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch > total: 0 errors, 0 warnings, 25 lines checked > 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch has no > obvious style problems and is ready for Submission. Okay I've just done an update pull and complete rebuild which fixes the fuzz, but I'm still seeing line ending errors in checkpatch.pl: $ ./scripts/checkpatch.pl /tmp/artyom/\[PATCH\ 1_2\ for-2.6\]\ target-sparc\:\ fix\ Nucleus\ quad\ LDD\ 128\ bit\ access\ for\ windowed\ registers.eml ERROR: DOS line endings #75: FILE: target-sparc/ldst_helper.c:2062: + env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr);^M$ ERROR: DOS line endings #76: FILE: target-sparc/ldst_helper.c:2063: + env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8);^M$ ERROR: DOS line endings #80: FILE: target-sparc/ldst_helper.c:2065: + bswap64s(&env->regwptr[rd - 8]);^M$ ERROR: DOS line endings #81: FILE: target-sparc/ldst_helper.c:2066: + bswap64s(&env->regwptr[rd + 1 - 8]);^M$ ERROR: DOS line endings #91: FILE: target-sparc/ldst_helper.c:2079: + env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0);^M$ ERROR: DOS line endings #92: FILE: target-sparc/ldst_helper.c:2080: + env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4, asi, 4, 0);^M$ total: 6 errors, 0 warnings, 25 lines checked /tmp/artyom/[PATCH 1_2 for-2.6] target-sparc: fix Nucleus quad LDD 128 bit access for windowed registers.eml has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. and: $ ./scripts/checkpatch.pl /tmp/artyom/\[PATCH\ 2_2\ for-2.6\]\ target-sparc\:\ fix\ Trap\ Based\ Address\ Register\ behavior\ for\ sparc64.eml ERROR: DOS line endings #85: FILE: target-sparc/int64_helper.c:161: + env->pc = env->tbr & ~0x7fffULL;^M$ ERROR: DOS line endings #86: FILE: target-sparc/int64_helper.c:162: + env->pc |= ((env->tl > 1) ? 1 << 14 : 0) | (intno << 5);^M$ total: 2 errors, 0 warnings, 11 lines checked /tmp/artyom/[PATCH 2_2 for-2.6] target-sparc: fix Trap Based Address Register behavior for sparc64.eml has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Possibly the DOS line endings are being added by a mail program? ATB, Mark.
On Thu, Apr 14, 2016 at 4:20 PM, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 14/04/16 15:04, Artyom Tarasenko wrote: > >> Can you please show the output of your checkpatch.pl report? I get: >> >> $ scripts/checkpatch.pl >> 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch >> total: 0 errors, 0 warnings, 25 lines checked >> 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch has no >> obvious style problems and is ready for Submission. > > Okay I've just done an update pull and complete rebuild which fixes the > fuzz, but I'm still seeing line ending errors in checkpatch.pl: > > > $ ./scripts/checkpatch.pl /tmp/artyom/\[PATCH\ 1_2\ for-2.6\]\ > target-sparc\:\ fix\ Nucleus\ quad\ LDD\ 128\ bit\ access\ for\ > windowed\ registers.eml > ERROR: DOS line endings > #75: FILE: target-sparc/ldst_helper.c:2062: > + env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr);^M$ > > ERROR: DOS line endings > #76: FILE: target-sparc/ldst_helper.c:2063: > + env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8);^M$ > > ERROR: DOS line endings > #80: FILE: target-sparc/ldst_helper.c:2065: > + bswap64s(&env->regwptr[rd - 8]);^M$ > > ERROR: DOS line endings > #81: FILE: target-sparc/ldst_helper.c:2066: > + bswap64s(&env->regwptr[rd + 1 - 8]);^M$ > > ERROR: DOS line endings > #91: FILE: target-sparc/ldst_helper.c:2079: > + env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0);^M$ > > ERROR: DOS line endings > #92: FILE: target-sparc/ldst_helper.c:2080: > + env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4, > asi, 4, 0);^M$ > > total: 6 errors, 0 warnings, 25 lines checked > > /tmp/artyom/[PATCH 1_2 for-2.6] target-sparc: fix Nucleus quad LDD 128 > bit access for windowed registers.eml has style problems, please review. > If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. > > and: > > $ ./scripts/checkpatch.pl /tmp/artyom/\[PATCH\ 2_2\ for-2.6\]\ > target-sparc\:\ fix\ Trap\ Based\ Address\ Register\ behavior\ for\ > sparc64.eml > ERROR: DOS line endings > #85: FILE: target-sparc/int64_helper.c:161: > + env->pc = env->tbr & ~0x7fffULL;^M$ > > ERROR: DOS line endings > #86: FILE: target-sparc/int64_helper.c:162: > + env->pc |= ((env->tl > 1) ? 1 << 14 : 0) | (intno << 5);^M$ > > total: 2 errors, 0 warnings, 11 lines checked > > /tmp/artyom/[PATCH 2_2 for-2.6] target-sparc: fix Trap Based Address > Register behavior for sparc64.eml has style problems, please review. If > any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. > > > Possibly the DOS line endings are being added by a mail program? I use git send-email (which uses msmtp). Used to work fine. Clicking on "download mbox" on https://patchwork.ozlabs.org/patch/610402/ and running checkpatch.pl reveals no errors. Are you sure the problem is not on your side? Kind regards, Artyom
On 14/04/16 15:49, Artyom Tarasenko wrote: >> Possibly the DOS line endings are being added by a mail program? > > I use git send-email (which uses msmtp). Used to work fine. > > Clicking on "download mbox" on > https://patchwork.ozlabs.org/patch/610402/ and running checkpatch.pl > reveals no errors. > > Are you sure the problem is not on your side? In that case please accept my apologies - it seems something in that patch is triggering my mail client to output CR+LF when exporting messages. If I switch over to mutt then then indeed the patches appear as they should. I'm not sure if Richard is around at the moment, but they look trivial enough and fix some nasty register corruption bugs so I think they are worth adding to 2.6. If you can re-send with my R-B then I'll send a pull request. ATB, Mark.
diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c index 2b0221c..a383074 100644 --- a/target-sparc/ldst_helper.c +++ b/target-sparc/ldst_helper.c @@ -2059,11 +2059,11 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd) bswap64s(&env->gregs[rd + 1]); } } else { - env->regwptr[rd] = cpu_ldq_nucleus(env, addr); - env->regwptr[rd + 1] = cpu_ldq_nucleus(env, addr + 8); + env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr); + env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8); if (asi == 0x2c) { - bswap64s(&env->regwptr[rd]); - bswap64s(&env->regwptr[rd + 1]); + bswap64s(&env->regwptr[rd - 8]); + bswap64s(&env->regwptr[rd + 1 - 8]); } } break; @@ -2076,8 +2076,8 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd) env->gregs[rd] = helper_ld_asi(env, addr, asi, 4, 0); env->gregs[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0); } else { - env->regwptr[rd] = helper_ld_asi(env, addr, asi, 4, 0); - env->regwptr[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0); + env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0); + env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4, asi, 4, 0); } break; }
Fix register offset calculation when regwptr is used. Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> --- target-sparc/ldst_helper.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)