From: Leonardo Bras <leonardo@linux.ibm.com>
To: Nathan Lynch <nathanl@linux.ibm.com>
Cc: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-kernel@vger.kernel.org, Nicholas Piggin <npiggin@gmail.com>,
Paul Mackerras <paulus@samba.org>, Nadav Amit <namit@vmware.com>,
Thomas Gleixner <tglx@linutronix.de>,
linuxppc-dev@lists.ozlabs.org,
Allison Randal <allison@lohutok.net>
Subject: Re: [PATCH 1/1] powerpc/rtas: Implement reentrant rtas call
Date: Wed, 13 May 2020 01:31:30 -0300 [thread overview]
Message-ID: <ba97d52df60ac9c4a4afc2c03121a8c263aa5a15.camel@linux.ibm.com> (raw)
In-Reply-To: <87ftdb87jf.fsf@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3666 bytes --]
Hello Nathan, thanks for the feedback!
On Fri, 2020-04-10 at 14:28 -0500, Nathan Lynch wrote:
> Leonardo Bras <leonardo@linux.ibm.com> writes:
> > 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 call.
> >
> From the language in the spec it's clear that these calls are intended
> to be reentrant with respect to themselves, but it's less clear to me
> that they are safe to call simultaneously with respect to each other or
> arbitrary other RTAS methods.
In my viewpoint, being reentrant to themselves, without being reentrant
to others would be very difficult to do, considering the way the
rtas_call is crafted to work.
I mean, I have no experience in rtas code, it's my viewpoint. In my
thoughts there is something like this:
common_path -> selects function by token -> reentrant function
|-> non-reentrant function
If there is one function that is reentrant, it means the common_path
and function selection by token would need to be reentrant too.
> > This can be useful to avoid deadlocks in crashing, where rtas-calls are
> > needed, but some other thread crashed holding the rtas.lock.
>
> Are these calls commonly used in the crash-handling path? Is this
> addressing a real issue you've seen?
>
Yes, I noticed deadlocks during crashes, like this one:
#0 arch_spin_lock
#1 lock_rtas ()
#2 rtas_call (token=8204, nargs=1, nret=1, outputs=0x0)
#3 ics_rtas_mask_real_irq (hw_irq=4100)
#4 machine_kexec_mask_interrupts
#5 default_machine_crash_shutdown
#6 machine_crash_shutdown
#7 __crash_kexec
#8 crash_kexec
#9 oops_end
On ics_rtas_mask_real_irq() we have both ibm_int_off and ibm_set_xive,
so it makes sense to also add ibm_int_on and ibm_get_xive as reentrant
too.
Full discussion available on this thread:
http://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200401000020.590447-1-leonardo@linux.ibm.com/
>
> > +/*
> > + * Used for reentrant rtas calls.
> > + * According to LoPAR documentation, only "ibm,int-on", "ibm,int-off",
> > + * "ibm,get-xive" and "ibm,set-xive" are currently reentrant.
> > + * Reentrant calls need their own rtas_args buffer, so not using rtas.args.
> > + */
>
> Please use kernel-doc format in new code.
Sure, v2 is going to be fixed.
>
>
> > +int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...)
> > +{
> > + va_list list;
> > + struct rtas_args rtas_args;
> > +
> > + if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
> > + return -1;
> > +
> > + va_start(list, outputs);
> > + va_rtas_call_unlocked(&rtas_args, token, nargs, nret, list);
> > + va_end(list);
>
> No, I don't think you can place the RTAS argument buffer on the stack:
>
> 7.2.7, Software Implementation Note:
> | The OS must be aware that the effective address range for RTAS is 4
> | GB when instantiated in 32-bit mode and the OS should not pass RTAS
> | addresses or blocks of data which might fall outside of this range.
Agree, moved to PACA.
I will send a v2 soon, it will be a 2-patch patchset.
Best regards,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-05-13 5:09 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-08 22:39 [PATCH 1/1] powerpc/rtas: Implement reentrant rtas call Leonardo Bras
2020-04-10 19:28 ` Nathan Lynch
2020-05-13 4:31 ` Leonardo Bras [this message]
2020-05-14 19:04 ` Nathan Lynch
2020-05-14 20:22 ` Leonardo Bras
2020-05-13 5:23 ` Leonardo Bras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ba97d52df60ac9c4a4afc2c03121a8c263aa5a15.camel@linux.ibm.com \
--to=leonardo@linux.ibm.com \
--cc=allison@lohutok.net \
--cc=ego@linux.vnet.ibm.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=namit@vmware.com \
--cc=nathanl@linux.ibm.com \
--cc=npiggin@gmail.com \
--cc=paulus@samba.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).