Re: [PATCH v2 5/6] MIPS: Loongson64: Make sure the PC address is correct when 3A4000+ CPU hotplug

[Tiezhu Yang]

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@xxxxxxxxxxx>
> > Signed-off-by: Jun Yi <yijun@xxxxxxxxxxx>
> > Signed-off-by: Tiezhu Yang <yangtiezhu@xxxxxxxxxxx>
>
> 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"