Re: [GIT pull] locking/urgent for v5.10-rc6

[Heiko Carstens <hca@linux.ibm.com>]

On Wed, Dec 02, 2020 at 10:38:05AM +0100, Peter Zijlstra wrote:
> On Wed, Dec 02, 2020 at 08:54:27AM +0100, Heiko Carstens wrote:
> > > > But but but...
> > > > 
> > > >   do_idle()			# IRQs on
> > > >     local_irq_disable();	# IRQs off
> > > >     defaul_idle_call()	# IRQs off
> > > 	lockdep_hardirqs_on();	# IRQs off, but lockdep things they're on
> > > >       arch_cpu_idle()		# IRQs off
> > > >         enabled_wait()	# IRQs off
> > > > 	  raw_local_save()	# still off
> > > > 	  psw_idle()		# very much off
> > > > 	    ext_int_handler	# get an interrupt ?!?!
> > > 	      rcu_irq_enter()	# lockdep thinks IRQs are on <- FAIL
> > > 
> > > I can't much read s390 assembler, but ext_int_handler() has a
> > > TRACE_IRQS_OFF, which would be sufficient to re-align the lockdep state
> > > with the actual state, but there's some condition before it, what's that
> > > test and is that right?
> > 
> > After digging a bit into our asm code: no, it is not right, and only
> > for psw_idle() it is wrong.
> > 
> > What happens with the current code:
> > 
> > - default_idle_call() calls lockdep_hardirqs_on() before calling into
> >   arch_cpu_idle()
> 
> Correct.
> 
> > - our arch_cpu_idle() calls psw_idle() which enables irqs. the irq
> >   handler will call/use the SWITCH_ASYNC macro which clears the
> >   interrupt enabled bits in the old program status word (_only_ for
> >   psw_idle)
> 
> This is the condition at 0: that compares r13 to psw_idle_exit?

Yes, exactly.

> > So I guess my patch which I sent yesterday evening should fix all that
> > mess
> 
> Yes, afaict it does the right thing. Exceptions should call
> TRACE_IRQS_OFF unconditionally, since the hardware will disable
> interrupts upon taking an exception, irrespective of what the previous
> context had.
> 
> On exception return the previous IRQ state is inspected and lockdep is
> changed to match (except for NMIs, which either are ignored by lockdep
> or need a little bit of extra care).

Yes, we do all that, except that it seems odd to test the previous
state for interrupts (not exceptions). Since for interrupts the
previous context obviously must have been enabled for interrupts.

Except if you play tricks with the old PSW, like we do :/