Re: [PATCH v4 2/2] powerpc/rtas: Implement reentrant rtas call

Hello Nick,

On Fri, 2020-05-15 at 17:30 +1000, Nicholas Piggin wrote:

Excerpts from Leonardo Bras's message of May 15, 2020 9:51 am:

quoted

Implement rtas_call_reentrant() for reentrant rtas-calls:
"ibm,int-on", "ibm,int-off",ibm,get-xive" and  "ibm,set-xive".

On LoPAPR Version 1.1 (March 24, 2016), from 7.3.10.1 to 7.3.10.4,
items 2 and 3 say:

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

So, these rtas-calls can be called in a lockless way, if using
a different buffer for each cpu doing such rtas call.

What about rtas_call_unlocked? Do the callers need to take the rtas 
lock?

Machine checks must call ibm,nmi-interlock too, which we really don't 
want to take a lock for either. Hopefully that's in a class of its own
and we can essentially ignore with respect to other rtas calls.

The spec is pretty vague too :(

"The ibm,get-xive call must be reentrant to the number of processors on 
the platform."

This suggests ibm,get-xive can be called concurrently by multiple
processors. It doesn't say anything about being re-entrant against any 
of the other re-entrant calls. Maybe that could be reasonably assumed,
but I don't know if it's reasonable to assume it can be called 
concurrently with a *non-reentrant* call, is it?

This was discussed on a previous version of the patchset:

https://lore.kernel.org/linuxppc-dev/875zcy2v8o.fsf@linux.ibm.com/ (local)

He checked with partition firmware development and these calls can be
used concurrently with arbitrary other RTAS calls.

quoted

For this, it was suggested to add the buffer (struct rtas_args)
in the PACA struct, so each cpu can have it's own buffer.

You can't do this, paca is not limited to RTAS_INSTANTIATE_MAX.
Which is good, because I didn't want you to add another 88 bytes to the 
paca :) Can you make it a pointer and allocate it separately? Check
the slb_shadow allocation, you could use a similar pattern.

Sure, I will send the next version with this change.

The other option would be to have just one more rtas args, and have the 
crashing CPU always that. That would skirt the re-entrancy issue -- the
concurrency is only ever a last resort. Would be a bit tricker though.

It seems a good idea, but I would like to try the previous alternative
first.

Thanks,
Nick

Thank you Nick!