Re: [PATCH v5 1/1] MIPS: Fix idle VS timer enqueue

[Huacai Chen]

Hi, Maciej,

On Sun, Mar 2, 2025 at 8:54 AM Maciej W. Rozycki <macro@xxxxxxxxxxx> wrote:
>
> On Fri, 28 Feb 2025, Marco Crivellari wrote:
>
> > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > index a572ce36a24f..474738d9124e 100644
> > --- a/arch/mips/kernel/genex.S
> > +++ b/arch/mips/kernel/genex.S
> > @@ -104,27 +104,30 @@ handle_vcei:
> >
> >       __FINIT
> >
> > -     .align  5       /* 32 byte rollback region */
> > +     .align  5
> >  LEAF(__r4k_wait)
> >       .set    push
> >       .set    noreorder
> > -     /* start of rollback region */
> > -     LONG_L  t0, TI_FLAGS($28)
> > -     nop
> > -     andi    t0, _TIF_NEED_RESCHED
> > -     bnez    t0, 1f
> > -      nop
> > -     nop
> > -     nop
> > -#ifdef CONFIG_CPU_MICROMIPS
> > -     nop
> > -     nop
> > -     nop
> > -     nop
> > -#endif
> > +     /* start of idle interrupt region */
> > +     MFC0    t0, CP0_STATUS
> > +     /* Enable Interrput */
>                         ^^
>  Typo here; also please capitalise sentences and use full stops.
>
> > +     ori     t0, 0x1f
>
>  No time for a thorough review here as I'm heading for a holiday right
> away, but I can see you still have a coprocessor move hazard here with
> MIPS III hardware.  The incoming value of $t0 to ORI is not what MFC0 has
> obtained.  It's the value from before MFC0.
If this is a problem, then the current local_irq_enable() is also
wrong for all MIPS III hardware, because this patch uses the same
instruction sequence of local_irq_enable().


Huacai

>
> > +     xori    t0, 0x1e
>
>  And then it's only this XORI that sees the output from MFC0 (though
> there's actually a race between the two competing writes to $t0), so
> effectively you clobber the CP0.Status register...
>
> > +     MTC0    t0, CP0_STATUS
>
>  ... here.  You need to schedule your instructions differently.  But do
> you need `.set noreorder' in the first place?  Can you maybe find a way to
> avoid it, making all the hazard avoidance stuff much easier?
>
>   Maciej