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

[Maciej W. Rozycki]

On Tue, 18 Feb 2025, Thomas Bogendoerfer wrote:

> > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > > > index a572ce36a24f..9747b216648f 100644
> > > > --- a/arch/mips/kernel/genex.S
> > > > +++ b/arch/mips/kernel/genex.S
> > > > @@ -104,25 +104,27 @@ 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
> > >
> > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but
> > > they are here for a purpose. I might still need them...
> > The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in
> > MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is
> > always 4 bytes, so we can remove #ifdefs.
> 
> ic

 This was wrong anyway (and I had arguments about it with people back in 
the time, but it was a hopeless battle).  Instead `.align ...' or an 
equivalent pseudo-op (`.balign', etc.) should have been used, removing the 
fragility of this piece or the need for CONFIG_CPU_MICROMIPS conditional.  
Here and in other places.

> > > > +     _ssnop
> > > > +     _ssnop
> > > > +     _ssnop
> > >
> > > instead of handcoded hazard nops, use __irq_enable_hazard for that
> > No, I don't think so, this region should make sure be 32 bytes on each
> > platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the
> > fallback implementation but available for all MIPS.
> 
> you are right for most cases, but there is one case
> 
> #elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \
>         defined(CONFIG_CPU_BMIPS)
> 
> which uses
> 
> #define __irq_enable_hazard                                             \
>         ___ssnop;                                                       \
>         ___ssnop;                                                       \
>         ___ssnop;                                                       \
>         ___ehb
> 
> if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as
> irq enable hazard.

 Indeed, EHB must always be included, because for R2+ CPUs SSNOP doesn't 
clear the hazard (it never clears, but with earlier CPUs it just stalls 
the pipeline long enough for the hazard to eventually clear by itself).

> > > But I doubt this works, because the wait instruction is not aligned to
> > > a 32 byte boundary, but the code assuemes this, IMHO.
> > Why? After this patch we only use 4 byte instructions.
> 
> I've should have looked at the compiled output, sorry for the noise

 Umm, there's the commit description to document justification for such 
changes made and it's not the reviewer's duty to chase every such detail 
that has been omitted from a submission.

 FAOD this is a request to include this detail in v3.

> Still this construct feels rather fragile.

 I think `.align', etc. can still be used to ensure enough space has been 
consumed and if you want to make a code sequence's size check, then a 
piece such as:

0:
	nop
	nop
	nop
	nop
1:
	.ifne	1b - 0b - 32
	.error	"code consistency verification failure"
	.endif

should do even where alignment does not matter.  And you could possibly do 
smarter stuff with `.rept' if (1b - 0b) turns out below rather than above 
the threshold required, but I'm leaving it as an exercise for the reader.

  Maciej