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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAAofZF6VppnQZXMj59o=QDCMbw9GV=6sMRoNWnpWn+mBpE0egg@mail.gmail.com>
Date: Thu, 20 Feb 2025 10:52:13 +0100
From: Marco Crivellari <marco.crivellari@...e.com>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>, Huacai Chen <chenhuacai@...nel.org>, 
	linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Frederic Weisbecker <frederic@...nel.org>, Anna-Maria Behnsen <anna-maria@...utronix.de>, 
	Thomas Gleixner <tglx@...utronix.de>, Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v2 1/1] MIPS: Fix idle VS timer enqueue

Hi,

Sorry for the late reply.

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

How does it sound integrate the v3 commit message with:

CONFIG_CPU_MICROMIPS has been removed along with the nop instructions.
There, NOPs are 2 byte in size, so change the code with 3 _ssnop which are
always 4 byte and remove the ifdef. Added ehb to make sure the hazard
is always cleared.

something more accurate to suggest?

Thank you

On Tue, Feb 18, 2025 at 4:23 PM Maciej W. Rozycki <macro@...am.me.uk> wrote:
>
> 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



-- 

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@...e.com