Re: [PATCH v3 1/1] ppc/crash: Reset spinlocks during crash

[Leonardo Bras]

Hello Nick, Michael,

On Fri, 2020-04-03 at 16:41 +1000, Nicholas Piggin wrote:
[...]
> > > PAPR says we are not allowed to have multiple CPUs calling RTAS at once,
> > > except for a very small list of RTAS calls. So if we bust the RTAS lock
> > > there's a risk we violate that part of PAPR and crash even harder.
> > 
> > Interesting, I was not aware.
> > 
> > > Also it's not specific to kdump, we can't even get through a normal
> > > reboot if we crash with the RTAS lock held.
> > > 
> > > Anyway here's a patch with some ideas. That allows me to get from a
> > > crash with the RTAS lock held through kdump into the 2nd kernel. But it
> > > only works if it's the crashing CPU that holds the RTAS lock.
> > > 
> > 
> > Nice idea. 
> > But my test environment is just triggering a crash from sysrq, so I
> > think it would not improve the result, given that this thread is
> > probably not holding the lock by the time.
> 
> Crash paths should not take that RTAS lock, it's a massive pain. I'm 
> fixing it for machine check, for other crashes I think it can be removed 
> too, it just needs to be unpicked. The good thing with crashing is that 
> you can reasonably *know* that you're single threaded, so you can 
> usually reason through situations like above.
> 
> > I noticed that when rtas is locked, irqs and preemption are also
> > disabled.
> > 
> > Should the IPI send by crash be able to interrupt a thread with
> > disabled irqs?
> 
> Yes. It's been a bit painful, but in the long term it means that a CPU 
> which hangs with interrupts off can be debugged, and it means we can 
> take it offline to crash without risking that it will be clobbering what 
> we're doing.
> 
> Arguably what I should have done is try a regular IPI first, wait a few 
> seconds, then NMI IPI.
> 
> A couple of problems with that. Firstly it probably avoids this issue 
> you hit almost all the time, so it won't get fixed. So when we really 
> need the NMI IPI in the field, it'll still be riddled with deadlocks.
> 
> Secondly, sending the IPI first in theory can be more intrusive to the 
> state that we want to debug. It uses the currently running stack, paca 
> save areas, ec. NMI IPI uses its own stack and save regions so it's a 
> little more isolated. Maybe this is only a small advantage but I'd like 
> to have it if we can.  
> 
> Thanks,
> Nick


I think the printk issue is solved (sent a patch on that), now what is
missing is the rtas call spinlock.

I noticed that rtas.lock is taken on machine_kexec_mask_interrupts(),
which happen after crashing the other threads and getting into
realmode. 

The following rtas are called each IRQ with valid interrupt descriptor:
ibm,int-off : Reset mask bit for that interrupt
ibm,set_xive : Set XIVE priority to 0xff

By what I could understand, these rtas calls happen to put the next
kexec kernel (kdump kernel) in a safer environment, so I think it's not
safe to just remove them.
 
(See commit d6c1a9081080c6c4658acf2a06d851feb2855933)

On the other hand, busting the rtas.lock could be dangerous, because
it's code we can't control.

According with LoPAR, for both of these rtas-calls, we have:

For the PowerPC External Interrupt option: The call must be reentrant
to the number of processors on the platform.
For the PowerPC External Interrupt option: The argument call buffer for
each simultaneous call must be physically unique.

Which I think means this rtas-calls can be done simultaneously.
Would it mean that busting the rtas.lock for these calls would be safe?

Best regards,
Leonardo Bras

Attachment: signature.asc
Description: This is a digitally signed message part