linux-kernel - Re: [PATCH v6 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.2503221516450.35806@angie.orcam.me.uk>
Date: Sat, 22 Mar 2025 16:08:31 +0000 (GMT)
From: "Maciej W. Rozycki" <macro@...am.me.uk>
To: Frederic Weisbecker <frederic@...nel.org>
cc: Marco Crivellari <marco.crivellari@...e.com>, linux-mips@...r.kernel.org, 
    linux-kernel@...r.kernel.org, 
    Thomas Bogendoerfer <tsbogend@...ha.franken.de>, 
    Anna-Maria Behnsen <anna-maria@...utronix.de>, 
    Thomas Gleixner <tglx@...utronix.de>, 
    Peter Zijlstra <peterz@...radead.org>, Huacai Chen <chenhuacai@...nel.org>
Subject: Re: [PATCH v6 1/1] MIPS: Fix idle VS timer enqueue

On Fri, 21 Mar 2025, Frederic Weisbecker wrote:

> > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > > index a572ce36a24f..4e012421d00f 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 interrupt. */
> > > +	ori 	t0, 0x1f
> > 
> >  This instruction sequence still suffers from the coprocessor move delay 
> > hazard.  How many times do I need to request to get it fixed (counting 
> > three so far)?
> 
> This is because your request had follow-ups from Huacai and Marco that
> were left unanswered:
> 
>      https://lore.kernel.org/all/CAAhV-H5ptAzHTPAr1bxrgByZrnFmMK8zJ68Z++RwC=NHWjqZmw@mail.gmail.com/

 The conclusion made there is however wrong: `local_irq_enable' code 
plays no tricks with instruction scheduling and lets the toolchain 
resolve any pipeline hazards automatically, while `__r4k_wait' arranges 
for instructions to be scheduled by hand and any hazards resolved by the 
human writer of the code.  There's even explicit `.set reorder' in 
`local_irq_enable', which is redundant, because it's the default mode 
for inline assembly.

 And I can't emphasise it enough: manual instruction scheduling is tough
and ought to be restricted to cases where there is no other way really, 
such as for placing an instruction in a branch delay slot where there is 
a data antidependency between the branch and the delay-slot instruction.  
Yet this approach has often been used by code authors for other reasons 
(or I daresay no reason at all), leaving it up to the maintainers to 
keep the code working in the changing conditions while the submitter has 
long gone.  I converted some of such code in the past, but it also takes 
time and effort that does not come for free.

>      https://lore.kernel.org/all/CAAofZF4HAczyRmuRe-JmQ2wcZatevLwGTOMLf1V1okGbj7q5Wg@mail.gmail.com/

 I missed that one, sorry.  A ping would have helped, and I never have 
an issue with being pinged.  I do hope I have now addressed that concern 
with my other reply.

 Thank you for the pointers.

> We have detected this longstanding architecture specific timer handling bug on
> loongson and MIPS and we could have just dropped a report and let you guys deal with
> it. Instead we decided to spend time ourselves (especially Marco) working on
> fixes for these architectures we don't run and which we are not familiar with,
> alongway taking reviews seriously and patiently re-iterating accordingly.

 Thank you for your effort, really appreciated.  Any fixes need to be 
technically correct however, it makes no sense to get one bug replaced 
with another one.  We've got enough technical debt accumulated already 
with a platform that no longer has any commercial support and relies 
solely on voluteers keeping it alive in their limited spare time.  I do 
have a long list of outstanding issues to address and ever so little 
time to take care of them, with hardware problems additionally kicking 
in and distracting every so often too.

> So please be gentle with us.

 As always, but also emphatic on this occasion.  We're in the same boat 
really, striving against the lack of resources and issues piling, and 
now we've made some progress.  Thank you for your understanding.

  Maciej