| Message ID | 20200202153409.28534-1-jrtc27@jrtc27.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/mips: Fix ll/sc after 7dd547e5ab6b31e7a0cfc182d3ad131dd55a948f | expand |
On 2/2/20 3:34 PM, James Clarke wrote: > From: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk> > > After 7dd547e5ab6b31e7a0cfc182d3ad131dd55a948f the env->llval value is > loaded as an unsigned value (instead of sign-extended as before). > Therefore, the CMPXCHG in gen_st_cond() in translate.c fails if the sign > bit is set in the loaded value. > Fix this by sign-extending the llval value for the 32-bit case. > > I discovered this issue because FreeBSD MIPS64 was looping forever in an > atomic helper function when trying to start /sbin/init. > > Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk> > Fixes: 7dd547e5ab6b ("target/mips: Use cpu_*_mmuidx_ra instead of MMU_MODE*_SUFFIX") > Buglink: https://bugs.launchpad.net/qemu/+bug/1861605 > Cc: Aurelien Jarno <aurelien@aurel32.net> > Cc: Aleksandar Markovic <amarkovic@wavecomp.com> > Cc: Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com> > Cc: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: James Clarke <jrtc27@jrtc27.com> > --- > target/mips/op_helper.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) My fault. Sorry about that. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Mon, Feb 3, 2020 at 8:52 AM Richard Henderson <richard.henderson@linaro.org> wrote: > On 2/2/20 3:34 PM, James Clarke wrote: > > From: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk> > > > > After 7dd547e5ab6b31e7a0cfc182d3ad131dd55a948f the env->llval value is > > loaded as an unsigned value (instead of sign-extended as before). > > Therefore, the CMPXCHG in gen_st_cond() in translate.c fails if the sign > > bit is set in the loaded value. > > Fix this by sign-extending the llval value for the 32-bit case. Thanks! This fix the problem I spent my Saturday debugging: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00071.html Too bad we crossed by few hours, you'd have saved my Saturday. Thomas, I think this also fixes the problem you reported here: https://www.mail-archive.com/qemu-devel@nongnu.org/msg676025.html Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > > I discovered this issue because FreeBSD MIPS64 was looping forever in an > > atomic helper function when trying to start /sbin/init. > > > > Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk> > > Fixes: 7dd547e5ab6b ("target/mips: Use cpu_*_mmuidx_ra instead of MMU_MODE*_SUFFIX") > > Buglink: https://bugs.launchpad.net/qemu/+bug/1861605 > > Cc: Aurelien Jarno <aurelien@aurel32.net> > > Cc: Aleksandar Markovic <amarkovic@wavecomp.com> > > Cc: Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com> > > Cc: Richard Henderson <richard.henderson@linaro.org> > > Signed-off-by: James Clarke <jrtc27@jrtc27.com> > > --- > > target/mips/op_helper.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > My fault. Sorry about that. > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > > r~ >
> From: Richard Henderson <richard.henderson@linaro.org> > Sent: Monday, February 3, 2020 8:52 AM > To: James Clarke; qemu-devel@nongnu.org > Cc: Alex Richardson; Aurelien Jarno; Aleksandar Markovic; Aleksandar Rikalo > Subject: [EXTERNAL]Re: [PATCH] target/mips: Fix ll/sc after > 7dd547e5ab6b31e7a0cfc182d3ad131dd55a948f > > On 2/2/20 3:34 PM, James Clarke wrote: > > From: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk> > > > > After 7dd547e5ab6b31e7a0cfc182d3ad131dd55a948f the env->llval value is > > loaded as an unsigned value (instead of sign-extended as before). > > Therefore, the CMPXCHG in gen_st_cond() in translate.c fails if the sign > > bit is set in the loaded value. > > Fix this by sign-extending the llval value for the 32-bit case. > > > > I discovered this issue because FreeBSD MIPS64 was looping forever in an > > atomic helper function when trying to start /sbin/init. > > > > Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk> > > Fixes: 7dd547e5ab6b ("target/mips: Use cpu_*_mmuidx_ra instead of MMU_MODE*_SUFFIX") > > Buglink: https://bugs.launchpad.net/qemu/+bug/1861605 > > Cc: Aurelien Jarno <aurelien@aurel32.net> > > Cc: Aleksandar Markovic <amarkovic@wavecomp.com> > > Cc: Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com> > > Cc: Richard Henderson <richard.henderson@linaro.org> > > Signed-off-by: James Clarke <jrtc27@jrtc27.com> > > --- > > target/mips/op_helper.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > My fault. Sorry about that. > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Applied to the next mips queue. Thanks to everyone involved!! Aleksandar > r~
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c index 15d05a5fbc..467914d1ca 100644 --- a/target/mips/op_helper.c +++ b/target/mips/op_helper.c @@ -305,7 +305,7 @@ static inline hwaddr do_translate_address(CPUMIPSState *env, } } -#define HELPER_LD_ATOMIC(name, insn, almask) \ +#define HELPER_LD_ATOMIC(name, insn, almask, do_cast) \ target_ulong helper_##name(CPUMIPSState *env, target_ulong arg, int mem_idx) \ { \ if (arg & almask) { \ @@ -316,12 +316,12 @@ target_ulong helper_##name(CPUMIPSState *env, target_ulong arg, int mem_idx) \ } \ env->CP0_LLAddr = do_translate_address(env, arg, 0, GETPC()); \ env->lladdr = arg; \ - env->llval = cpu_##insn##_mmuidx_ra(env, arg, mem_idx, GETPC()); \ + env->llval = do_cast cpu_##insn##_mmuidx_ra(env, arg, mem_idx, GETPC()); \ return env->llval; \ } -HELPER_LD_ATOMIC(ll, ldl, 0x3) +HELPER_LD_ATOMIC(ll, ldl, 0x3, (target_long)(int32_t)) #ifdef TARGET_MIPS64 -HELPER_LD_ATOMIC(lld, ldq, 0x7) +HELPER_LD_ATOMIC(lld, ldq, 0x7, (target_ulong)) #endif #undef HELPER_LD_ATOMIC #endif