| Message ID | 1604373306-3599-6-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Modify some registers operations and move decode_cpucfg() to loongson_regs.h | expand |
在 2020/11/3 11:15, Tiezhu Yang 写道: > In loongson3_type3_play_dead(), in order to make sure the PC address is > correct, use lw to read the low 32 bits first, if the result is not zero, > then use ld to read the whole 64 bits, otherwise there maybe exists atomic > problem due to write high 32 bits first and then low 32 bits, like this: > > high 32 bits (write done) > -- only read high 32-bits which is wrong > low 32 bits (not yet write done) > > This problem is especially for Loongson 3A4000+ CPU due to using Mail_Send > register which can only send 32 bits data one time. Although it is hard to > reproduce, we can do something at the software level to avoid the risks for > 3A4000+ CPU, this change has no influence on the other Loongson CPUs. > > Signed-off-by: Lu Zeng <zenglu@loongson.cn> > Signed-off-by: Jun Yi <yijun@loongson.cn> > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> Hi Tiezhu, Sorry that I didn't look this patch carefully in previous rev, here's my comments, Firstly the commit message and code comment looks bogus... I'd prefer --- MIPS: Loongson64: SMP: Fix up play_dead jump indicator In play_dead function, the whole 64-bit PC mailbox was used as a indicator to determine if the master core had written boot jump information. However, after we introduced CSR mailsend, the 64-bit PC mailbox won't be written atomicly. Thus we have to use the lower 32-bit, which will be written at the last, as the jump indicator instead. -- Thanks. > --- > > v2: No changes > > arch/mips/loongson64/smp.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/mips/loongson64/smp.c b/arch/mips/loongson64/smp.c > index 736e98d..e32b46e 100644 > --- a/arch/mips/loongson64/smp.c > +++ b/arch/mips/loongson64/smp.c > @@ -764,9 +764,10 @@ static void loongson3_type3_play_dead(int *state_addr) > "1: li %[count], 0x100 \n" /* wait for init loop */ > "2: bnez %[count], 2b \n" /* limit mailbox access */ > " addiu %[count], -1 \n" > - " ld %[initfunc], 0x20(%[base]) \n" /* get PC via mailbox */ > + " lw %[initfunc], 0x20(%[base]) \n" /* get PC (low 32 bits) via mailbox */ Here you can comment as "Check jump indicator (lower 32-bit of PC mailbox)" Thanks. - Jiaxun > " beqz %[initfunc], 1b \n" > " nop \n" > + " ld %[initfunc], 0x20(%[base]) \n" /* get PC (whole 64 bits) via mailbox */ > " ld $sp, 0x28(%[base]) \n" /* get SP via mailbox */ > " ld $gp, 0x30(%[base]) \n" /* get GP via mailbox */ > " ld $a1, 0x38(%[base]) \n"
On 11/03/2020 01:28 PM, Jiaxun Yang wrote: > > > 在 2020/11/3 11:15, Tiezhu Yang 写道: >> In loongson3_type3_play_dead(), in order to make sure the PC address is >> correct, use lw to read the low 32 bits first, if the result is not >> zero, >> then use ld to read the whole 64 bits, otherwise there maybe exists >> atomic >> problem due to write high 32 bits first and then low 32 bits, like this: >> >> high 32 bits (write done) >> -- only read high 32-bits which is >> wrong >> low 32 bits (not yet write done) >> >> This problem is especially for Loongson 3A4000+ CPU due to using >> Mail_Send >> register which can only send 32 bits data one time. Although it is >> hard to >> reproduce, we can do something at the software level to avoid the >> risks for >> 3A4000+ CPU, this change has no influence on the other Loongson CPUs. >> >> Signed-off-by: Lu Zeng <zenglu@loongson.cn> >> Signed-off-by: Jun Yi <yijun@loongson.cn> >> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > > Hi Tiezhu, > > Sorry that I didn't look this patch carefully in previous rev, here's > my comments, > > Firstly the commit message and code comment looks bogus... > > I'd prefer Hi Jiaxun, Thanks for your detail review, it looks better. Let me update it and then send v3. Thanks, Tiezhu > > --- > MIPS: Loongson64: SMP: Fix up play_dead jump indicator > > In play_dead function, the whole 64-bit PC mailbox was used as a > indicator > to determine if the master core had written boot jump information. > > However, after we introduced CSR mailsend, the 64-bit PC mailbox won't be > written atomicly. Thus we have to use the lower 32-bit, which will be > written at > the last, as the jump indicator instead. > -- > > Thanks. > >> --- >> >> v2: No changes >> >> arch/mips/loongson64/smp.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/mips/loongson64/smp.c b/arch/mips/loongson64/smp.c >> index 736e98d..e32b46e 100644 >> --- a/arch/mips/loongson64/smp.c >> +++ b/arch/mips/loongson64/smp.c >> @@ -764,9 +764,10 @@ static void loongson3_type3_play_dead(int >> *state_addr) >> "1: li %[count], 0x100 \n" /* wait for init >> loop */ >> "2: bnez %[count], 2b \n" /* limit mailbox >> access */ >> " addiu %[count], -1 \n" >> - " ld %[initfunc], 0x20(%[base]) \n" /* get PC via >> mailbox */ >> + " lw %[initfunc], 0x20(%[base]) \n" /* get PC (low 32 >> bits) via mailbox */ > > Here you can comment as "Check jump indicator (lower 32-bit of PC > mailbox)" > > Thanks. > > - Jiaxun >> " beqz %[initfunc], 1b \n" >> " nop \n" >> + " ld %[initfunc], 0x20(%[base]) \n" /* get PC (whole >> 64 bits) via mailbox */ >> " ld $sp, 0x28(%[base]) \n" /* get SP via >> mailbox */ >> " ld $gp, 0x30(%[base]) \n" /* get GP via >> mailbox */ >> " ld $a1, 0x38(%[base]) \n"
diff --git a/arch/mips/loongson64/smp.c b/arch/mips/loongson64/smp.c index 736e98d..e32b46e 100644 --- a/arch/mips/loongson64/smp.c +++ b/arch/mips/loongson64/smp.c @@ -764,9 +764,10 @@ static void loongson3_type3_play_dead(int *state_addr) "1: li %[count], 0x100 \n" /* wait for init loop */ "2: bnez %[count], 2b \n" /* limit mailbox access */ " addiu %[count], -1 \n" - " ld %[initfunc], 0x20(%[base]) \n" /* get PC via mailbox */ + " lw %[initfunc], 0x20(%[base]) \n" /* get PC (low 32 bits) via mailbox */ " beqz %[initfunc], 1b \n" " nop \n" + " ld %[initfunc], 0x20(%[base]) \n" /* get PC (whole 64 bits) via mailbox */ " ld $sp, 0x28(%[base]) \n" /* get SP via mailbox */ " ld $gp, 0x30(%[base]) \n" /* get GP via mailbox */ " ld $a1, 0x38(%[base]) \n"
In loongson3_type3_play_dead(), in order to make sure the PC address is correct, use lw to read the low 32 bits first, if the result is not zero, then use ld to read the whole 64 bits, otherwise there maybe exists atomic problem due to write high 32 bits first and then low 32 bits, like this: high 32 bits (write done) -- only read high 32-bits which is wrong low 32 bits (not yet write done) This problem is especially for Loongson 3A4000+ CPU due to using Mail_Send register which can only send 32 bits data one time. Although it is hard to reproduce, we can do something at the software level to avoid the risks for 3A4000+ CPU, this change has no influence on the other Loongson CPUs. Signed-off-by: Lu Zeng <zenglu@loongson.cn> Signed-off-by: Jun Yi <yijun@loongson.cn> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- v2: No changes arch/mips/loongson64/smp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)