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: <Z-KwKACJQhH98EoW@localhost.localdomain>
Date: Tue, 25 Mar 2025 14:31:20 +0100
From: Frederic Weisbecker <frederic@...nel.org>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
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

Le Sat, Mar 22, 2025 at 04:08:31PM +0000, Maciej W. Rozycki a écrit :
> 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.

Hopefully, I'll let Marco follow-up on that as I must confess I'm lost
with these details. But your help has been very valuable!

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

Yeah I totally understand that!

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

Heh I know... Thanks a lot!

> 
>   Maciej