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

[Marco Crivellari]

Hi,

> +     /* Enable Interrput */
>                        ^^
> Typo here; also please capitalise sentences and use full stops.

Sorry, I probably forgot to run checkpatch this time.

>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().

This is the doubt I have indeed.
Quoting from the manual (about M4K):

"The Spacing column shown in Table 2.6 and Table 2.7 indicates the
number of unrelated instructions (such as NOPs or SSNOPs) that,
prior to the capabilities of Release 2, would need to be placed
between the producer and consumer of the hazard in order to ensure
that the effects of the first instruction are seen by the second instruction."

The "Spacing column" value is 3, indeed.

"With the hazard elimination instructions available in Release 2, the
preferred method to eliminate hazards is to place one of the
instructions listed in Table 2.8 between the producer and consumer of the
hazard. Execution hazards can be removed by using the EHB [...]"

Not sure if I'm missing something here.

Thanks (to everyone)!

On Mon, Mar 3, 2025 at 9:13 AM Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
>
> 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



--

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@xxxxxxxx