[PATCH 00/13] RTAS maintenance

[PATCH 00/13] RTAS maintenance

[Nathan Lynch]

A collection of loosely-related RTAS code changes, most notably:

* Fixing misuses of rtas_token() for non-function properties.
* The stronger validation for sys_rtas() offered by the
  PPC_RTAS_FILTER config option is always enabled.
* Improved function token lookups, including efficient "reverse"
  token-to-name mappings.
* Static tracepoints for RTAS entry and exit.

Nathan Lynch (13):
  powerpc/rtas: document rtas_call()
  powerpc/rtasd: use correct OF API for event scan rate
  powerpc/rtas: avoid device tree lookups in rtas_os_term()
  powerpc/rtas: avoid scheduling in rtas_os_term()
  powerpc/pseries/eeh: use correct API for error log size
  powerpc/rtas: clean up rtas_error_log_max initialization
  powerpc/rtas: clean up includes
  powerpc/rtas: define pr_fmt and convert printk call sites
  powerpc/rtas: mandate RTAS syscall filtering
  powerpc/rtas: improve function information lookups
  powerpc/rtas: strengthen do_enter_rtas() type safety, drop inline
  powerpc/tracing: tracepoints for RTAS entry and exit
  powerpc/rtas: place tracepoints in do_enter_rtas()

 arch/powerpc/Kconfig                         |  13 -
 arch/powerpc/include/asm/rtas.h              | 102 +-
 arch/powerpc/include/asm/trace.h             | 116 +++
 arch/powerpc/kernel/rtas.c                   | 961 +++++++++++++++----
 arch/powerpc/kernel/rtasd.c                  |   7 +-
 arch/powerpc/platforms/pseries/eeh_pseries.c |   2 +-
 6 files changed, 986 insertions(+), 215 deletions(-)

-- 
2.37.1

[PATCH 01/13] powerpc/rtas: document rtas_call()

[Nathan Lynch]

rtas_call() has a complex calling convention, non-standard return
values, and many users. Add kernel-doc for it and remove the less
structured commentary from rtas.h.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/include/asm/rtas.h | 15 ---------
 arch/powerpc/kernel/rtas.c      | 58 +++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 56319aea646e..479a95cb2770 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -33,21 +33,6 @@
 #define RTAS_THREADS_ACTIVE     -9005 /* Multiple processor threads active */
 #define RTAS_OUTSTANDING_COPROC -9006 /* Outstanding coprocessor operations */
 
-/*
- * In general to call RTAS use rtas_token("string") to lookup
- * an RTAS token for the given string (e.g. "event-scan").
- * To actually perform the call use
- *    ret = rtas_call(token, n_in, n_out, ...)
- * Where n_in is the number of input parameters and
- *       n_out is the number of output parameters
- *
- * If the "string" is invalid on this system, RTAS_UNKNOWN_SERVICE
- * will be returned as a token.  rtas_call() does look for this
- * token and error out gracefully so rtas_call(rtas_token("str"), ...)
- * may be safely used for one-shot calls to RTAS.
- *
- */
-
 /* RTAS event classes */
 #define RTAS_INTERNAL_ERROR		0x80000000 /* set bit 0 */
 #define RTAS_EPOW_WARNING		0x40000000 /* set bit 1 */
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index e847f9b1c5b9..c12dd5ed5e00 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -467,6 +467,64 @@ void rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret,
 static int ibm_open_errinjct_token;
 static int ibm_errinjct_token;
 
+/**
+ * rtas_call() - Invoke an RTAS firmware function.
+ * @token: Identifies the function being invoked.
+ * @nargs: Number of input parameters. Does not include token.
+ * @nret: Number of output parameters, including the call status.
+ * @outputs: Array of @nret output words.
+ * @....: List of @nargs input parameters.
+ *
+ * Invokes the RTAS function indicated by @token, which the caller
+ * should obtain via rtas_token().
+ *
+ * The @nargs and @nret arguments must match the number of input and
+ * output parameters specified for the RTAS function.
+ *
+ * rtas_call() returns RTAS status codes, not conventional Linux errno
+ * values. Callers must translate any failure to an appropriate errno
+ * in syscall context. Most callers of RTAS functions that can return
+ * -2 or 990x should use rtas_busy_delay() to correctly handle those
+ * statuses before calling again.
+ *
+ * The return value descriptions are adapted from 7.2.8 [RTAS] Return
+ * Codes of the PAPR and CHRP specifications.
+ *
+ * Context: Process context preferably, interrupt context if
+ *          necessary.  Acquires an internal spinlock and may perform
+ *          GFP_ATOMIC slab allocation in error path. Unsafe for NMI
+ *          context.
+ * Return:
+ * *                          0 - RTAS function call succeeded.
+ * *                         -1 - RTAS function encountered a hardware or
+ *                                platform error, or the token is invalid,
+ *                                or the function is restricted by kernel policy.
+ * *                         -2 - Specs say "A necessary hardware device was busy,
+ *                                and the requested function could not be
+ *                                performed. The operation should be retried at
+ *                                a later time." This is misleading, at least with
+ *                                respect to current RTAS implementations. What it
+ *                                usually means in practice is that the function
+ *                                could not be completed while meeting RTAS's
+ *                                deadline for returning control to the OS (250us
+ *                                for PAPR/PowerVM, typically), but the call may be
+ *                                immediately reattempted to resume work on it.
+ * *                         -3 - Parameter error.
+ * *                         -7 - Unexpected state change.
+ * *                9000...9899 - Vendor-specific success codes.
+ * *                9900...9905 - Advisory extended delay. Caller should try
+ *                                again after ~10^x ms has elapsed, where x is
+ *                                the last digit of the status [0-5]. Again going
+ *                                beyond the PAPR text, 990x on PowerVM indicates
+ *                                contention for RTAS-internal resources. Other
+ *                                RTAS call sequences in progress should be
+ *                                allowed to complete before reattempting the
+ *                                call.
+ * *                      -9000 - Multi-level isolation error.
+ * *              -9999...-9004 - Vendor-specific error codes.
+ * * Additional negative values - Function-specific error.
+ * * Additional positive values - Function-specific success.
+ */
 int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 {
 	va_list list;
-- 
2.37.1

Re: [PATCH 01/13] powerpc/rtas: document rtas_call()

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> rtas_call() has a complex calling convention, non-standard return
> values, and many users. Add kernel-doc for it and remove the less
> structured commentary from rtas.h.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Excellent work!

I'm not overly familiar with some of the RTAS internal stuff you
describe, but I've checked the status codes against my copy of PAPR and
they concur, so I'll give this a:

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/include/asm/rtas.h | 15 ---------
>  arch/powerpc/kernel/rtas.c      | 58
> +++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/rtas.h
> b/arch/powerpc/include/asm/rtas.h
> index 56319aea646e..479a95cb2770 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -33,21 +33,6 @@
>  #define RTAS_THREADS_ACTIVE     -9005 /* Multiple processor threads
> active */
>  #define RTAS_OUTSTANDING_COPROC -9006 /* Outstanding coprocessor
> operations */
>  
> -/*
> - * In general to call RTAS use rtas_token("string") to lookup
> - * an RTAS token for the given string (e.g. "event-scan").
> - * To actually perform the call use
> - *    ret = rtas_call(token, n_in, n_out, ...)
> - * Where n_in is the number of input parameters and
> - *       n_out is the number of output parameters
> - *
> - * If the "string" is invalid on this system, RTAS_UNKNOWN_SERVICE
> - * will be returned as a token.  rtas_call() does look for this
> - * token and error out gracefully so rtas_call(rtas_token("str"),
> ...)
> - * may be safely used for one-shot calls to RTAS.
> - *
> - */
> -
>  /* RTAS event classes */
>  #define RTAS_INTERNAL_ERROR            0x80000000 /* set bit 0 */
>  #define RTAS_EPOW_WARNING              0x40000000 /* set bit 1 */
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index e847f9b1c5b9..c12dd5ed5e00 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -467,6 +467,64 @@ void rtas_call_unlocked(struct rtas_args *args,
> int token, int nargs, int nret,
>  static int ibm_open_errinjct_token;
>  static int ibm_errinjct_token;
>  
> +/**
> + * rtas_call() - Invoke an RTAS firmware function.
> + * @token: Identifies the function being invoked.
> + * @nargs: Number of input parameters. Does not include token.
> + * @nret: Number of output parameters, including the call status.
> + * @outputs: Array of @nret output words.
> + * @....: List of @nargs input parameters.
> + *
> + * Invokes the RTAS function indicated by @token, which the caller
> + * should obtain via rtas_token().
> + *
> + * The @nargs and @nret arguments must match the number of input and
> + * output parameters specified for the RTAS function.
> + *
> + * rtas_call() returns RTAS status codes, not conventional Linux
> errno
> + * values. Callers must translate any failure to an appropriate
> errno
> + * in syscall context. Most callers of RTAS functions that can
> return
> + * -2 or 990x should use rtas_busy_delay() to correctly handle those
> + * statuses before calling again.
> + *
> + * The return value descriptions are adapted from 7.2.8 [RTAS]
> Return
> + * Codes of the PAPR and CHRP specifications.
> + *
> + * Context: Process context preferably, interrupt context if
> + *          necessary.  Acquires an internal spinlock and may
> perform
> + *          GFP_ATOMIC slab allocation in error path. Unsafe for NMI
> + *          context.
> + * Return:
> + * *                          0 - RTAS function call succeeded.
> + * *                         -1 - RTAS function encountered a
> hardware or
> + *                                platform error, or the token is
> invalid,
> + *                                or the function is restricted by
> kernel policy.
> + * *                         -2 - Specs say "A necessary hardware
> device was busy,
> + *                                and the requested function could
> not be
> + *                                performed. The operation should be
> retried at
> + *                                a later time." This is misleading,
> at least with
> + *                                respect to current RTAS
> implementations. What it
> + *                                usually means in practice is that
> the function
> + *                                could not be completed while
> meeting RTAS's
> + *                                deadline for returning control to
> the OS (250us
> + *                                for PAPR/PowerVM, typically), but
> the call may be
> + *                                immediately reattempted to resume
> work on it.
> + * *                         -3 - Parameter error.
> + * *                         -7 - Unexpected state change.
> + * *                9000...9899 - Vendor-specific success codes.
> + * *                9900...9905 - Advisory extended delay. Caller
> should try
> + *                                again after ~10^x ms has elapsed,
> where x is
> + *                                the last digit of the status [0-
> 5]. Again going
> + *                                beyond the PAPR text, 990x on
> PowerVM indicates
> + *                                contention for RTAS-internal
> resources. Other
> + *                                RTAS call sequences in progress
> should be
> + *                                allowed to complete before
> reattempting the
> + *                                call.
> + * *                      -9000 - Multi-level isolation error.
> + * *              -9999...-9004 - Vendor-specific error codes.
> + * * Additional negative values - Function-specific error.
> + * * Additional positive values - Function-specific success.
> + */
>  int rtas_call(int token, int nargs, int nret, int *outputs, ...)
>  {
>         va_list list;

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH 02/13] powerpc/rtasd: use correct OF API for event scan rate

[Nathan Lynch]

rtas_token() should be used only for properties that are RTAS function
tokens. "rtas-event-scan-rate" does not contain a function token, but it
has the same size/format as token properties so reading it with
rtas_token() happens to work.

Convert to of_property_read_u32().

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtasd.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
index 5270b450bbde..cc56ac6ba4b0 100644
--- a/arch/powerpc/kernel/rtasd.c
+++ b/arch/powerpc/kernel/rtasd.c
@@ -9,6 +9,7 @@
 #include <linux/errno.h>
 #include <linux/sched.h>
 #include <linux/kernel.h>
+#include <linux/of.h>
 #include <linux/poll.h>
 #include <linux/proc_fs.h>
 #include <linux/init.h>
@@ -499,6 +500,8 @@ EXPORT_SYMBOL_GPL(rtas_cancel_event_scan);
 
 static int __init rtas_event_scan_init(void)
 {
+	int err;
+
 	if (!machine_is(pseries) && !machine_is(chrp))
 		return 0;
 
@@ -509,8 +512,8 @@ static int __init rtas_event_scan_init(void)
 		return -ENODEV;
 	}
 
-	rtas_event_scan_rate = rtas_token("rtas-event-scan-rate");
-	if (rtas_event_scan_rate == RTAS_UNKNOWN_SERVICE) {
+	err = of_property_read_u32(rtas.dev, "rtas-event-scan-rate", &rtas_event_scan_rate);
+	if (err) {
 		printk(KERN_ERR "rtasd: no rtas-event-scan-rate on system\n");
 		return -ENODEV;
 	}
-- 
2.37.1

Re: [PATCH 02/13] powerpc/rtasd: use correct OF API for event scan rate

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> rtas_token() should be used only for properties that are RTAS
> function
> tokens. "rtas-event-scan-rate" does not contain a function token, but
> it
> has the same size/format as token properties so reading it with
> rtas_token() happens to work.
> 
> Convert to of_property_read_u32().
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Did a quick grep for the other OF device tree properties listed in R1-
7.2.6-3 of PAPR to see if we make the same mistake elsewhere, and the
only other one is rtas-error-log-max which I see you clean up later in
the series.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/kernel/rtasd.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtasd.c
> b/arch/powerpc/kernel/rtasd.c
> index 5270b450bbde..cc56ac6ba4b0 100644
> --- a/arch/powerpc/kernel/rtasd.c
> +++ b/arch/powerpc/kernel/rtasd.c
> @@ -9,6 +9,7 @@
>  #include <linux/errno.h>
>  #include <linux/sched.h>
>  #include <linux/kernel.h>
> +#include <linux/of.h>
>  #include <linux/poll.h>
>  #include <linux/proc_fs.h>
>  #include <linux/init.h>
> @@ -499,6 +500,8 @@ EXPORT_SYMBOL_GPL(rtas_cancel_event_scan);
>  
>  static int __init rtas_event_scan_init(void)
>  {
> +       int err;
> +
>         if (!machine_is(pseries) && !machine_is(chrp))
>                 return 0;
>  
> @@ -509,8 +512,8 @@ static int __init rtas_event_scan_init(void)
>                 return -ENODEV;
>         }
>  
> -       rtas_event_scan_rate = rtas_token("rtas-event-scan-rate");
> -       if (rtas_event_scan_rate == RTAS_UNKNOWN_SERVICE) {
> +       err = of_property_read_u32(rtas.dev, "rtas-event-scan-rate",
> &rtas_event_scan_rate);
> +       if (err) {
>                 printk(KERN_ERR "rtasd: no rtas-event-scan-rate on
> system\n");
>                 return -ENODEV;
>         }

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()

[Nathan Lynch]

rtas_os_term() is called during panic. Its behavior depends on a
couple of conditions in the /rtas node of the device tree, the
traversal of which entails locking and local IRQ state changes. If the
kernel panics while devtree_lock is held, rtas_os_term() as currently
written could hang.

Instead of discovering the relevant characteristics at panic time,
cache them in file-static variables at boot. Note the lookup for
"ibm,extended-os-term" is converted to of_property_read_bool() since
it is a boolean property, not a RTAS function token.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index c12dd5ed5e00..81e4996012b7 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -947,6 +947,8 @@ void __noreturn rtas_halt(void)
 
 /* Must be in the RMO region, so we place it here */
 static char rtas_os_term_buf[2048];
+static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE;
+static bool ibm_extended_os_term;
 
 void rtas_os_term(char *str)
 {
@@ -958,14 +960,13 @@ void rtas_os_term(char *str)
 	 * this property may terminate the partition which we want to avoid
 	 * since it interferes with panic_timeout.
 	 */
-	if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") ||
-	    RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os-term"))
+	if (ibm_os_term_token == RTAS_UNKNOWN_SERVICE || !ibm_extended_os_term)
 		return;
 
 	snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
 
 	do {
-		status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL,
+		status = rtas_call(ibm_os_term_token, 1, 1, NULL,
 				   __pa(rtas_os_term_buf));
 	} while (rtas_busy_delay(status));
 
@@ -1335,6 +1336,13 @@ void __init rtas_initialize(void)
 	no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry);
 	rtas.entry = no_entry ? rtas.base : entry;
 
+	/*
+	 * Discover these now to avoid device tree lookups in the
+	 * panic path.
+	 */
+	ibm_os_term_token = rtas_token("ibm,os-term");
+	ibm_extended_os_term = of_property_read_bool(rtas.dev, "ibm,extended-os-term");
+
 	/* If RTAS was found, allocate the RMO buffer for it and look for
 	 * the stop-self token if any
 	 */
-- 
2.37.1

Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> rtas_os_term() is called during panic. Its behavior depends on a
> couple of conditions in the /rtas node of the device tree, the
> traversal of which entails locking and local IRQ state changes. If
> the
> kernel panics while devtree_lock is held, rtas_os_term() as currently
> written could hang.
> 
> Instead of discovering the relevant characteristics at panic time,
> cache them in file-static variables at boot. Note the lookup for
> "ibm,extended-os-term" is converted to of_property_read_bool() since
> it is a boolean property, not a RTAS function token.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

This seems sensible, minor comment below.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/kernel/rtas.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index c12dd5ed5e00..81e4996012b7 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void)
>  
>  /* Must be in the RMO region, so we place it here */
>  static char rtas_os_term_buf[2048];
> +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE;

s32 and int are obviously identical, but rtas_token is declared using
int, as are the other variables where we cache various tokens.

> +static bool ibm_extended_os_term;
>  
>  void rtas_os_term(char *str)
>  {
> @@ -958,14 +960,13 @@ void rtas_os_term(char *str)
>          * this property may terminate the partition which we want to
> avoid
>          * since it interferes with panic_timeout.
>          */
> -       if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") ||
> -           RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os-
> term"))
> +       if (ibm_os_term_token == RTAS_UNKNOWN_SERVICE ||
> !ibm_extended_os_term)
>                 return;
>  
>         snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
>  
>         do {
> -               status = rtas_call(rtas_token("ibm,os-term"), 1, 1,
> NULL,
> +               status = rtas_call(ibm_os_term_token, 1, 1, NULL,
>                                    __pa(rtas_os_term_buf));
>         } while (rtas_busy_delay(status));
>  
> @@ -1335,6 +1336,13 @@ void __init rtas_initialize(void)
>         no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry",
> &entry);
>         rtas.entry = no_entry ? rtas.base : entry;
>  
> +       /*
> +        * Discover these now to avoid device tree lookups in the
> +        * panic path.
> +        */
> +       ibm_os_term_token = rtas_token("ibm,os-term");
> +       ibm_extended_os_term = of_property_read_bool(rtas.dev,
> "ibm,extended-os-term");
> +
>         /* If RTAS was found, allocate the RMO buffer for it and look
> for
>          * the stop-self token if any
>          */

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()

[Nathan Lynch]

Andrew Donnellan <ajd@linux.ibm.com> writes:
>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>> index c12dd5ed5e00..81e4996012b7 100644
>> --- a/arch/powerpc/kernel/rtas.c
>> +++ b/arch/powerpc/kernel/rtas.c
>> @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void)
>>  
>>  /* Must be in the RMO region, so we place it here */
>>  static char rtas_os_term_buf[2048];
>> +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE;
>
> s32 and int are obviously identical, but rtas_token is declared using
> int, as are the other variables where we cache various tokens.

Right... I think it's better practice to use an explicitly sized type
where the data is directly derived from the device tree and ultimately
passed to the firmware call interface. Gradually enacting this while
tolerating some cosmetic inconsistency in the code seems OK to me, but
I'm open to other opinions.

Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()

[Nicholas Piggin]

On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
> rtas_os_term() is called during panic. Its behavior depends on a
> couple of conditions in the /rtas node of the device tree, the
> traversal of which entails locking and local IRQ state changes. If the
> kernel panics while devtree_lock is held, rtas_os_term() as currently
> written could hang.

Nice.

>
> Instead of discovering the relevant characteristics at panic time,
> cache them in file-static variables at boot. Note the lookup for
> "ibm,extended-os-term" is converted to of_property_read_bool() since
> it is a boolean property, not a RTAS function token.

Small nit, but you could do that at the query site unless you
were going to start using ibm,os-term without the extended
capability.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/kernel/rtas.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index c12dd5ed5e00..81e4996012b7 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void)
>  
>  /* Must be in the RMO region, so we place it here */
>  static char rtas_os_term_buf[2048];
> +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE;
> +static bool ibm_extended_os_term;
>  
>  void rtas_os_term(char *str)
>  {
> @@ -958,14 +960,13 @@ void rtas_os_term(char *str)
>  	 * this property may terminate the partition which we want to avoid
>  	 * since it interferes with panic_timeout.
>  	 */
> -	if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") ||
> -	    RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os-term"))
> +	if (ibm_os_term_token == RTAS_UNKNOWN_SERVICE || !ibm_extended_os_term)
>  		return;
>  
>  	snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
>  
>  	do {
> -		status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL,
> +		status = rtas_call(ibm_os_term_token, 1, 1, NULL,
>  				   __pa(rtas_os_term_buf));
>  	} while (rtas_busy_delay(status));
>  
> @@ -1335,6 +1336,13 @@ void __init rtas_initialize(void)
>  	no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry);
>  	rtas.entry = no_entry ? rtas.base : entry;
>  
> +	/*
> +	 * Discover these now to avoid device tree lookups in the
> +	 * panic path.
> +	 */
> +	ibm_os_term_token = rtas_token("ibm,os-term");
> +	ibm_extended_os_term = of_property_read_bool(rtas.dev, "ibm,extended-os-term");
> +
>  	/* If RTAS was found, allocate the RMO buffer for it and look for
>  	 * the stop-self token if any
>  	 */
> -- 
> 2.37.1

Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()

[Nathan Lynch]

"Nicholas Piggin" <npiggin@gmail.com> writes:
> On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
>> rtas_os_term() is called during panic. Its behavior depends on a
>> couple of conditions in the /rtas node of the device tree, the
>> traversal of which entails locking and local IRQ state changes. If the
>> kernel panics while devtree_lock is held, rtas_os_term() as currently
>> written could hang.
>
> Nice.
>
>>
>> Instead of discovering the relevant characteristics at panic time,
>> cache them in file-static variables at boot. Note the lookup for
>> "ibm,extended-os-term" is converted to of_property_read_bool() since
>> it is a boolean property, not a RTAS function token.
>
> Small nit, but you could do that at the query site unless you
> were going to start using ibm,os-term without the extended
> capability.

I'm unsure that this is what you're suggesting, but we don't want to use
of_property_read_bool() in this context either, because it has the same
undesirable qualities as rtas_token().

> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks!

Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()

[Nicholas Piggin]

On Tue Nov 29, 2022 at 4:26 AM AEST, Nathan Lynch wrote:
> "Nicholas Piggin" <npiggin@gmail.com> writes:
> > On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
> >> rtas_os_term() is called during panic. Its behavior depends on a
> >> couple of conditions in the /rtas node of the device tree, the
> >> traversal of which entails locking and local IRQ state changes. If the
> >> kernel panics while devtree_lock is held, rtas_os_term() as currently
> >> written could hang.
> >
> > Nice.
> >
> >>
> >> Instead of discovering the relevant characteristics at panic time,
> >> cache them in file-static variables at boot. Note the lookup for
> >> "ibm,extended-os-term" is converted to of_property_read_bool() since
> >> it is a boolean property, not a RTAS function token.
> >
> > Small nit, but you could do that at the query site unless you
> > were going to start using ibm,os-term without the extended
> > capability.
>
> I'm unsure that this is what you're suggesting, but we don't want to use
> of_property_read_bool() in this context either, because it has the same
> undesirable qualities as rtas_token().

I mean rtas_initialize() could do

	if (of_property_read_bool(rtas.dev, "ibm,extended-os-term"))
		ibm_os_term_token = rtas_token("ibm,os-term");

Thanks,
Nick

Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()

[Nathan Lynch]

"Nicholas Piggin" <npiggin@gmail.com> writes:
> On Tue Nov 29, 2022 at 4:26 AM AEST, Nathan Lynch wrote:
>> "Nicholas Piggin" <npiggin@gmail.com> writes:
>> > On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
>> >> rtas_os_term() is called during panic. Its behavior depends on a
>> >> couple of conditions in the /rtas node of the device tree, the
>> >> traversal of which entails locking and local IRQ state changes. If the
>> >> kernel panics while devtree_lock is held, rtas_os_term() as currently
>> >> written could hang.
>> >
>> > Nice.
>> >
>> >>
>> >> Instead of discovering the relevant characteristics at panic time,
>> >> cache them in file-static variables at boot. Note the lookup for
>> >> "ibm,extended-os-term" is converted to of_property_read_bool() since
>> >> it is a boolean property, not a RTAS function token.
>> >
>> > Small nit, but you could do that at the query site unless you
>> > were going to start using ibm,os-term without the extended
>> > capability.
>>
>> I'm unsure that this is what you're suggesting, but we don't want to use
>> of_property_read_bool() in this context either, because it has the same
>> undesirable qualities as rtas_token().
>
> I mean rtas_initialize() could do
>
> 	if (of_property_read_bool(rtas.dev, "ibm,extended-os-term"))
> 		ibm_os_term_token = rtas_token("ibm,os-term");

Oh of course, thanks. Since I need to do a v2 anyway, I'll make that
change.

[PATCH 04/13] powerpc/rtas: avoid scheduling in rtas_os_term()

[Nathan Lynch]

It's unsafe to use rtas_busy_delay() to handle a busy status from
the ibm,os-term RTAS function in rtas_os_term():

Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
BUG: sleeping function called from invalid context at arch/powerpc/kernel/rtas.c:618
in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1, name: swapper/0
preempt_count: 2, expected: 0
CPU: 7 PID: 1 Comm: swapper/0 Tainted: G      D            6.0.0-rc5-02182-gf8553a572277-dirty #9
Call Trace:
[c000000007b8f000] [c000000001337110] dump_stack_lvl+0xb4/0x110 (unreliable)
[c000000007b8f040] [c0000000002440e4] __might_resched+0x394/0x3c0
[c000000007b8f0e0] [c00000000004f680] rtas_busy_delay+0x120/0x1b0
[c000000007b8f100] [c000000000052d04] rtas_os_term+0xb8/0xf4
[c000000007b8f180] [c0000000001150fc] pseries_panic+0x50/0x68
[c000000007b8f1f0] [c000000000036354] ppc_panic_platform_handler+0x34/0x50
[c000000007b8f210] [c0000000002303c4] notifier_call_chain+0xd4/0x1c0
[c000000007b8f2b0] [c0000000002306cc] atomic_notifier_call_chain+0xac/0x1c0
[c000000007b8f2f0] [c0000000001d62b8] panic+0x228/0x4d0
[c000000007b8f390] [c0000000001e573c] do_exit+0x140c/0x1420
[c000000007b8f480] [c0000000001e586c] make_task_dead+0xdc/0x200

Use rtas_busy_delay_time() instead, which signals without side effects
whether to attempt the ibm,os-term RTAS call again.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 81e4996012b7..51f0508593a7 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -965,10 +965,15 @@ void rtas_os_term(char *str)
 
 	snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
 
+	/*
+	 * Keep calling as long as RTAS returns a "try again" status,
+	 * but don't use rtas_busy_delay(), which potentially
+	 * schedules.
+	 */
 	do {
 		status = rtas_call(ibm_os_term_token, 1, 1, NULL,
 				   __pa(rtas_os_term_buf));
-	} while (rtas_busy_delay(status));
+	} while (rtas_busy_delay_time(status));
 
 	if (status != 0)
 		printk(KERN_EMERG "ibm,os-term call failed %d\n", status);
-- 
2.37.1

Re: [PATCH 04/13] powerpc/rtas: avoid scheduling in rtas_os_term()

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> It's unsafe to use rtas_busy_delay() to handle a busy status from
> the ibm,os-term RTAS function in rtas_os_term():
> 
> Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x0000000b
> BUG: sleeping function called from invalid context at
> arch/powerpc/kernel/rtas.c:618
> in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1, name:
> swapper/0
> preempt_count: 2, expected: 0
> CPU: 7 PID: 1 Comm: swapper/0 Tainted: G      D            6.0.0-rc5-
> 02182-gf8553a572277-dirty #9
> Call Trace:
> [c000000007b8f000] [c000000001337110] dump_stack_lvl+0xb4/0x110
> (unreliable)
> [c000000007b8f040] [c0000000002440e4] __might_resched+0x394/0x3c0
> [c000000007b8f0e0] [c00000000004f680] rtas_busy_delay+0x120/0x1b0
> [c000000007b8f100] [c000000000052d04] rtas_os_term+0xb8/0xf4
> [c000000007b8f180] [c0000000001150fc] pseries_panic+0x50/0x68
> [c000000007b8f1f0] [c000000000036354]
> ppc_panic_platform_handler+0x34/0x50
> [c000000007b8f210] [c0000000002303c4] notifier_call_chain+0xd4/0x1c0
> [c000000007b8f2b0] [c0000000002306cc]
> atomic_notifier_call_chain+0xac/0x1c0
> [c000000007b8f2f0] [c0000000001d62b8] panic+0x228/0x4d0
> [c000000007b8f390] [c0000000001e573c] do_exit+0x140c/0x1420
> [c000000007b8f480] [c0000000001e586c] make_task_dead+0xdc/0x200
> 
> Use rtas_busy_delay_time() instead, which signals without side
> effects
> whether to attempt the ibm,os-term RTAS call again.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Makes sense.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/kernel/rtas.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 81e4996012b7..51f0508593a7 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -965,10 +965,15 @@ void rtas_os_term(char *str)
>  
>         snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
>  
> +       /*
> +        * Keep calling as long as RTAS returns a "try again" status,
> +        * but don't use rtas_busy_delay(), which potentially
> +        * schedules.
> +        */
>         do {
>                 status = rtas_call(ibm_os_term_token, 1, 1, NULL,
>                                    __pa(rtas_os_term_buf));
> -       } while (rtas_busy_delay(status));
> +       } while (rtas_busy_delay_time(status));
>  
>         if (status != 0)
>                 printk(KERN_EMERG "ibm,os-term call failed %d\n",
> status);

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 04/13] powerpc/rtas: avoid scheduling in rtas_os_term()

[Nicholas Piggin]

On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
> It's unsafe to use rtas_busy_delay() to handle a busy status from
> the ibm,os-term RTAS function in rtas_os_term():
>
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> BUG: sleeping function called from invalid context at arch/powerpc/kernel/rtas.c:618
> in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1, name: swapper/0
> preempt_count: 2, expected: 0
> CPU: 7 PID: 1 Comm: swapper/0 Tainted: G      D            6.0.0-rc5-02182-gf8553a572277-dirty #9
> Call Trace:
> [c000000007b8f000] [c000000001337110] dump_stack_lvl+0xb4/0x110 (unreliable)
> [c000000007b8f040] [c0000000002440e4] __might_resched+0x394/0x3c0
> [c000000007b8f0e0] [c00000000004f680] rtas_busy_delay+0x120/0x1b0
> [c000000007b8f100] [c000000000052d04] rtas_os_term+0xb8/0xf4
> [c000000007b8f180] [c0000000001150fc] pseries_panic+0x50/0x68
> [c000000007b8f1f0] [c000000000036354] ppc_panic_platform_handler+0x34/0x50
> [c000000007b8f210] [c0000000002303c4] notifier_call_chain+0xd4/0x1c0
> [c000000007b8f2b0] [c0000000002306cc] atomic_notifier_call_chain+0xac/0x1c0
> [c000000007b8f2f0] [c0000000001d62b8] panic+0x228/0x4d0
> [c000000007b8f390] [c0000000001e573c] do_exit+0x140c/0x1420
> [c000000007b8f480] [c0000000001e586c] make_task_dead+0xdc/0x200
>
> Use rtas_busy_delay_time() instead, which signals without side effects
> whether to attempt the ibm,os-term RTAS call again.

rtas_busy_delay should probably be renamed to rtas_busy_sleep, to make
that self-documenting that it can schedule. You could then add a
rtas_busy_delay which doesn't sleep, which a few other places could
use...

But that's a bigger chance and there is precedent for using this call
this way, so looks okay to me. Maybe you could open-code an mdelay
though, although I guess firmware should be tolerant of calling it in
a loop.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/kernel/rtas.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 81e4996012b7..51f0508593a7 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -965,10 +965,15 @@ void rtas_os_term(char *str)
>  
>  	snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
>  
> +	/*
> +	 * Keep calling as long as RTAS returns a "try again" status,
> +	 * but don't use rtas_busy_delay(), which potentially
> +	 * schedules.
> +	 */
>  	do {
>  		status = rtas_call(ibm_os_term_token, 1, 1, NULL,
>  				   __pa(rtas_os_term_buf));
> -	} while (rtas_busy_delay(status));
> +	} while (rtas_busy_delay_time(status));
>  
>  	if (status != 0)
>  		printk(KERN_EMERG "ibm,os-term call failed %d\n", status);
> -- 
> 2.37.1

[PATCH 05/13] powerpc/pseries/eeh: use correct API for error log size

[Nathan Lynch]

rtas-error-log-max is not the name of an RTAS function, so
rtas_token() is not the appropriate API for retrieving its value. We
already have rtas_get_error_log_max() which returns a sensible value
if the property is absent for any reason, so use that instead.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
Fixes: 8d633291b4fc ("powerpc/eeh: pseries platform EEH error log retrieval")
---
 arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 8e40ccac0f44..e5e4f4aa5afd 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -848,7 +848,7 @@ static int __init eeh_pseries_init(void)
 	}
 
 	/* Initialize error log size */
-	eeh_error_buf_size = rtas_token("rtas-error-log-max");
+	eeh_error_buf_size = rtas_get_error_log_max();
 	if (eeh_error_buf_size == RTAS_UNKNOWN_SERVICE) {
 		pr_info("%s: unknown EEH error log size\n",
 			__func__);
-- 
2.37.1

Re: [PATCH 05/13] powerpc/pseries/eeh: use correct API for error log size

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> rtas-error-log-max is not the name of an RTAS function, so
> rtas_token() is not the appropriate API for retrieving its value. We
> already have rtas_get_error_log_max() which returns a sensible value
> if the property is absent for any reason, so use that instead.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> Fixes: 8d633291b4fc ("powerpc/eeh: pseries platform EEH error log
> retrieval")
> ---
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c
> b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 8e40ccac0f44..e5e4f4aa5afd 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -848,7 +848,7 @@ static int __init eeh_pseries_init(void)
>         }
>  
>         /* Initialize error log size */
> -       eeh_error_buf_size = rtas_token("rtas-error-log-max");
> +       eeh_error_buf_size = rtas_get_error_log_max();
>         if (eeh_error_buf_size == RTAS_UNKNOWN_SERVICE) {

This is now impossible, and the whole block makes little sense after
the next patch

>                 pr_info("%s: unknown EEH error log size\n",
>                         __func__);

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 05/13] powerpc/pseries/eeh: use correct API for error log size

[Nathan Lynch]

Andrew Donnellan <ajd@linux.ibm.com> writes:
> On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
>> rtas-error-log-max is not the name of an RTAS function, so
>> rtas_token() is not the appropriate API for retrieving its value. We
>> already have rtas_get_error_log_max() which returns a sensible value
>> if the property is absent for any reason, so use that instead.
>> 
>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>> Fixes: 8d633291b4fc ("powerpc/eeh: pseries platform EEH error log
>> retrieval")
>> ---
>>  arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c
>> b/arch/powerpc/platforms/pseries/eeh_pseries.c
>> index 8e40ccac0f44..e5e4f4aa5afd 100644
>> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
>> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
>> @@ -848,7 +848,7 @@ static int __init eeh_pseries_init(void)
>>         }
>>  
>>         /* Initialize error log size */
>> -       eeh_error_buf_size = rtas_token("rtas-error-log-max");
>> +       eeh_error_buf_size = rtas_get_error_log_max();
>>         if (eeh_error_buf_size == RTAS_UNKNOWN_SERVICE) {
>
> This is now impossible, and the whole block makes little sense after
> the next patch

Indeed, will fix in v2, thanks.

[PATCH 06/13] powerpc/rtas: clean up rtas_error_log_max initialization

[Nathan Lynch]

The code in rtas_get_error_log_max() doesn't cause problems in
practice, but there are no measures to ensure that the lazy
initialization of the static rtas_error_log_max variable is atomic,
and it's not worth adding them.

Initialize the static rtas_error_log_max variable at boot when we're
single-threaded instead of lazily on first use. Use the more
appropriate of_property_read_u32() API instead of rtas_token() to
consult the "rtas-error-log-max" property, which is not the name of an
RTAS function. Convert use of printk() to pr_warn() and distinguish
the possible error cases.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 51f0508593a7..3fa84c247415 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -353,6 +353,9 @@ int rtas_service_present(const char *service)
 EXPORT_SYMBOL(rtas_service_present);
 
 #ifdef CONFIG_RTAS_ERROR_LOGGING
+
+static u32 rtas_error_log_max __ro_after_init = RTAS_ERROR_LOG_MAX;
+
 /*
  * Return the firmware-specified size of the error log buffer
  *  for all rtas calls that require an error buffer argument.
@@ -360,21 +363,30 @@ EXPORT_SYMBOL(rtas_service_present);
  */
 int rtas_get_error_log_max(void)
 {
-	static int rtas_error_log_max;
-	if (rtas_error_log_max)
-		return rtas_error_log_max;
-
-	rtas_error_log_max = rtas_token ("rtas-error-log-max");
-	if ((rtas_error_log_max == RTAS_UNKNOWN_SERVICE) ||
-	    (rtas_error_log_max > RTAS_ERROR_LOG_MAX)) {
-		printk (KERN_WARNING "RTAS: bad log buffer size %d\n",
-			rtas_error_log_max);
-		rtas_error_log_max = RTAS_ERROR_LOG_MAX;
-	}
 	return rtas_error_log_max;
 }
 EXPORT_SYMBOL(rtas_get_error_log_max);
 
+static void __init init_error_log_max(void)
+{
+	static const char propname[] __initconst = "rtas-error-log-max";
+	u32 max;
+
+	if (of_property_read_u32(rtas.dev, propname, &max)) {
+		pr_warn("%s not found, using default of %u\n",
+			propname, RTAS_ERROR_LOG_MAX);
+		max = RTAS_ERROR_LOG_MAX;
+	}
+
+	if (max > RTAS_ERROR_LOG_MAX) {
+		pr_warn("%s = %u, clamping max error log size to %u\n",
+			propname, max, RTAS_ERROR_LOG_MAX);
+		max = RTAS_ERROR_LOG_MAX;
+	}
+
+	rtas_error_log_max = max;
+}
+
 
 static char rtas_err_buf[RTAS_ERROR_LOG_MAX];
 static int rtas_last_error_token;
@@ -432,6 +444,7 @@ static char *__fetch_rtas_last_error(char *altbuf)
 #else /* CONFIG_RTAS_ERROR_LOGGING */
 #define __fetch_rtas_last_error(x)	NULL
 #define get_errorlog_buffer()		NULL
+static void __init init_error_log_max(void) {}
 #endif
 
 
@@ -1341,6 +1354,8 @@ void __init rtas_initialize(void)
 	no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry);
 	rtas.entry = no_entry ? rtas.base : entry;
 
+	init_error_log_max();
+
 	/*
 	 * Discover these now to avoid device tree lookups in the
 	 * panic path.
-- 
2.37.1

Re: [PATCH 06/13] powerpc/rtas: clean up rtas_error_log_max initialization

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> The code in rtas_get_error_log_max() doesn't cause problems in
> practice, but there are no measures to ensure that the lazy
> initialization of the static rtas_error_log_max variable is atomic,
> and it's not worth adding them.
> 
> Initialize the static rtas_error_log_max variable at boot when we're
> single-threaded instead of lazily on first use. Use the more
> appropriate of_property_read_u32() API instead of rtas_token() to
> consult the "rtas-error-log-max" property, which is not the name of
> an
> RTAS function. Convert use of printk() to pr_warn() and distinguish
> the possible error cases.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/kernel/rtas.c | 37 ++++++++++++++++++++++++++----------
> -
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 51f0508593a7..3fa84c247415 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -353,6 +353,9 @@ int rtas_service_present(const char *service)
>  EXPORT_SYMBOL(rtas_service_present);
>  
>  #ifdef CONFIG_RTAS_ERROR_LOGGING
> +
> +static u32 rtas_error_log_max __ro_after_init = RTAS_ERROR_LOG_MAX;
> +
>  /*
>   * Return the firmware-specified size of the error log buffer
>   *  for all rtas calls that require an error buffer argument.
> @@ -360,21 +363,30 @@ EXPORT_SYMBOL(rtas_service_present);
>   */
>  int rtas_get_error_log_max(void)
>  {
> -       static int rtas_error_log_max;
> -       if (rtas_error_log_max)
> -               return rtas_error_log_max;
> -
> -       rtas_error_log_max = rtas_token ("rtas-error-log-max");
> -       if ((rtas_error_log_max == RTAS_UNKNOWN_SERVICE) ||
> -           (rtas_error_log_max > RTAS_ERROR_LOG_MAX)) {
> -               printk (KERN_WARNING "RTAS: bad log buffer size
> %d\n",
> -                       rtas_error_log_max);
> -               rtas_error_log_max = RTAS_ERROR_LOG_MAX;
> -       }
>         return rtas_error_log_max;
>  }
>  EXPORT_SYMBOL(rtas_get_error_log_max);
>  
> +static void __init init_error_log_max(void)
> +{
> +       static const char propname[] __initconst = "rtas-error-log-
> max";
> +       u32 max;
> +
> +       if (of_property_read_u32(rtas.dev, propname, &max)) {
> +               pr_warn("%s not found, using default of %u\n",
> +                       propname, RTAS_ERROR_LOG_MAX);
> +               max = RTAS_ERROR_LOG_MAX;
> +       }
> +
> +       if (max > RTAS_ERROR_LOG_MAX) {
> +               pr_warn("%s = %u, clamping max error log size to
> %u\n",
> +                       propname, max, RTAS_ERROR_LOG_MAX);
> +               max = RTAS_ERROR_LOG_MAX;
> +       }
> +
> +       rtas_error_log_max = max;
> +}
> +
>  
>  static char rtas_err_buf[RTAS_ERROR_LOG_MAX];
>  static int rtas_last_error_token;
> @@ -432,6 +444,7 @@ static char *__fetch_rtas_last_error(char
> *altbuf)
>  #else /* CONFIG_RTAS_ERROR_LOGGING */
>  #define __fetch_rtas_last_error(x)     NULL
>  #define get_errorlog_buffer()          NULL
> +static void __init init_error_log_max(void) {}
>  #endif
>  
>  
> @@ -1341,6 +1354,8 @@ void __init rtas_initialize(void)
>         no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry",
> &entry);
>         rtas.entry = no_entry ? rtas.base : entry;
>  
> +       init_error_log_max();
> +
>         /*
>          * Discover these now to avoid device tree lookups in the
>          * panic path.

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH 07/13] powerpc/rtas: clean up includes

[Nathan Lynch]

rtas.c used to host complex code related to pseries-specific guest
migration and suspend, which used atomics, completions, hcalls, and
CPU hotplug APIs. That's all been deleted or moved, so remove the
include directives that have been rendered unnecessary. Sort the
remainder (with linux/ before asm/) to impose some order on where
future additions go.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 42 +++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 3fa84c247415..7a5812624e11 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -7,43 +7,33 @@
  * Copyright (C) 2001 IBM.
  */
 
-#include <linux/stdarg.h>
-#include <linux/kernel.h>
-#include <linux/types.h>
-#include <linux/spinlock.h>
-#include <linux/export.h>
-#include <linux/init.h>
 #include <linux/capability.h>
 #include <linux/delay.h>
-#include <linux/cpu.h>
-#include <linux/sched.h>
-#include <linux/smp.h>
-#include <linux/completion.h>
-#include <linux/cpumask.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
 #include <linux/memblock.h>
-#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
 #include <linux/reboot.h>
+#include <linux/sched.h>
 #include <linux/security.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/stdarg.h>
 #include <linux/syscalls.h>
-#include <linux/of.h>
-#include <linux/of_fdt.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
 
+#include <asm/delay.h>
+#include <asm/firmware.h>
 #include <asm/interrupt.h>
-#include <asm/rtas.h>
-#include <asm/hvcall.h>
 #include <asm/machdep.h>
-#include <asm/firmware.h>
+#include <asm/mmu.h>
 #include <asm/page.h>
-#include <asm/param.h>
-#include <asm/delay.h>
-#include <linux/uaccess.h>
-#include <asm/udbg.h>
-#include <asm/syscalls.h>
-#include <asm/smp.h>
-#include <linux/atomic.h>
+#include <asm/rtas.h>
 #include <asm/time.h>
-#include <asm/mmu.h>
-#include <asm/topology.h>
+#include <asm/udbg.h>
 
 /* This is here deliberately so it's only used in this file */
 void enter_rtas(unsigned long);
-- 
2.37.1

Re: [PATCH 07/13] powerpc/rtas: clean up includes

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> rtas.c used to host complex code related to pseries-specific guest
> migration and suspend, which used atomics, completions, hcalls, and
> CPU hotplug APIs. That's all been deleted or moved, so remove the
> include directives that have been rendered unnecessary. Sort the
> remainder (with linux/ before asm/) to impose some order on where
> future additions go.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Compiles for me.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/kernel/rtas.c | 42 +++++++++++++++---------------------
> --
>  1 file changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 3fa84c247415..7a5812624e11 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -7,43 +7,33 @@
>   * Copyright (C) 2001 IBM.
>   */
>  
> -#include <linux/stdarg.h>
> -#include <linux/kernel.h>
> -#include <linux/types.h>
> -#include <linux/spinlock.h>
> -#include <linux/export.h>
> -#include <linux/init.h>
>  #include <linux/capability.h>
>  #include <linux/delay.h>
> -#include <linux/cpu.h>
> -#include <linux/sched.h>
> -#include <linux/smp.h>
> -#include <linux/completion.h>
> -#include <linux/cpumask.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
>  #include <linux/memblock.h>
> -#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
>  #include <linux/reboot.h>
> +#include <linux/sched.h>
>  #include <linux/security.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/stdarg.h>
>  #include <linux/syscalls.h>
> -#include <linux/of.h>
> -#include <linux/of_fdt.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
>  
> +#include <asm/delay.h>
> +#include <asm/firmware.h>
>  #include <asm/interrupt.h>
> -#include <asm/rtas.h>
> -#include <asm/hvcall.h>
>  #include <asm/machdep.h>
> -#include <asm/firmware.h>
> +#include <asm/mmu.h>
>  #include <asm/page.h>
> -#include <asm/param.h>
> -#include <asm/delay.h>
> -#include <linux/uaccess.h>
> -#include <asm/udbg.h>
> -#include <asm/syscalls.h>
> -#include <asm/smp.h>
> -#include <linux/atomic.h>
> +#include <asm/rtas.h>
>  #include <asm/time.h>
> -#include <asm/mmu.h>
> -#include <asm/topology.h>
> +#include <asm/udbg.h>
>  
>  /* This is here deliberately so it's only used in this file */
>  void enter_rtas(unsigned long);

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH 08/13] powerpc/rtas: define pr_fmt and convert printk call sites

[Nathan Lynch]

Set pr_fmt to "rtas: " and convert the handful of printk() uses in
rtas.c, adjusting the messages to remove now-redundant "RTAS"
strings.

Note that rtas_restart(), rtas_power_off(), and rtas_halt() all
currently use printk() without specifying a log level. These have been
changed to use pr_emerg(), which matches the behavior of
rtas_os_term().

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 7a5812624e11..c3142d352f41 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -7,6 +7,8 @@
  * Copyright (C) 2001 IBM.
  */
 
+#define pr_fmt(fmt)	"rtas: " fmt
+
 #include <linux/capability.h>
 #include <linux/delay.h>
 #include <linux/export.h>
@@ -718,8 +720,7 @@ static int rtas_error_rc(int rtas_rc)
 			rc = -ENODEV;
 			break;
 		default:
-			printk(KERN_ERR "%s: unexpected RTAS error %d\n",
-					__func__, rtas_rc);
+			pr_err("%s: unexpected error %d\n", __func__, rtas_rc);
 			rc = -ERANGE;
 			break;
 	}
@@ -923,8 +924,8 @@ void __noreturn rtas_restart(char *cmd)
 {
 	if (rtas_flash_term_hook)
 		rtas_flash_term_hook(SYS_RESTART);
-	printk("RTAS system-reboot returned %d\n",
-	       rtas_call(rtas_token("system-reboot"), 0, 1, NULL));
+	pr_emerg("system-reboot returned %d\n",
+		 rtas_call(rtas_token("system-reboot"), 0, 1, NULL));
 	for (;;);
 }
 
@@ -933,8 +934,8 @@ void rtas_power_off(void)
 	if (rtas_flash_term_hook)
 		rtas_flash_term_hook(SYS_POWER_OFF);
 	/* allow power on only with power button press */
-	printk("RTAS power-off returned %d\n",
-	       rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1));
+	pr_emerg("power-off returned %d\n",
+		 rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1));
 	for (;;);
 }
 
@@ -943,8 +944,8 @@ void __noreturn rtas_halt(void)
 	if (rtas_flash_term_hook)
 		rtas_flash_term_hook(SYS_HALT);
 	/* allow power on only with power button press */
-	printk("RTAS power-off returned %d\n",
-	       rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1));
+	pr_emerg("power-off returned %d\n",
+		 rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1));
 	for (;;);
 }
 
@@ -979,7 +980,7 @@ void rtas_os_term(char *str)
 	} while (rtas_busy_delay_time(status));
 
 	if (status != 0)
-		printk(KERN_EMERG "ibm,os-term call failed %d\n", status);
+		pr_emerg("ibm,os-term call failed %d\n", status);
 }
 
 /**
-- 
2.37.1

Re: [PATCH 08/13] powerpc/rtas: define pr_fmt and convert printk call sites

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> Set pr_fmt to "rtas: " and convert the handful of printk() uses in
> rtas.c, adjusting the messages to remove now-redundant "RTAS"
> strings.
> 
> Note that rtas_restart(), rtas_power_off(), and rtas_halt() all
> currently use printk() without specifying a log level. These have
> been
> changed to use pr_emerg(), which matches the behavior of
> rtas_os_term().
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/kernel/rtas.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 7a5812624e11..c3142d352f41 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -7,6 +7,8 @@
>   * Copyright (C) 2001 IBM.
>   */
>  
> +#define pr_fmt(fmt)    "rtas: " fmt
> +
>  #include <linux/capability.h>
>  #include <linux/delay.h>
>  #include <linux/export.h>
> @@ -718,8 +720,7 @@ static int rtas_error_rc(int rtas_rc)
>                         rc = -ENODEV;
>                         break;
>                 default:
> -                       printk(KERN_ERR "%s: unexpected RTAS error
> %d\n",
> -                                       __func__, rtas_rc);
> +                       pr_err("%s: unexpected error %d\n", __func__,
> rtas_rc);
>                         rc = -ERANGE;
>                         break;
>         }
> @@ -923,8 +924,8 @@ void __noreturn rtas_restart(char *cmd)
>  {
>         if (rtas_flash_term_hook)
>                 rtas_flash_term_hook(SYS_RESTART);
> -       printk("RTAS system-reboot returned %d\n",
> -              rtas_call(rtas_token("system-reboot"), 0, 1, NULL));
> +       pr_emerg("system-reboot returned %d\n",
> +                rtas_call(rtas_token("system-reboot"), 0, 1, NULL));
>         for (;;);
>  }
>  
> @@ -933,8 +934,8 @@ void rtas_power_off(void)
>         if (rtas_flash_term_hook)
>                 rtas_flash_term_hook(SYS_POWER_OFF);
>         /* allow power on only with power button press */
> -       printk("RTAS power-off returned %d\n",
> -              rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -
> 1));
> +       pr_emerg("power-off returned %d\n",
> +                rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -
> 1));
>         for (;;);
>  }
>  
> @@ -943,8 +944,8 @@ void __noreturn rtas_halt(void)
>         if (rtas_flash_term_hook)
>                 rtas_flash_term_hook(SYS_HALT);
>         /* allow power on only with power button press */
> -       printk("RTAS power-off returned %d\n",
> -              rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -
> 1));
> +       pr_emerg("power-off returned %d\n",
> +                rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -
> 1));
>         for (;;);
>  }
>  
> @@ -979,7 +980,7 @@ void rtas_os_term(char *str)
>         } while (rtas_busy_delay_time(status));
>  
>         if (status != 0)
> -               printk(KERN_EMERG "ibm,os-term call failed %d\n",
> status);
> +               pr_emerg("ibm,os-term call failed %d\n", status);
>  }
>  
>  /**

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH 09/13] powerpc/rtas: mandate RTAS syscall filtering

[Nathan Lynch]

CONFIG_PPC_RTAS_FILTER has been optional but default-enabled since its
introduction. It's been enabled in enterprise distro kernels for a
while without causing ABI breakage that wasn't easily fixed, and it
prevents harmful abuses of the rtas syscall.

Let's make it unconditional.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/Kconfig       | 13 -------------
 arch/powerpc/kernel/rtas.c | 16 ----------------
 2 files changed, 29 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2ca5418457ed..8092915a4e9b 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -1012,19 +1012,6 @@ config PPC_SECVAR_SYSFS
 	  read/write operations on these variables. Say Y if you have
 	  secure boot enabled and want to expose variables to userspace.
 
-config PPC_RTAS_FILTER
-	bool "Enable filtering of RTAS syscalls"
-	default y
-	depends on PPC_RTAS
-	help
-	  The RTAS syscall API has security issues that could be used to
-	  compromise system integrity. This option enforces restrictions on the
-	  RTAS calls and arguments passed by userspace programs to mitigate
-	  these issues.
-
-	  Say Y unless you know what you are doing and the filter is causing
-	  problems for you.
-
 endmenu
 
 config ISA_DMA_API
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index c3142d352f41..3929bcea92c0 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -1051,8 +1051,6 @@ noinstr struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log
 	return NULL;
 }
 
-#ifdef CONFIG_PPC_RTAS_FILTER
-
 /*
  * The sys_rtas syscall, as originally designed, allows root to pass
  * arbitrary physical addresses to RTAS calls. A number of RTAS calls
@@ -1201,20 +1199,6 @@ static void __init rtas_syscall_filter_init(void)
 		rtas_filters[i].token = rtas_token(rtas_filters[i].name);
 }
 
-#else
-
-static bool block_rtas_call(int token, int nargs,
-			    struct rtas_args *args)
-{
-	return false;
-}
-
-static void __init rtas_syscall_filter_init(void)
-{
-}
-
-#endif /* CONFIG_PPC_RTAS_FILTER */
-
 /* We assume to be passed big endian arguments */
 SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 {
-- 
2.37.1

Re: [PATCH 09/13] powerpc/rtas: mandate RTAS syscall filtering

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> CONFIG_PPC_RTAS_FILTER has been optional but default-enabled since
> its
> introduction. It's been enabled in enterprise distro kernels for a
> while without causing ABI breakage that wasn't easily fixed, and it
> prevents harmful abuses of the rtas syscall.
> 
> Let's make it unconditional.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Agreed.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/Kconfig       | 13 -------------
>  arch/powerpc/kernel/rtas.c | 16 ----------------
>  2 files changed, 29 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 2ca5418457ed..8092915a4e9b 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -1012,19 +1012,6 @@ config PPC_SECVAR_SYSFS
>           read/write operations on these variables. Say Y if you have
>           secure boot enabled and want to expose variables to
> userspace.
>  
> -config PPC_RTAS_FILTER
> -       bool "Enable filtering of RTAS syscalls"
> -       default y
> -       depends on PPC_RTAS
> -       help
> -         The RTAS syscall API has security issues that could be used
> to
> -         compromise system integrity. This option enforces
> restrictions on the
> -         RTAS calls and arguments passed by userspace programs to
> mitigate
> -         these issues.
> -
> -         Say Y unless you know what you are doing and the filter is
> causing
> -         problems for you.
> -
>  endmenu
>  
>  config ISA_DMA_API
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index c3142d352f41..3929bcea92c0 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -1051,8 +1051,6 @@ noinstr struct pseries_errorlog
> *get_pseries_errorlog(struct rtas_error_log *log
>         return NULL;
>  }
>  
> -#ifdef CONFIG_PPC_RTAS_FILTER
> -
>  /*
>   * The sys_rtas syscall, as originally designed, allows root to pass
>   * arbitrary physical addresses to RTAS calls. A number of RTAS
> calls
> @@ -1201,20 +1199,6 @@ static void __init
> rtas_syscall_filter_init(void)
>                 rtas_filters[i].token =
> rtas_token(rtas_filters[i].name);
>  }
>  
> -#else
> -
> -static bool block_rtas_call(int token, int nargs,
> -                           struct rtas_args *args)
> -{
> -       return false;
> -}
> -
> -static void __init rtas_syscall_filter_init(void)
> -{
> -}
> -
> -#endif /* CONFIG_PPC_RTAS_FILTER */
> -
>  /* We assume to be passed big endian arguments */
>  SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
>  {

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH 10/13] powerpc/rtas: improve function information lookups

[Nathan Lynch]

The core RTAS support code and its clients perform two types of lookup
for RTAS firmware function information.

First, mapping a known function name to a token. The typical use case
invokes rtas_token() to retrieve the token value to pass to
rtas_call(). rtas_token() relies on of_get_property(), which performs
a linear search of the /rtas node's property list under a lock with
IRQs disabled.

Second, and less common: given a token value, looking up some
information about the function. The primary example is the sys_rtas
filter path, which linearly scans a small table to match the token to
a rtas_filter struct. Another use case to come is RTAS entry/exit
tracepoints, which will require efficient lookup of function names
from token values. Currently there is no general API for this.

We need something much like the existing rtas_filters table, but more
general and organized to facilitate efficient lookups.

Introduce:

* A new rtas_function type, aggregating function name, token,
  and filter. Other function characteristics could be added in the
  future.

* An array of rtas_function, where each element corresponds to a known
  RTAS function. All information in the table is static save the token
  values, which are derived from the device tree at boot. The array is
  sorted by function name to allow binary search.

* A named constant for each known RTAS function, used to index the
  function array. These also will be used in a client-facing API to be
  added later.

* An xarray that maps valid tokens to rtas_function objects.

Fold the existing rtas_filter table into the new rtas_function array,
with the appropriate adjustments to block_rtas_call(). Remove
now-redundant fields from struct rtas_filter.

Convert rtas_token() to use a lockless binary search on the function
table. Fall back to the old behavior for lookups against names that
are not known to be RTAS functions, but issue a warning. rtas_token()
is for function names; it is not a general facility for accessing
arbitrary properties of the /rtas node. All known misuses of
rtas_token() have been converted to more appropriate of_ APIs in
preceding changes.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/include/asm/rtas.h |  87 ++++
 arch/powerpc/kernel/rtas.c      | 735 +++++++++++++++++++++++++++-----
 2 files changed, 709 insertions(+), 113 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 479a95cb2770..14fe79217c26 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -16,6 +16,93 @@
  * Copyright (C) 2001 PPC 64 Team, IBM Corp
  */
 
+#define rtas_fnidx(x_) RTAS_FNIDX__ ## x_
+
+enum rtas_function_index {
+	rtas_fnidx(CHECK_EXCEPTION),
+	rtas_fnidx(DISPLAY_CHARACTER),
+	rtas_fnidx(EVENT_SCAN),
+	rtas_fnidx(FREEZE_TIME_BASE),
+	rtas_fnidx(GET_POWER_LEVEL),
+	rtas_fnidx(GET_SENSOR_STATE),
+	rtas_fnidx(GET_TERM_CHAR),
+	rtas_fnidx(GET_TIME_OF_DAY),
+	rtas_fnidx(IBM_ACTIVATE_FIRMWARE),
+	rtas_fnidx(IBM_CBE_START_PTCAL),
+	rtas_fnidx(IBM_CBE_STOP_PTCAL),
+	rtas_fnidx(IBM_CHANGE_MSI),
+	rtas_fnidx(IBM_CLOSE_ERRINJCT),
+	rtas_fnidx(IBM_CONFIGURE_BRIDGE),
+	rtas_fnidx(IBM_CONFIGURE_CONNECTOR),
+	rtas_fnidx(IBM_CONFIGURE_KERNEL_DUMP),
+	rtas_fnidx(IBM_CONFIGURE_PE),
+	rtas_fnidx(IBM_CREATE_PE_DMA_WINDOW),
+	rtas_fnidx(IBM_DISPLAY_MESSAGE),
+	rtas_fnidx(IBM_ERRINJCT),
+	rtas_fnidx(IBM_EXTI2C),
+	rtas_fnidx(IBM_GET_CONFIG_ADDR_INFO),
+	rtas_fnidx(IBM_GET_CONFIG_ADDR_INFO2),
+	rtas_fnidx(IBM_GET_DYNAMIC_SENSOR_STATE),
+	rtas_fnidx(IBM_GET_INDICES),
+	rtas_fnidx(IBM_GET_RIO_TOPOLOGY),
+	rtas_fnidx(IBM_GET_SYSTEM_PARAMETER),
+	rtas_fnidx(IBM_GET_VPD),
+	rtas_fnidx(IBM_GET_XIVE),
+	rtas_fnidx(IBM_INT_OFF),
+	rtas_fnidx(IBM_INT_ON),
+	rtas_fnidx(IBM_IO_QUIESCE_ACK),
+	rtas_fnidx(IBM_LPAR_PERFTOOLS),
+	rtas_fnidx(IBM_MANAGE_FLASH_IMAGE),
+	rtas_fnidx(IBM_MANAGE_STORAGE_PRESERVATION),
+	rtas_fnidx(IBM_NMI_INTERLOCK),
+	rtas_fnidx(IBM_NMI_REGISTER),
+	rtas_fnidx(IBM_OPEN_ERRINJCT),
+	rtas_fnidx(IBM_OPEN_SRIOV_ALLOW_UNFREEZE),
+	rtas_fnidx(IBM_OPEN_SRIOV_MAP_PE_NUMBER),
+	rtas_fnidx(IBM_OS_TERM),
+	rtas_fnidx(IBM_PARTNER_CONTROL),
+	rtas_fnidx(IBM_PHYSICAL_ATTESTATION),
+	rtas_fnidx(IBM_PLATFORM_DUMP),
+	rtas_fnidx(IBM_POWER_OFF_UPS),
+	rtas_fnidx(IBM_QUERY_INTERRUPT_SOURCE_NUMBER),
+	rtas_fnidx(IBM_QUERY_PE_DMA_WINDOW),
+	rtas_fnidx(IBM_READ_PCI_CONFIG),
+	rtas_fnidx(IBM_READ_SLOT_RESET_STATE),
+	rtas_fnidx(IBM_READ_SLOT_RESET_STATE2),
+	rtas_fnidx(IBM_REMOVE_PE_DMA_WINDOW),
+	rtas_fnidx(IBM_RESET_PE_DMA_WINDOWS),
+	rtas_fnidx(IBM_SCAN_LOG_DUMP),
+	rtas_fnidx(IBM_SET_DYNAMIC_INDICATOR),
+	rtas_fnidx(IBM_SET_EEH_OPTION),
+	rtas_fnidx(IBM_SET_SLOT_RESET),
+	rtas_fnidx(IBM_SET_SYSTEM_PARAMETER),
+	rtas_fnidx(IBM_SET_XIVE),
+	rtas_fnidx(IBM_SLOT_ERROR_DETAIL),
+	rtas_fnidx(IBM_SUSPEND_ME),
+	rtas_fnidx(IBM_TUNE_DMA_PARMS),
+	rtas_fnidx(IBM_UPDATE_FLASH_64_AND_REBOOT),
+	rtas_fnidx(IBM_UPDATE_NODES),
+	rtas_fnidx(IBM_UPDATE_PROPERTIES),
+	rtas_fnidx(IBM_VALIDATE_FLASH_IMAGE),
+	rtas_fnidx(IBM_WRITE_PCI_CONFIG),
+	rtas_fnidx(NVRAM_FETCH),
+	rtas_fnidx(NVRAM_STORE),
+	rtas_fnidx(POWER_OFF),
+	rtas_fnidx(PUT_TERM_CHAR),
+	rtas_fnidx(QUERY_CPU_STOPPED_STATE),
+	rtas_fnidx(READ_PCI_CONFIG),
+	rtas_fnidx(RTAS_LAST_ERROR),
+	rtas_fnidx(SET_INDICATOR),
+	rtas_fnidx(SET_POWER_LEVEL),
+	rtas_fnidx(SET_TIME_FOR_POWER_ON),
+	rtas_fnidx(SET_TIME_OF_DAY),
+	rtas_fnidx(START_CPU),
+	rtas_fnidx(STOP_SELF),
+	rtas_fnidx(SYSTEM_REBOOT),
+	rtas_fnidx(THAW_TIME_BASE),
+	rtas_fnidx(WRITE_PCI_CONFIG),
+};
+
 #define RTAS_UNKNOWN_SERVICE (-1)
 #define RTAS_INSTANTIATE_MAX (1ULL<<30) /* Don't instantiate rtas at/above this value */
 
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 3929bcea92c0..a88db3b3486f 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -9,10 +9,12 @@
 
 #define pr_fmt(fmt)	"rtas: " fmt
 
+#include <linux/bsearch.h>
 #include <linux/capability.h>
 #include <linux/delay.h>
 #include <linux/export.h>
 #include <linux/init.h>
+#include <linux/kconfig.h>
 #include <linux/kernel.h>
 #include <linux/memblock.h>
 #include <linux/of.h>
@@ -26,6 +28,7 @@
 #include <linux/syscalls.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
+#include <linux/xarray.h>
 
 #include <asm/delay.h>
 #include <asm/firmware.h>
@@ -37,6 +40,485 @@
 #include <asm/time.h>
 #include <asm/udbg.h>
 
+enum rtas_function_flags {
+	RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE = (1 << 0),
+};
+
+struct rtas_filter {
+	/* Indexes into the args buffer, -1 if not used */
+	const int buf_idx1;
+	const int size_idx1;
+	const int buf_idx2;
+	const int size_idx2;
+	/*
+	 * Assumed buffer size per the spec if the function does not
+	 * have a size parameter, e.g. ibm,errinjct. 0 if unused.
+	 */
+	const int fixed_size;
+};
+
+/**
+ * struct rtas_function - Descriptor for RTAS functions.
+ *
+ * @token: value of @name if it exists under the /rtas node
+ * @name: function name
+ * @flags: properties of the function
+ * @filter: If non-NULL and allowed by @flags, invoking this function via the
+ *          rtas syscall is allowed, and @filter describes constraints on
+ *          the arguments.
+ */
+struct rtas_function {
+	s32 token;
+	const u16 flags;
+	const char * const name;
+	const struct rtas_filter *filter;
+};
+
+static struct rtas_function rtas_function_table[] __ro_after_init = {
+	[rtas_fnidx(CHECK_EXCEPTION)] = {
+		.name = "check-exception",
+	},
+	[rtas_fnidx(DISPLAY_CHARACTER)] = {
+		.name = "display-character",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(EVENT_SCAN)] = {
+		.name = "event-scan",
+	},
+	[rtas_fnidx(FREEZE_TIME_BASE)] = {
+		.name = "freeze-time-base",
+	},
+	[rtas_fnidx(GET_POWER_LEVEL)] = {
+		.name = "get-power-level",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(GET_SENSOR_STATE)] = {
+		.name = "get-sensor-state",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(GET_TERM_CHAR)] = {
+		.name = "get-term-char",
+	},
+	[rtas_fnidx(GET_TIME_OF_DAY)] = {
+		.name = "get-time-of-day",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_ACTIVATE_FIRMWARE)] = {
+		.name = "ibm,activate-firmware",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_CBE_START_PTCAL)] = {
+		.name = "ibm,cbe-start-ptcal",
+	},
+	[rtas_fnidx(IBM_CBE_STOP_PTCAL)] = {
+		.name = "ibm,cbe-stop-ptcal",
+	},
+	[rtas_fnidx(IBM_CHANGE_MSI)] = {
+		.name = "ibm,change-msi",
+	},
+	[rtas_fnidx(IBM_CLOSE_ERRINJCT)] = {
+		.name = "ibm,close-errinjct",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_CONFIGURE_BRIDGE)] = {
+		.name = "ibm,configure-bridge",
+	},
+	[rtas_fnidx(IBM_CONFIGURE_CONNECTOR)] = {
+		.name = "ibm,configure-connector",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 0, .size_idx1 = -1,
+			.buf_idx2 = 1, .size_idx2 = -1,
+			.fixed_size = 4096,
+		},
+	},
+	[rtas_fnidx(IBM_CONFIGURE_KERNEL_DUMP)] = {
+		.name = "ibm,configure-kernel-dump",
+	},
+	[rtas_fnidx(IBM_CONFIGURE_PE)] = {
+		.name = "ibm,configure-pe",
+	},
+	[rtas_fnidx(IBM_CREATE_PE_DMA_WINDOW)] = {
+		.name = "ibm,create-pe-dma-window",
+	},
+	[rtas_fnidx(IBM_DISPLAY_MESSAGE)] = {
+		.name = "ibm,display-message",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 0, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_ERRINJCT)] = {
+		.name = "ibm,errinjct",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 2, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+			.fixed_size = 1024,
+		},
+	},
+	[rtas_fnidx(IBM_EXTI2C)] = {
+		.name = "ibm,exti2c",
+	},
+	[rtas_fnidx(IBM_GET_CONFIG_ADDR_INFO)] = {
+		.name = "ibm,get-config-addr-info",
+	},
+	[rtas_fnidx(IBM_GET_CONFIG_ADDR_INFO2)] = {
+		.name = "ibm,get-config-addr-info2",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_GET_DYNAMIC_SENSOR_STATE)] = {
+		.name = "ibm,get-dynamic-sensor-state",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_GET_INDICES)] = {
+		.name = "ibm,get-indices",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 2, .size_idx1 = 3,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_GET_RIO_TOPOLOGY)] = {
+		.name = "ibm,get-rio-topology",
+	},
+	[rtas_fnidx(IBM_GET_SYSTEM_PARAMETER)] = {
+		.name = "ibm,get-system-parameter",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 1, .size_idx1 = 2,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_GET_VPD)] = {
+		.name = "ibm,get-vpd",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 0, .size_idx1 = -1,
+			.buf_idx2 = 1, .size_idx2 = 2,
+		},
+	},
+	[rtas_fnidx(IBM_GET_XIVE)] = {
+		.name = "ibm,get-xive",
+	},
+	[rtas_fnidx(IBM_INT_OFF)] = {
+		.name = "ibm,int-off",
+	},
+	[rtas_fnidx(IBM_INT_ON)] = {
+		.name = "ibm,int-on",
+	},
+	[rtas_fnidx(IBM_IO_QUIESCE_ACK)] = {
+		.name = "ibm,io-quiesce-ack",
+	},
+	[rtas_fnidx(IBM_LPAR_PERFTOOLS)] = {
+		.name = "ibm,lpar-perftools",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 2, .size_idx1 = 3,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_MANAGE_FLASH_IMAGE)] = {
+		.name = "ibm,manage-flash-image",
+	},
+	[rtas_fnidx(IBM_MANAGE_STORAGE_PRESERVATION)] = {
+		.name = "ibm,manage-storage-preservation",
+	},
+	[rtas_fnidx(IBM_NMI_INTERLOCK)] = {
+		.name = "ibm,nmi-interlock",
+	},
+	[rtas_fnidx(IBM_NMI_REGISTER)] = {
+		.name = "ibm,nmi-register",
+	},
+	[rtas_fnidx(IBM_OPEN_ERRINJCT)] = {
+		.name = "ibm,open-errinjct",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_OPEN_SRIOV_ALLOW_UNFREEZE)] = {
+		.name = "ibm,open-sriov-allow-unfreeze",
+	},
+	[rtas_fnidx(IBM_OPEN_SRIOV_MAP_PE_NUMBER)] = {
+		.name = "ibm,open-sriov-map-pe-number",
+	},
+	[rtas_fnidx(IBM_OS_TERM)] = {
+		.name = "ibm,os-term",
+	},
+	[rtas_fnidx(IBM_PARTNER_CONTROL)] = {
+		.name = "ibm,partner-control",
+	},
+	[rtas_fnidx(IBM_PHYSICAL_ATTESTATION)] = {
+		.name = "ibm,physical-attestation",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 0, .size_idx1 = 1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_PLATFORM_DUMP)] = {
+		.name = "ibm,platform-dump",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 4, .size_idx1 = 5,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_POWER_OFF_UPS)] = {
+		.name = "ibm,power-off-ups",
+	},
+	[rtas_fnidx(IBM_QUERY_INTERRUPT_SOURCE_NUMBER)] = {
+		.name = "ibm,query-interrupt-source-number",
+	},
+	[rtas_fnidx(IBM_QUERY_PE_DMA_WINDOW)] = {
+		.name = "ibm,query-pe-dma-window",
+	},
+	[rtas_fnidx(IBM_READ_PCI_CONFIG)] = {
+		.name = "ibm,read-pci-config",
+	},
+	[rtas_fnidx(IBM_READ_SLOT_RESET_STATE)] = {
+		.name = "ibm,read-slot-reset-state",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_READ_SLOT_RESET_STATE2)] = {
+		.name = "ibm,read-slot-reset-state2",
+	},
+	[rtas_fnidx(IBM_REMOVE_PE_DMA_WINDOW)] = {
+		.name = "ibm,remove-pe-dma-window",
+	},
+	[rtas_fnidx(IBM_RESET_PE_DMA_WINDOWS)] = {
+		.name = "ibm,reset-pe-dma-windows",
+	},
+	[rtas_fnidx(IBM_SCAN_LOG_DUMP)] = {
+		.name = "ibm,scan-log-dump",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 0, .size_idx1 = 1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_SET_DYNAMIC_INDICATOR)] = {
+		.name = "ibm,set-dynamic-indicator",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 2, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_SET_EEH_OPTION)] = {
+		.name = "ibm,set-eeh-option",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_SET_SLOT_RESET)] = {
+		.name = "ibm,set-slot-reset",
+	},
+	[rtas_fnidx(IBM_SET_SYSTEM_PARAMETER)] = {
+		.name = "ibm,set-system-parameter",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_SET_XIVE)] = {
+		.name = "ibm,set-xive",
+	},
+	[rtas_fnidx(IBM_SLOT_ERROR_DETAIL)] = {
+		.name = "ibm,slot-error-detail",
+	},
+	[rtas_fnidx(IBM_SUSPEND_ME)] = {
+		.name = "ibm,suspend-me",
+		.flags = RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE,
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(IBM_TUNE_DMA_PARMS)] = {
+		.name = "ibm,tune-dma-parms",
+	},
+	[rtas_fnidx(IBM_UPDATE_FLASH_64_AND_REBOOT)] = {
+		.name = "ibm,update-flash-64-and-reboot",
+	},
+	[rtas_fnidx(IBM_UPDATE_NODES)] = {
+		.name = "ibm,update-nodes",
+		.flags = RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE,
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 0, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+			.fixed_size = 4096,
+		},
+	},
+	[rtas_fnidx(IBM_UPDATE_PROPERTIES)] = {
+		.name = "ibm,update-properties",
+		.flags = RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE,
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = 0, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+			.fixed_size = 4096,
+		},
+	},
+	[rtas_fnidx(IBM_VALIDATE_FLASH_IMAGE)] = {
+		.name = "ibm,validate-flash-image",
+	},
+	[rtas_fnidx(IBM_WRITE_PCI_CONFIG)] = {
+		.name = "ibm,write-pci-config",
+	},
+	[rtas_fnidx(NVRAM_FETCH)] = {
+		.name = "nvram-fetch",
+	},
+	[rtas_fnidx(NVRAM_STORE)] = {
+		.name = "nvram-store",
+	},
+	[rtas_fnidx(POWER_OFF)] = {
+		.name = "power-off",
+	},
+	[rtas_fnidx(PUT_TERM_CHAR)] = {
+		.name = "put-term-char",
+	},
+	[rtas_fnidx(QUERY_CPU_STOPPED_STATE)] = {
+		.name = "query-cpu-stopped-state",
+	},
+	[rtas_fnidx(READ_PCI_CONFIG)] = {
+		.name = "read-pci-config",
+	},
+	[rtas_fnidx(RTAS_LAST_ERROR)] = {
+		.name = "rtas-last-error",
+	},
+	[rtas_fnidx(SET_INDICATOR)] = {
+		.name = "set-indicator",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(SET_POWER_LEVEL)] = {
+		.name = "set-power-level",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(SET_TIME_FOR_POWER_ON)] = {
+		.name = "set-time-for-power-on",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(SET_TIME_OF_DAY)] = {
+		.name = "set-time-of-day",
+		.filter = &(const struct rtas_filter) {
+			.buf_idx1 = -1, .size_idx1 = -1,
+			.buf_idx2 = -1, .size_idx2 = -1,
+		},
+	},
+	[rtas_fnidx(START_CPU)] = {
+		.name = "start-cpu",
+	},
+	[rtas_fnidx(STOP_SELF)] = {
+		.name = "stop-self",
+	},
+	[rtas_fnidx(SYSTEM_REBOOT)] = {
+		.name = "system-reboot",
+	},
+	[rtas_fnidx(THAW_TIME_BASE)] = {
+		.name = "thaw-time-base",
+	},
+	[rtas_fnidx(WRITE_PCI_CONFIG)] = {
+		.name = "write-pci-config",
+	},
+};
+
+static int rtas_function_cmp(const void *a, const void *b)
+{
+	const struct rtas_function *f1 = a;
+	const struct rtas_function *f2 = b;
+
+	return strcmp(f1->name, f2->name);
+}
+
+/*
+ * Boot-time initialization of the function table needs the lookup to
+ * return a non-const-qualified object. Use rtas_name_to_function()
+ * in all other contexts.
+ */
+static struct rtas_function *__rtas_name_to_function(const char *name)
+{
+	const struct rtas_function key = {
+		.name = name,
+	};
+	struct rtas_function *found;
+
+	found = bsearch(&key, rtas_function_table, ARRAY_SIZE(rtas_function_table),
+			sizeof(rtas_function_table[0]), rtas_function_cmp);
+
+	return found;
+}
+
+static const struct rtas_function *rtas_name_to_function(const char *name)
+{
+	return __rtas_name_to_function(name);
+}
+
+static DEFINE_XARRAY(rtas_token_to_function_xarray);
+
+static int __init rtas_token_to_function_xarray_init(void)
+{
+	int err = 0;
+
+	for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
+		const struct rtas_function *func = &rtas_function_table[i];
+		const s32 token = func->token;
+
+		if (token == RTAS_UNKNOWN_SERVICE)
+			continue;
+
+		err = xa_err(xa_store(&rtas_token_to_function_xarray,
+				      token, (void *)func, GFP_KERNEL));
+		if (err)
+			break;
+	}
+
+	return err;
+}
+arch_initcall(rtas_token_to_function_xarray_init);
+
+static const struct rtas_function *rtas_token_to_function(s32 token)
+{
+	const struct rtas_function *func;
+
+	if (WARN_ONCE(token < 0, "invalid token %d", token))
+		return NULL;
+
+	func = xa_load(&rtas_token_to_function_xarray, (unsigned long)token);
+
+	if (WARN_ONCE(!func, "unexpected failed lookup for token %d", token))
+		return NULL;
+
+	return func;
+}
+
 /* This is here deliberately so it's only used in this file */
 void enter_rtas(unsigned long);
 
@@ -330,9 +812,25 @@ EXPORT_SYMBOL(rtas_progress);		/* needed by rtas_flash module */
 
 int rtas_token(const char *service)
 {
+	const struct rtas_function *func;
 	const __be32 *tokp;
+
 	if (rtas.dev == NULL)
 		return RTAS_UNKNOWN_SERVICE;
+
+	func = rtas_name_to_function(service);
+	if (func)
+		return func->token;
+	/*
+	 * The caller is looking up a name that is not known to be an
+	 * RTAS function. Either it's a function that needs to be
+	 * added to the table, or they're misusing rtas_token() to
+	 * access non-function properties of the /rtas node. Warn and
+	 * fall back to the legacy behavior.
+	 */
+	WARN_ONCE(1, "unknown function `%s`, should it be added to rtas_function_table?\n",
+		  service);
+
 	tokp = of_get_property(rtas.dev, service, NULL);
 	return tokp ? be32_to_cpu(*tokp) : RTAS_UNKNOWN_SERVICE;
 }
@@ -1064,56 +1562,12 @@ noinstr struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log
  *
  * Accordingly, we filter RTAS requests to check that the call is
  * permitted, and that provided pointers fall within the RMO buffer.
- * The rtas_filters list contains an entry for each permitted call,
- * with the indexes of the parameters which are expected to contain
- * addresses and sizes of buffers allocated inside the RMO buffer.
+ * If a function is allowed to be invoked via the syscall, then its
+ * entry in the rtas_functions table points to a rtas_filter that
+ * describes its constraints, with the indexes of the parameters which
+ * are expected to contain addresses and sizes of buffers allocated
+ * inside the RMO buffer.
  */
-struct rtas_filter {
-	const char *name;
-	int token;
-	/* Indexes into the args buffer, -1 if not used */
-	int buf_idx1;
-	int size_idx1;
-	int buf_idx2;
-	int size_idx2;
-
-	int fixed_size;
-};
-
-static struct rtas_filter rtas_filters[] __ro_after_init = {
-	{ "ibm,activate-firmware", -1, -1, -1, -1, -1 },
-	{ "ibm,configure-connector", -1, 0, -1, 1, -1, 4096 },	/* Special cased */
-	{ "display-character", -1, -1, -1, -1, -1 },
-	{ "ibm,display-message", -1, 0, -1, -1, -1 },
-	{ "ibm,errinjct", -1, 2, -1, -1, -1, 1024 },
-	{ "ibm,close-errinjct", -1, -1, -1, -1, -1 },
-	{ "ibm,open-errinjct", -1, -1, -1, -1, -1 },
-	{ "ibm,get-config-addr-info2", -1, -1, -1, -1, -1 },
-	{ "ibm,get-dynamic-sensor-state", -1, 1, -1, -1, -1 },
-	{ "ibm,get-indices", -1, 2, 3, -1, -1 },
-	{ "get-power-level", -1, -1, -1, -1, -1 },
-	{ "get-sensor-state", -1, -1, -1, -1, -1 },
-	{ "ibm,get-system-parameter", -1, 1, 2, -1, -1 },
-	{ "get-time-of-day", -1, -1, -1, -1, -1 },
-	{ "ibm,get-vpd", -1, 0, -1, 1, 2 },
-	{ "ibm,lpar-perftools", -1, 2, 3, -1, -1 },
-	{ "ibm,platform-dump", -1, 4, 5, -1, -1 },		/* Special cased */
-	{ "ibm,read-slot-reset-state", -1, -1, -1, -1, -1 },
-	{ "ibm,scan-log-dump", -1, 0, 1, -1, -1 },
-	{ "ibm,set-dynamic-indicator", -1, 2, -1, -1, -1 },
-	{ "ibm,set-eeh-option", -1, -1, -1, -1, -1 },
-	{ "set-indicator", -1, -1, -1, -1, -1 },
-	{ "set-power-level", -1, -1, -1, -1, -1 },
-	{ "set-time-for-power-on", -1, -1, -1, -1, -1 },
-	{ "ibm,set-system-parameter", -1, 1, -1, -1, -1 },
-	{ "set-time-of-day", -1, -1, -1, -1, -1 },
-#ifdef CONFIG_CPU_BIG_ENDIAN
-	{ "ibm,suspend-me", -1, -1, -1, -1, -1 },
-	{ "ibm,update-nodes", -1, 0, -1, -1, -1, 4096 },
-	{ "ibm,update-properties", -1, 0, -1, -1, -1, 4096 },
-#endif
-	{ "ibm,physical-attestation", -1, 0, 1, -1, -1 },
-};
 
 static bool in_rmo_buf(u32 base, u32 end)
 {
@@ -1127,63 +1581,76 @@ static bool in_rmo_buf(u32 base, u32 end)
 static bool block_rtas_call(int token, int nargs,
 			    struct rtas_args *args)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++) {
-		struct rtas_filter *f = &rtas_filters[i];
-		u32 base, size, end;
-
-		if (token != f->token)
-			continue;
-
-		if (f->buf_idx1 != -1) {
-			base = be32_to_cpu(args->args[f->buf_idx1]);
-			if (f->size_idx1 != -1)
-				size = be32_to_cpu(args->args[f->size_idx1]);
-			else if (f->fixed_size)
-				size = f->fixed_size;
-			else
-				size = 1;
-
-			end = base + size - 1;
-
-			/*
-			 * Special case for ibm,platform-dump - NULL buffer
-			 * address is used to indicate end of dump processing
-			 */
-			if (!strcmp(f->name, "ibm,platform-dump") &&
-			    base == 0)
-				return false;
-
-			if (!in_rmo_buf(base, end))
-				goto err;
-		}
-
-		if (f->buf_idx2 != -1) {
-			base = be32_to_cpu(args->args[f->buf_idx2]);
-			if (f->size_idx2 != -1)
-				size = be32_to_cpu(args->args[f->size_idx2]);
-			else if (f->fixed_size)
-				size = f->fixed_size;
-			else
-				size = 1;
-			end = base + size - 1;
-
-			/*
-			 * Special case for ibm,configure-connector where the
-			 * address can be 0
-			 */
-			if (!strcmp(f->name, "ibm,configure-connector") &&
-			    base == 0)
-				return false;
-
-			if (!in_rmo_buf(base, end))
-				goto err;
-		}
-
-		return false;
+	const struct rtas_function *func;
+	const struct rtas_filter *f;
+	u32 base, size, end;
+
+	/*
+	 * If this token doesn't correspond to a function the kernel
+	 * understands, you're not allowed to call it.
+	 */
+	func = rtas_token_to_function(token);
+	if (!func)
+		goto err;
+	/*
+	 * And only functions with filters attached are allowed.
+	 */
+	f = func->filter;
+	if (!f)
+		goto err;
+	/*
+	 * And some functions aren't allowed on LE.
+	 */
+	if (IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) &&
+	    (func->flags & RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE))
+		goto err;
+
+	if (f->buf_idx1 != -1) {
+		base = be32_to_cpu(args->args[f->buf_idx1]);
+		if (f->size_idx1 != -1)
+			size = be32_to_cpu(args->args[f->size_idx1]);
+		else if (f->fixed_size)
+			size = f->fixed_size;
+		else
+			size = 1;
+
+		end = base + size - 1;
+
+		/*
+		 * Special case for ibm,platform-dump - NULL buffer
+		 * address is used to indicate end of dump processing
+		 */
+		if (!strcmp(func->name, "ibm,platform-dump") &&
+		    base == 0)
+			return false;
+
+		if (!in_rmo_buf(base, end))
+			goto err;
+	}
+
+	if (f->buf_idx2 != -1) {
+		base = be32_to_cpu(args->args[f->buf_idx2]);
+		if (f->size_idx2 != -1)
+			size = be32_to_cpu(args->args[f->size_idx2]);
+		else if (f->fixed_size)
+			size = f->fixed_size;
+		else
+			size = 1;
+		end = base + size - 1;
+
+		/*
+		 * Special case for ibm,configure-connector where the
+		 * address can be 0
+		 */
+		if (!strcmp(func->name, "ibm,configure-connector") &&
+		    base == 0)
+			return false;
+
+		if (!in_rmo_buf(base, end))
+			goto err;
 	}
 
+	return false;
 err:
 	pr_err_ratelimited("sys_rtas: RTAS call blocked - exploit attempt?\n");
 	pr_err_ratelimited("sys_rtas: token=0x%x, nargs=%d (called by %s)\n",
@@ -1191,14 +1658,6 @@ static bool block_rtas_call(int token, int nargs,
 	return true;
 }
 
-static void __init rtas_syscall_filter_init(void)
-{
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++)
-		rtas_filters[i].token = rtas_token(rtas_filters[i].name);
-}
-
 /* We assume to be passed big endian arguments */
 SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 {
@@ -1298,6 +1757,54 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 	return 0;
 }
 
+static void __init rtas_function_table_init(void)
+{
+	struct property *prop;
+
+	for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
+		struct rtas_function *curr = &rtas_function_table[i];
+		struct rtas_function *prior;
+		int cmp;
+
+		curr->token = RTAS_UNKNOWN_SERVICE;
+
+		if (i == 0)
+			continue;
+		/*
+		 * Ensure table is sorted correctly for binary search
+		 * on function names.
+		 */
+		prior = &rtas_function_table[i - 1];
+
+		cmp = strcmp(prior->name, curr->name);
+		if (cmp < 0)
+			continue;
+
+		if (cmp == 0) {
+			pr_err("'%s' has duplicate function table entries\n",
+			       curr->name);
+		} else {
+			pr_err("function table unsorted: '%s' wrongly precedes '%s'\n",
+			       prior->name, curr->name);
+		}
+	}
+
+	for_each_property_of_node(rtas.dev, prop) {
+		struct rtas_function *func;
+
+		if (prop->length != sizeof(u32))
+			continue;
+
+		func = __rtas_name_to_function(prop->name);
+		if (!func)
+			continue;
+
+		func->token = be32_to_cpup((__be32 *)prop->value);
+
+		pr_debug("function %s has token %u\n", func->name, func->token);
+	}
+}
+
 /*
  * Call early during boot, before mem init, to retrieve the RTAS
  * information from the device-tree and allocate the RMO buffer for userland
@@ -1331,6 +1838,9 @@ void __init rtas_initialize(void)
 
 	init_error_log_max();
 
+	/* Must be called before any function token lookups */
+	rtas_function_table_init();
+
 	/*
 	 * Discover these now to avoid device tree lookups in the
 	 * panic path.
@@ -1356,7 +1866,6 @@ void __init rtas_initialize(void)
 #endif
 	ibm_open_errinjct_token = rtas_token("ibm,open-errinjct");
 	ibm_errinjct_token = rtas_token("ibm,errinjct");
-	rtas_syscall_filter_init();
 }
 
 int __init early_init_dt_scan_rtas(unsigned long node,
-- 
2.37.1

Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> Convert rtas_token() to use a lockless binary search on the function
> table. Fall back to the old behavior for lookups against names that
> are not known to be RTAS functions, but issue a warning. rtas_token()
> is for function names; it is not a general facility for accessing
> arbitrary properties of the /rtas node. All known misuses of
> rtas_token() have been converted to more appropriate of_ APIs in
> preceding changes.

For in-kernel users, why not go all the way: make rtas_token() static
and use it purely for the userspace API, and switch kernel users over
to using rtas_function_index directly?

> +enum rtas_function_flags {
> +       RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE = (1 << 0),
> +};

This seems to be new, what's the justification?

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

[Nick Child]

On 11/22/22 20:51, Andrew Donnellan wrote:
> On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
>> +enum rtas_function_flags {
>> +       RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE = (1 << 0),
>> +};
> 
> This seems to be new, what's the justification?
> 

Seems to be a run-time replacement of:
#ifdef CONFIG_CPU_BIG_ENDIAN
	{ "ibm,suspend-me", -1, -1, -1, -1, -1 },
	{ "ibm,update-nodes", -1, 0, -1, -1, -1, 4096 },
	{ "ibm,update-properties", -1, 0, -1, -1, -1, 4096 },
#endif

It looks to be handled logically:
+ if (IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) &&
+	    (func->flags & RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE))
+		goto err;

Perhaps, also allow the addition of any future special cases
for rtas functions easier to maintain?

Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

[Andrew Donnellan]

On Wed, 2022-11-23 at 13:32 -0600, Nick Child wrote:
> On 11/22/22 20:51, Andrew Donnellan wrote:
> > On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> > > +enum rtas_function_flags {
> > > +       RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE = (1 << 0),
> > > +};
> > 
> > This seems to be new, what's the justification?
> > 
> 
> Seems to be a run-time replacement of:
> #ifdef CONFIG_CPU_BIG_ENDIAN
>         { "ibm,suspend-me", -1, -1, -1, -1, -1 },
>         { "ibm,update-nodes", -1, 0, -1, -1, -1, 4096 },
>         { "ibm,update-properties", -1, 0, -1, -1, -1, 4096 },
> #endif
> 
> It looks to be handled logically:
> + if (IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) &&
> +           (func->flags & RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE))
> +               goto err;
> 
> Perhaps, also allow the addition of any future special cases
> for rtas functions easier to maintain?

Makes sense, though I'm slightly confused about the original rationale
for the ifdef and why it's not being fixed in userspace.

Slightly clunky name though, something like
RTAS_FN_FLAG_SYSCALL_BE_ONLY might be less clunky?

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

[Nathan Lynch]

Andrew Donnellan <ajd@linux.ibm.com> writes:
> On Wed, 2022-11-23 at 13:32 -0600, Nick Child wrote:
>> On 11/22/22 20:51, Andrew Donnellan wrote:
>> > On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
>> > > +enum rtas_function_flags {
>> > > +       RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE = (1 << 0),
>> > > +};
>> > 
>> > This seems to be new, what's the justification?
>> > 
>> 
>> Seems to be a run-time replacement of:
>> #ifdef CONFIG_CPU_BIG_ENDIAN
>>         { "ibm,suspend-me", -1, -1, -1, -1, -1 },
>>         { "ibm,update-nodes", -1, 0, -1, -1, -1, 4096 },
>>         { "ibm,update-properties", -1, 0, -1, -1, -1, 4096 },
>> #endif
>> 
>> It looks to be handled logically:
>> + if (IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) &&
>> +           (func->flags & RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE))
>> +               goto err;
>> 
>> Perhaps, also allow the addition of any future special cases
>> for rtas functions easier to maintain?
>
> Makes sense, though I'm slightly confused about the original rationale
> for the ifdef and why it's not being fixed in userspace.

Nick C's explanation is correct. I will make the commit message more
explicit about the conversion, and document the flag in the code.

The original rationale:

commit de0f7349a0dd072e54b5fc04c305907b22d28a5f
Author: Nathan Lynch <nathanl@linux.ibm.com>
Date:   Mon Dec 7 15:51:33 2020 -0600

    powerpc/rtas: prevent suspend-related sys_rtas use on LE

    While drmgr has had work in some areas to make its RTAS syscall
    interactions endian-neutral, its code for performing partition
    migration via the syscall has never worked on LE. While it is able to
    complete ibm,suspend-me successfully, it crashes when attempting the
    subsequent ibm,update-nodes call.

    drmgr is the only known (or plausible) user of ibm,suspend-me,
    ibm,update-nodes, and ibm,update-properties, so allow them only in
    big-endian configurations.

To summarize: we know these functions have never had working users via
sys_rtas on ppc64le, and we want to keep it that way.

> Slightly clunky name though, something like
> RTAS_FN_FLAG_SYSCALL_BE_ONLY might be less clunky?

RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE is verbose, but I think it
communicates better that we are consciously imposing a policy in a
specific context.

Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

[Nathan Lynch]

Andrew Donnellan <ajd@linux.ibm.com> writes:
> On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
>> Convert rtas_token() to use a lockless binary search on the function
>> table. Fall back to the old behavior for lookups against names that
>> are not known to be RTAS functions, but issue a warning. rtas_token()
>> is for function names; it is not a general facility for accessing
>> arbitrary properties of the /rtas node. All known misuses of
>> rtas_token() have been converted to more appropriate of_ APIs in
>> preceding changes.
>
> For in-kernel users, why not go all the way: make rtas_token() static
> and use it purely for the userspace API,

Not sure what userspace API refers to here, can you be more specific? I
don't think rtas_token() is exposed to user space.

> and switch kernel users over
> to using rtas_function_index directly?

Well, I have work in progress to transition all rtas_token() users to a
different API, but using opaque symbolic handles rather than exposing
the indexes. Something like:

/*
 * Opaque handle for client code to refer to RTAS functions. All valid
 * function handles are build-time constants prefixed with RTAS_FN_.
 */
typedef struct {
	enum rtas_function_index index;
} rtas_fn_handle_t;

#define rtas_fn_handle(x_) (const rtas_fn_handle_t) { .index = rtas_fnidx(x_), }

#define RTAS_FN_CHECK_EXCEPTION                   rtas_fn_handle(CHECK_EXCEPTION)
#define RTAS_FN_DISPLAY_CHARACTER                 rtas_fn_handle(DISPLAY_CHARACTER)
#define RTAS_FN_EVENT_SCAN                        rtas_fn_handle(EVENT_SCAN)

...

/**
 * rtas_function_token() - RTAS function token lookup.
 * @handle: Function handle, e.g. RTAS_FN_EVENT_SCAN.
 *
 * Context: Any context.
 * Return: the token value for the function if implemented by this platform,
 *         otherwise RTAS_UNKNOWN_SERVICE.
 */
s32 rtas_function_token(const rtas_fn_handle_t handle)
{
	const size_t index = handle.index;
	const bool out_of_bounds = index >= ARRAY_SIZE(rtas_function_table);

	if (WARN_ONCE(out_of_bounds, "invalid function index %zu", index))
		return RTAS_UNKNOWN_SERVICE;

	return rtas_function_table[index].token;
}

But that's a tree-wide change that would touch various drivers, and I
feel like the current series is what I can reasonably hope to get
applied right now.

Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

[Andrew Donnellan]

On Mon, 2022-11-28 at 18:08 -0600, Nathan Lynch wrote:
> Andrew Donnellan <ajd@linux.ibm.com> writes:
> > On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> > > Convert rtas_token() to use a lockless binary search on the
> > > function
> > > table. Fall back to the old behavior for lookups against names
> > > that
> > > are not known to be RTAS functions, but issue a warning.
> > > rtas_token()
> > > is for function names; it is not a general facility for accessing
> > > arbitrary properties of the /rtas node. All known misuses of
> > > rtas_token() have been converted to more appropriate of_ APIs in
> > > preceding changes.
> > 
> > For in-kernel users, why not go all the way: make rtas_token()
> > static
> > and use it purely for the userspace API,
> 
> Not sure what userspace API refers to here, can you be more specific?
> I
> don't think rtas_token() is exposed to user space.

Right, what I actually meant to refer to here is the fact that sys_rtas
takes a token, but when I wrote this I forgot that userspace doesn't
pass a string, rather librtas implements rtas_token itself using the
/proc interface to the device tree.

> 
> > and switch kernel users over
> > to using rtas_function_index directly?
> 
> Well, I have work in progress to transition all rtas_token() users to
> a
> different API, but using opaque symbolic handles rather than exposing
> the indexes. Something like:
> 
> /*
>  * Opaque handle for client code to refer to RTAS functions. All
> valid
>  * function handles are build-time constants prefixed with RTAS_FN_.
>  */
> typedef struct {
>         enum rtas_function_index index;
> } rtas_fn_handle_t;
> 
> #define rtas_fn_handle(x_) (const rtas_fn_handle_t) { .index =
> rtas_fnidx(x_), }
> 
> #define RTAS_FN_CHECK_EXCEPTION                  
> rtas_fn_handle(CHECK_EXCEPTION)
> #define RTAS_FN_DISPLAY_CHARACTER                
> rtas_fn_handle(DISPLAY_CHARACTER)
> #define RTAS_FN_EVENT_SCAN                       
> rtas_fn_handle(EVENT_SCAN)
> 
> ...
> 
> /**
>  * rtas_function_token() - RTAS function token lookup.
>  * @handle: Function handle, e.g. RTAS_FN_EVENT_SCAN.
>  *
>  * Context: Any context.
>  * Return: the token value for the function if implemented by this
> platform,
>  *         otherwise RTAS_UNKNOWN_SERVICE.
>  */
> s32 rtas_function_token(const rtas_fn_handle_t handle)
> {
>         const size_t index = handle.index;
>         const bool out_of_bounds = index >=
> ARRAY_SIZE(rtas_function_table);
> 
>         if (WARN_ONCE(out_of_bounds, "invalid function index %zu",
> index))
>                 return RTAS_UNKNOWN_SERVICE;
> 
>         return rtas_function_table[index].token;
> }
> 
> But that's a tree-wide change that would touch various drivers, and I
> feel like the current series is what I can reasonably hope to get
> applied right now.

It's not clear to me what the benefit of adding this additional layer
of indirection would be.


-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

[Nathan Lynch]

Andrew Donnellan <ajd@linux.ibm.com> writes:
> On Mon, 2022-11-28 at 18:08 -0600, Nathan Lynch wrote:
>> Andrew Donnellan <ajd@linux.ibm.com> writes:
>> > On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
>> > > Convert rtas_token() to use a lockless binary search on the
>> > > function table. Fall back to the old behavior for lookups against
>> > > names that are not known to be RTAS functions, but issue a
>> > > warning.  rtas_token() is for function names; it is not a general
>> > > facility for accessing arbitrary properties of the /rtas
>> > > node. All known misuses of rtas_token() have been converted to
>> > > more appropriate of_ APIs in preceding changes.
>> > 
>> > For in-kernel users, why not go all the way: make rtas_token()
>> > static and use it purely for the userspace API, and switch kernel
>> > users over to using rtas_function_index directly?
>> 
>> Well, I have work in progress to transition all rtas_token() users to
>> a different API, but using opaque symbolic handles rather than
>> exposing the indexes. Something like:
>> 
>> /*
>>  * Opaque handle for client code to refer to RTAS functions. All
>> valid
>>  * function handles are build-time constants prefixed with RTAS_FN_.
>>  */
>> typedef struct {
>>         enum rtas_function_index index;
>> } rtas_fn_handle_t;
>> 
>> #define rtas_fn_handle(x_) (const rtas_fn_handle_t) { .index =
>> rtas_fnidx(x_), }
>> 
>> #define RTAS_FN_CHECK_EXCEPTION                  
>> rtas_fn_handle(CHECK_EXCEPTION)
>> #define RTAS_FN_DISPLAY_CHARACTER                
>> rtas_fn_handle(DISPLAY_CHARACTER)
>> #define RTAS_FN_EVENT_SCAN                       
>> rtas_fn_handle(EVENT_SCAN)
>> 
>> ...
>> 
>> /**
>>  * rtas_function_token() - RTAS function token lookup.
>>  * @handle: Function handle, e.g. RTAS_FN_EVENT_SCAN.
>>  *
>>  * Context: Any context.
>>  * Return: the token value for the function if implemented by this
>> platform,
>>  *         otherwise RTAS_UNKNOWN_SERVICE.
>>  */
>> s32 rtas_function_token(const rtas_fn_handle_t handle)
>> {
>>         const size_t index = handle.index;
>>         const bool out_of_bounds = index >=
>> ARRAY_SIZE(rtas_function_table);
>> 
>>         if (WARN_ONCE(out_of_bounds, "invalid function index %zu",
>> index))
>>                 return RTAS_UNKNOWN_SERVICE;
>> 
>>         return rtas_function_table[index].token;
>> }
>> 
>> But that's a tree-wide change that would touch various drivers, and I
>> feel like the current series is what I can reasonably hope to get
>> applied right now.
>
> It's not clear to me what the benefit of adding this additional layer
> of indirection would be.

One benefit would be that the type system won't let you use a token
(int) where you meant to use a handle (struct), and vice versa. I
anticipate that property being valuable for making API changes. But also
it's just good interface hygiene: how the token lookup is implemented
isn't the concern of code outside of rtas.c.

Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

[Nick Child]

On 11/18/22 09:07, Nathan Lynch wrote:
> +static int __init rtas_token_to_function_xarray_init(void)
> +{
> +	int err = 0;
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
> +		const struct rtas_function *func = &rtas_function_table[i];
> +		const s32 token = func->token;
> +
> +		if (token == RTAS_UNKNOWN_SERVICE)
> +			continue;
> +
> +		err = xa_err(xa_store(&rtas_token_to_function_xarray,
> +				      token, (void *)func, GFP_KERNEL));
> +		if (err)
> +			break;
> +	}
> +
> +	return err;
> +}
> +arch_initcall(rtas_token_to_function_xarray_init);
> +
> +static const struct rtas_function *rtas_token_to_function(s32 token)
> +{
> +	const struct rtas_function *func;
> +
> +	if (WARN_ONCE(token < 0, "invalid token %d", token))
> +		return NULL;
> +
> +	func = xa_load(&rtas_token_to_function_xarray, (unsigned long)token);
> +
Why typecast token here and not in xa_store?


> +static void __init rtas_function_table_init(void)
> +{
> +	struct property *prop;
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
> +		struct rtas_function *curr = &rtas_function_table[i];
> +		struct rtas_function *prior;
> +		int cmp;
> +
> +		curr->token = RTAS_UNKNOWN_SERVICE;
> +
> +		if (i == 0)
> +			continue;
> +		/*
> +		 * Ensure table is sorted correctly for binary search
> +		 * on function names.
> +		 */
> +		prior = &rtas_function_table[i - 1];
> +
> +		cmp = strcmp(prior->name, curr->name);
> +		if (cmp < 0)
> +			continue;
> +
> +		if (cmp == 0) {
> +			pr_err("'%s' has duplicate function table entries\n",
> +			       curr->name);
> +		} else {
> +			pr_err("function table unsorted: '%s' wrongly precedes '%s'\n",
> +			       prior->name, curr->name);
> +		}
> +	}
Just a thought, would it be simpler to use sort()? you already have the
cmp_func implemented for bsearch().


As for the series as a whole:
I am no RTAS expert but was able to build, boot and mess around with new
tracepoints without errors:

Tested-by: Nick Child <nnac123@linux.ibm.com>

Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

[Nathan Lynch]

Nick Child <nnac123@linux.ibm.com> writes:
> On 11/18/22 09:07, Nathan Lynch wrote:
>> +static int __init rtas_token_to_function_xarray_init(void)
>> +{
>> +	int err = 0;
>> +
>> +	for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
>> +		const struct rtas_function *func = &rtas_function_table[i];
>> +		const s32 token = func->token;
>> +
>> +		if (token == RTAS_UNKNOWN_SERVICE)
>> +			continue;
>> +
>> +		err = xa_err(xa_store(&rtas_token_to_function_xarray,
>> +				      token, (void *)func, GFP_KERNEL));
>> +		if (err)
>> +			break;
>> +	}
>> +
>> +	return err;
>> +}
>> +arch_initcall(rtas_token_to_function_xarray_init);
>> +
>> +static const struct rtas_function *rtas_token_to_function(s32 token)
>> +{
>> +	const struct rtas_function *func;
>> +
>> +	if (WARN_ONCE(token < 0, "invalid token %d", token))
>> +		return NULL;
>> +
>> +	func = xa_load(&rtas_token_to_function_xarray, (unsigned long)token);
>> +
> Why typecast token here and not in xa_store?

No good reason. I'll add it to the xa_store() call site.

>> +static void __init rtas_function_table_init(void)
>> +{
>> +	struct property *prop;
>> +
>> +	for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
>> +		struct rtas_function *curr = &rtas_function_table[i];
>> +		struct rtas_function *prior;
>> +		int cmp;
>> +
>> +		curr->token = RTAS_UNKNOWN_SERVICE;
>> +
>> +		if (i == 0)
>> +			continue;
>> +		/*
>> +		 * Ensure table is sorted correctly for binary search
>> +		 * on function names.
>> +		 */
>> +		prior = &rtas_function_table[i - 1];
>> +
>> +		cmp = strcmp(prior->name, curr->name);
>> +		if (cmp < 0)
>> +			continue;
>> +
>> +		if (cmp == 0) {
>> +			pr_err("'%s' has duplicate function table entries\n",
>> +			       curr->name);
>> +		} else {
>> +			pr_err("function table unsorted: '%s' wrongly precedes '%s'\n",
>> +			       prior->name, curr->name);
>> +		}
>> +	}
> Just a thought, would it be simpler to use sort()? you already have the
> cmp_func implemented for bsearch().

It's an option, but I think a tradeoff is that we would have to
sacrifice some const-ness in the data structures (i.e. remove the const
qualifier from struct rtas_function's fields). And the table has to be
in *some* order, so it may as well be sorted by name from the start.

That said, I don't love resorting to a boot-time check for this. We
could sidestep the issue by generating the C code for the table and
indexes at build time, but it's hard to justify the effort when the set
of RTAS functions changes very slowly over time.

> As for the series as a whole:
> I am no RTAS expert but was able to build, boot and mess around with new
> tracepoints without errors:
>
> Tested-by: Nick Child <nnac123@linux.ibm.com>

Thanks for testing and reviewing!

[PATCH 11/13] powerpc/rtas: strengthen do_enter_rtas() type safety, drop inline

[Nathan Lynch]

Make do_enter_rtas() take a pointer to struct rtas_args and do the
__pa() conversion in one place instead of leaving it to callers. This
also makes it possible to introduce enter/exit tracepoints that access
the rtas_args struct fields.

There's no apparent reason to force inlining of do_enter_rtas()
either, and it seems to bloat the code a bit. Let the compiler decide.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index a88db3b3486f..198366d641d0 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -522,7 +522,7 @@ static const struct rtas_function *rtas_token_to_function(s32 token)
 /* This is here deliberately so it's only used in this file */
 void enter_rtas(unsigned long);
 
-static inline void do_enter_rtas(unsigned long args)
+static void do_enter_rtas(struct rtas_args *args)
 {
 	unsigned long msr;
 
@@ -537,7 +537,7 @@ static inline void do_enter_rtas(unsigned long args)
 
 	hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
 
-	enter_rtas(args);
+	enter_rtas(__pa(args));
 
 	srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
 }
@@ -908,7 +908,7 @@ static char *__fetch_rtas_last_error(char *altbuf)
 	save_args = rtas.args;
 	rtas.args = err_args;
 
-	do_enter_rtas(__pa(&rtas.args));
+	do_enter_rtas(&rtas.args);
 
 	err_args = rtas.args;
 	rtas.args = save_args;
@@ -955,7 +955,7 @@ va_rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret,
 	for (i = 0; i < nret; ++i)
 		args->rets[i] = 0;
 
-	do_enter_rtas(__pa(args));
+	do_enter_rtas(args);
 }
 
 void rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret, ...)
@@ -1731,7 +1731,7 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 	flags = lock_rtas();
 
 	rtas.args = args;
-	do_enter_rtas(__pa(&rtas.args));
+	do_enter_rtas(&rtas.args);
 	args = rtas.args;
 
 	/* A -1 return code indicates that the last command couldn't
-- 
2.37.1

Re: [PATCH 11/13] powerpc/rtas: strengthen do_enter_rtas() type safety, drop inline

[Andrew Donnellan]

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> Make do_enter_rtas() take a pointer to struct rtas_args and do the
> __pa() conversion in one place instead of leaving it to callers. This
> also makes it possible to introduce enter/exit tracepoints that
> access
> the rtas_args struct fields.
> 
> There's no apparent reason to force inlining of do_enter_rtas()
> either, and it seems to bloat the code a bit. Let the compiler
> decide.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Makes sense!

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/kernel/rtas.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index a88db3b3486f..198366d641d0 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -522,7 +522,7 @@ static const struct rtas_function
> *rtas_token_to_function(s32 token)
>  /* This is here deliberately so it's only used in this file */
>  void enter_rtas(unsigned long);
>  
> -static inline void do_enter_rtas(unsigned long args)
> +static void do_enter_rtas(struct rtas_args *args)
>  {
>         unsigned long msr;
>  
> @@ -537,7 +537,7 @@ static inline void do_enter_rtas(unsigned long
> args)
>  
>         hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
>  
> -       enter_rtas(args);
> +       enter_rtas(__pa(args));
>  
>         srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
>  }
> @@ -908,7 +908,7 @@ static char *__fetch_rtas_last_error(char
> *altbuf)
>         save_args = rtas.args;
>         rtas.args = err_args;
>  
> -       do_enter_rtas(__pa(&rtas.args));
> +       do_enter_rtas(&rtas.args);
>  
>         err_args = rtas.args;
>         rtas.args = save_args;
> @@ -955,7 +955,7 @@ va_rtas_call_unlocked(struct rtas_args *args, int
> token, int nargs, int nret,
>         for (i = 0; i < nret; ++i)
>                 args->rets[i] = 0;
>  
> -       do_enter_rtas(__pa(args));
> +       do_enter_rtas(args);
>  }
>  
>  void rtas_call_unlocked(struct rtas_args *args, int token, int
> nargs, int nret, ...)
> @@ -1731,7 +1731,7 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user
> *, uargs)
>         flags = lock_rtas();
>  
>         rtas.args = args;
> -       do_enter_rtas(__pa(&rtas.args));
> +       do_enter_rtas(&rtas.args);
>         args = rtas.args;
>  
>         /* A -1 return code indicates that the last command couldn't

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 11/13] powerpc/rtas: strengthen do_enter_rtas() type safety, drop inline

[Nicholas Piggin]

On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
> Make do_enter_rtas() take a pointer to struct rtas_args and do the
> __pa() conversion in one place instead of leaving it to callers. This
> also makes it possible to introduce enter/exit tracepoints that access
> the rtas_args struct fields.
>
> There's no apparent reason to force inlining of do_enter_rtas()
> either, and it seems to bloat the code a bit. Let the compiler decide.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/kernel/rtas.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index a88db3b3486f..198366d641d0 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -522,7 +522,7 @@ static const struct rtas_function *rtas_token_to_function(s32 token)
>  /* This is here deliberately so it's only used in this file */
>  void enter_rtas(unsigned long);
>  
> -static inline void do_enter_rtas(unsigned long args)
> +static void do_enter_rtas(struct rtas_args *args)
>  {
>  	unsigned long msr;
>  
> @@ -537,7 +537,7 @@ static inline void do_enter_rtas(unsigned long args)
>  
>  	hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
>  
> -	enter_rtas(args);
> +	enter_rtas(__pa(args));
>  
>  	srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
>  }
> @@ -908,7 +908,7 @@ static char *__fetch_rtas_last_error(char *altbuf)
>  	save_args = rtas.args;
>  	rtas.args = err_args;
>  
> -	do_enter_rtas(__pa(&rtas.args));
> +	do_enter_rtas(&rtas.args);
>  
>  	err_args = rtas.args;
>  	rtas.args = save_args;
> @@ -955,7 +955,7 @@ va_rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret,
>  	for (i = 0; i < nret; ++i)
>  		args->rets[i] = 0;
>  
> -	do_enter_rtas(__pa(args));
> +	do_enter_rtas(args);
>  }
>  
>  void rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret, ...)
> @@ -1731,7 +1731,7 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
>  	flags = lock_rtas();
>  
>  	rtas.args = args;
> -	do_enter_rtas(__pa(&rtas.args));
> +	do_enter_rtas(&rtas.args);
>  	args = rtas.args;
>  
>  	/* A -1 return code indicates that the last command couldn't
> -- 
> 2.37.1

[PATCH 12/13] powerpc/tracing: tracepoints for RTAS entry and exit

[Nathan Lynch]

Add two sets of tracepoints to be used around RTAS entry:

* rtas_input/rtas_output, which emit the function name, its inputs,
  the returned status, and any other outputs. These produce an API-level
  record of OS<->RTAS activity.

* rtas_ll_entry/rtas_ll_exit, which are lower-level and emit the
  entire contents of the parameter block (aka rtas_args) on entry and
  exit. Likely useful only for debugging.

With uses of these tracepoints in do_enter_rtas() to be added in the
following patch, examples of get-time-of-day and event-scan functions
as rendered by trace-cmd (with some multi-line formatting manually
imposed on the rtas_ll_* entries to avoid extremely long lines in the
commit message):

cat-36800 [059]  4978.518303: rtas_input:           get-time-of-day arguments:
cat-36800 [059]  4978.518306: rtas_ll_entry:        token=3 nargs=0 nret=8
                                                    params: [0]=0x00000000 [1]=0x00000000 [2]=0x00000000 [3]=0x00000000
                                                            [4]=0x00000000 [5]=0x00000000 [6]=0x00000000 [7]=0x00000000
							    [8]=0x00000000 [9]=0x00000000 [10]=0x00000000 [11]=0x00000000
							    [12]=0x00000000 [13]=0x00000000 [14]=0x00000000 [15]=0x00000000
cat-36800 [059]  4978.518366: rtas_ll_exit:         token=3 nargs=0 nret=8
                                                    params: [0]=0x00000000 [1]=0x000007e6 [2]=0x0000000b [3]=0x00000001
						            [4]=0x00000000 [5]=0x0000000e [6]=0x00000008 [7]=0x2e0dac40
							    [8]=0x00000000 [9]=0x00000000 [10]=0x00000000 [11]=0x00000000
							    [12]=0x00000000 [13]=0x00000000 [14]=0x00000000 [15]=0x00000000
cat-36800 [059]  4978.518366: rtas_output:          get-time-of-day status: 0, other outputs: 2022 11 1 0 14 8 772648000

kworker/39:1-336   [039]  4982.731623: rtas_input:           event-scan arguments: 4294967295 0 80484920 2048
kworker/39:1-336   [039]  4982.731626: rtas_ll_entry:        token=6 nargs=4 nret=1
                                                             params: [0]=0xffffffff [1]=0x00000000 [2]=0x04cc1a38 [3]=0x00000800
							             [4]=0x00000000 [5]=0x0000000e [6]=0x00000008 [7]=0x2e0dac40
								     [8]=0x00000000 [9]=0x00000000 [10]=0x00000000 [11]=0x00000000
								     [12]=0x00000000 [13]=0x00000000 [14]=0x00000000 [15]=0x00000000
kworker/39:1-336   [039]  4982.731676: rtas_ll_exit:         token=6 nargs=4 nret=1
                                                             params: [0]=0xffffffff [1]=0x00000000 [2]=0x04cc1a38 [3]=0x00000800
							             [4]=0x00000001 [5]=0x0000000e [6]=0x00000008 [7]=0x2e0dac40
								     [8]=0x00000000 [9]=0x00000000 [10]=0x00000000 [11]=0x00000000
								     [12]=0x00000000 [13]=0x00000000 [14]=0x00000000 [15]=0x00000000
kworker/39:1-336   [039]  4982.731677: rtas_output:          event-scan status: 1, other outputs:

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/include/asm/trace.h | 116 +++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/arch/powerpc/include/asm/trace.h b/arch/powerpc/include/asm/trace.h
index 08cd60cd70b7..e7a301c9eb95 100644
--- a/arch/powerpc/include/asm/trace.h
+++ b/arch/powerpc/include/asm/trace.h
@@ -119,6 +119,122 @@ TRACE_EVENT_FN_COND(hcall_exit,
 );
 #endif
 
+#ifdef CONFIG_PPC_RTAS
+
+#include <asm/rtas-types.h>
+
+/*
+ * Since stop-self is how CPUs go offline on RTAS platforms,
+ * these tracepoints are conditional.
+ */
+
+TRACE_EVENT_CONDITION(rtas_input,
+
+	TP_PROTO(struct rtas_args *rtas_args, const char *name),
+
+	TP_ARGS(rtas_args, name),
+
+	TP_CONDITION(cpu_online(raw_smp_processor_id())),
+
+	TP_STRUCT__entry(
+		__field(__u32, nargs)
+		__string(name, name)
+		__dynamic_array(__u32, inputs, be32_to_cpu(rtas_args->nargs))
+	),
+
+	TP_fast_assign(
+		__entry->nargs = be32_to_cpu(rtas_args->nargs);
+		__assign_str(name, name);
+		be32_to_cpu_array(__get_dynamic_array(inputs), rtas_args->args, __entry->nargs);
+	),
+
+	TP_printk("%s arguments: %s", __get_str(name),
+		  __print_array(__get_dynamic_array(inputs), __entry->nargs, 4)
+	)
+);
+
+TRACE_EVENT_CONDITION(rtas_output,
+
+	TP_PROTO(struct rtas_args *rtas_args, const char *name),
+
+	TP_ARGS(rtas_args, name),
+
+	TP_CONDITION(cpu_online(raw_smp_processor_id())),
+
+	TP_STRUCT__entry(
+		__field(__u32, nr_other)
+		__field(__s32, status)
+		__string(name, name)
+		__dynamic_array(__u32, other_outputs, be32_to_cpu(rtas_args->nret) - 1)
+	),
+
+	TP_fast_assign(
+		__entry->nr_other = be32_to_cpu(rtas_args->nret) - 1;
+		__entry->status = be32_to_cpu(rtas_args->rets[0]);
+		__assign_str(name, name);
+		be32_to_cpu_array(__get_dynamic_array(other_outputs),
+				  &rtas_args->rets[1], __entry->nr_other);
+	),
+
+	TP_printk("%s status: %d, other outputs: %s", __get_str(name), __entry->status,
+		  __print_array(__get_dynamic_array(other_outputs),
+				__entry->nr_other, 4)
+	)
+);
+
+DECLARE_EVENT_CLASS(rtas_parameter_block,
+
+	TP_PROTO(struct rtas_args *rtas_args),
+
+	TP_ARGS(rtas_args),
+
+	TP_STRUCT__entry(
+		__field(u32, token)
+		__field(u32, nargs)
+		__field(u32, nret)
+		__array(__u32, params, 16)
+	),
+
+	TP_fast_assign(
+		__entry->token = be32_to_cpu(rtas_args->token);
+		__entry->nargs = be32_to_cpu(rtas_args->nargs);
+		__entry->nret = be32_to_cpu(rtas_args->nret);
+		be32_to_cpu_array(__entry->params, rtas_args->args, ARRAY_SIZE(rtas_args->args));
+	),
+
+	TP_printk("token=%u nargs=%u nret=%u params:"
+		  " [0]=0x%08x [1]=0x%08x [2]=0x%08x [3]=0x%08x"
+		  " [4]=0x%08x [5]=0x%08x [6]=0x%08x [7]=0x%08x"
+		  " [8]=0x%08x [9]=0x%08x [10]=0x%08x [11]=0x%08x"
+		  " [12]=0x%08x [13]=0x%08x [14]=0x%08x [15]=0x%08x",
+		  __entry->token, __entry->nargs, __entry->nret,
+		  __entry->params[0], __entry->params[1], __entry->params[2], __entry->params[3],
+		  __entry->params[4], __entry->params[5], __entry->params[6], __entry->params[7],
+		  __entry->params[8], __entry->params[9], __entry->params[10], __entry->params[11],
+		  __entry->params[12], __entry->params[13], __entry->params[14], __entry->params[15]
+	)
+);
+
+DEFINE_EVENT_CONDITION(rtas_parameter_block, rtas_ll_entry,
+
+	TP_PROTO(struct rtas_args *rtas_args),
+
+	TP_ARGS(rtas_args),
+
+	TP_CONDITION(cpu_online(raw_smp_processor_id()))
+);
+
+DEFINE_EVENT_CONDITION(rtas_parameter_block, rtas_ll_exit,
+
+	TP_PROTO(struct rtas_args *rtas_args),
+
+	TP_ARGS(rtas_args),
+
+	TP_CONDITION(cpu_online(raw_smp_processor_id()))
+);
+
+#endif /* CONFIG_PPC_RTAS */
+
 #ifdef CONFIG_PPC_POWERNV
 extern int opal_tracepoint_regfunc(void);
 extern void opal_tracepoint_unregfunc(void);
-- 
2.37.1

Re: [PATCH 12/13] powerpc/tracing: tracepoints for RTAS entry and exit

[Nicholas Piggin]

On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
> Add two sets of tracepoints to be used around RTAS entry:
>
> * rtas_input/rtas_output, which emit the function name, its inputs,
>   the returned status, and any other outputs. These produce an API-level
>   record of OS<->RTAS activity.
>
> * rtas_ll_entry/rtas_ll_exit, which are lower-level and emit the
>   entire contents of the parameter block (aka rtas_args) on entry and
>   exit. Likely useful only for debugging.
>
> With uses of these tracepoints in do_enter_rtas() to be added in the
> following patch, examples of get-time-of-day and event-scan functions
> as rendered by trace-cmd (with some multi-line formatting manually
> imposed on the rtas_ll_* entries to avoid extremely long lines in the
> commit message):
>
> cat-36800 [059]  4978.518303: rtas_input:           get-time-of-day arguments:
> cat-36800 [059]  4978.518306: rtas_ll_entry:        token=3 nargs=0 nret=8
>                                                     params: [0]=0x00000000 [1]=0x00000000 [2]=0x00000000 [3]=0x00000000
>                                                             [4]=0x00000000 [5]=0x00000000 [6]=0x00000000 [7]=0x00000000
> 							    [8]=0x00000000 [9]=0x00000000 [10]=0x00000000 [11]=0x00000000
> 							    [12]=0x00000000 [13]=0x00000000 [14]=0x00000000 [15]=0x00000000
> cat-36800 [059]  4978.518366: rtas_ll_exit:         token=3 nargs=0 nret=8
>                                                     params: [0]=0x00000000 [1]=0x000007e6 [2]=0x0000000b [3]=0x00000001
> 						            [4]=0x00000000 [5]=0x0000000e [6]=0x00000008 [7]=0x2e0dac40
> 							    [8]=0x00000000 [9]=0x00000000 [10]=0x00000000 [11]=0x00000000
> 							    [12]=0x00000000 [13]=0x00000000 [14]=0x00000000 [15]=0x00000000
> cat-36800 [059]  4978.518366: rtas_output:          get-time-of-day status: 0, other outputs: 2022 11 1 0 14 8 772648000
>
> kworker/39:1-336   [039]  4982.731623: rtas_input:           event-scan arguments: 4294967295 0 80484920 2048
> kworker/39:1-336   [039]  4982.731626: rtas_ll_entry:        token=6 nargs=4 nret=1
>                                                              params: [0]=0xffffffff [1]=0x00000000 [2]=0x04cc1a38 [3]=0x00000800
> 							             [4]=0x00000000 [5]=0x0000000e [6]=0x00000008 [7]=0x2e0dac40
> 								     [8]=0x00000000 [9]=0x00000000 [10]=0x00000000 [11]=0x00000000
> 								     [12]=0x00000000 [13]=0x00000000 [14]=0x00000000 [15]=0x00000000
> kworker/39:1-336   [039]  4982.731676: rtas_ll_exit:         token=6 nargs=4 nret=1
>                                                              params: [0]=0xffffffff [1]=0x00000000 [2]=0x04cc1a38 [3]=0x00000800
> 							             [4]=0x00000001 [5]=0x0000000e [6]=0x00000008 [7]=0x2e0dac40
> 								     [8]=0x00000000 [9]=0x00000000 [10]=0x00000000 [11]=0x00000000
> 								     [12]=0x00000000 [13]=0x00000000 [14]=0x00000000 [15]=0x00000000
> kworker/39:1-336   [039]  4982.731677: rtas_output:          event-scan status: 1, other outputs:
>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/trace.h | 116 +++++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/trace.h b/arch/powerpc/include/asm/trace.h
> index 08cd60cd70b7..e7a301c9eb95 100644
> --- a/arch/powerpc/include/asm/trace.h
> +++ b/arch/powerpc/include/asm/trace.h
> @@ -119,6 +119,122 @@ TRACE_EVENT_FN_COND(hcall_exit,
>  );
>  #endif
>  
> +#ifdef CONFIG_PPC_RTAS
> +
> +#include <asm/rtas-types.h>
> +
> +/*
> + * Since stop-self is how CPUs go offline on RTAS platforms,
> + * these tracepoints are conditional.
> + */
> +
> +TRACE_EVENT_CONDITION(rtas_input,
> +
> +	TP_PROTO(struct rtas_args *rtas_args, const char *name),
> +
> +	TP_ARGS(rtas_args, name),
> +
> +	TP_CONDITION(cpu_online(raw_smp_processor_id())),
> +
> +	TP_STRUCT__entry(
> +		__field(__u32, nargs)
> +		__string(name, name)
> +		__dynamic_array(__u32, inputs, be32_to_cpu(rtas_args->nargs))
> +	),
> +
> +	TP_fast_assign(
> +		__entry->nargs = be32_to_cpu(rtas_args->nargs);
> +		__assign_str(name, name);
> +		be32_to_cpu_array(__get_dynamic_array(inputs), rtas_args->args, __entry->nargs);
> +	),
> +
> +	TP_printk("%s arguments: %s", __get_str(name),
> +		  __print_array(__get_dynamic_array(inputs), __entry->nargs, 4)
> +	)
> +);
> +
> +TRACE_EVENT_CONDITION(rtas_output,
> +
> +	TP_PROTO(struct rtas_args *rtas_args, const char *name),
> +
> +	TP_ARGS(rtas_args, name),
> +
> +	TP_CONDITION(cpu_online(raw_smp_processor_id())),
> +
> +	TP_STRUCT__entry(
> +		__field(__u32, nr_other)
> +		__field(__s32, status)
> +		__string(name, name)
> +		__dynamic_array(__u32, other_outputs, be32_to_cpu(rtas_args->nret) - 1)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->nr_other = be32_to_cpu(rtas_args->nret) - 1;
> +		__entry->status = be32_to_cpu(rtas_args->rets[0]);
> +		__assign_str(name, name);
> +		be32_to_cpu_array(__get_dynamic_array(other_outputs),
> +				  &rtas_args->rets[1], __entry->nr_other);
> +	),
> +
> +	TP_printk("%s status: %d, other outputs: %s", __get_str(name), __entry->status,
> +		  __print_array(__get_dynamic_array(other_outputs),
> +				__entry->nr_other, 4)
> +	)
> +);
> +
> +DECLARE_EVENT_CLASS(rtas_parameter_block,
> +
> +	TP_PROTO(struct rtas_args *rtas_args),
> +
> +	TP_ARGS(rtas_args),
> +
> +	TP_STRUCT__entry(
> +		__field(u32, token)
> +		__field(u32, nargs)
> +		__field(u32, nret)
> +		__array(__u32, params, 16)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->token = be32_to_cpu(rtas_args->token);
> +		__entry->nargs = be32_to_cpu(rtas_args->nargs);
> +		__entry->nret = be32_to_cpu(rtas_args->nret);
> +		be32_to_cpu_array(__entry->params, rtas_args->args, ARRAY_SIZE(rtas_args->args));
> +	),
> +
> +	TP_printk("token=%u nargs=%u nret=%u params:"
> +		  " [0]=0x%08x [1]=0x%08x [2]=0x%08x [3]=0x%08x"
> +		  " [4]=0x%08x [5]=0x%08x [6]=0x%08x [7]=0x%08x"
> +		  " [8]=0x%08x [9]=0x%08x [10]=0x%08x [11]=0x%08x"
> +		  " [12]=0x%08x [13]=0x%08x [14]=0x%08x [15]=0x%08x",

You could justify these since you went to the trouble to format them
nicely. Not a big deal though.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> +		  __entry->token, __entry->nargs, __entry->nret,
> +		  __entry->params[0], __entry->params[1], __entry->params[2], __entry->params[3],
> +		  __entry->params[4], __entry->params[5], __entry->params[6], __entry->params[7],
> +		  __entry->params[8], __entry->params[9], __entry->params[10], __entry->params[11],
> +		  __entry->params[12], __entry->params[13], __entry->params[14], __entry->params[15]
> +	)
> +);
> +
> +DEFINE_EVENT_CONDITION(rtas_parameter_block, rtas_ll_entry,
> +
> +	TP_PROTO(struct rtas_args *rtas_args),
> +
> +	TP_ARGS(rtas_args),
> +
> +	TP_CONDITION(cpu_online(raw_smp_processor_id()))
> +);
> +
> +DEFINE_EVENT_CONDITION(rtas_parameter_block, rtas_ll_exit,
> +
> +	TP_PROTO(struct rtas_args *rtas_args),
> +
> +	TP_ARGS(rtas_args),
> +
> +	TP_CONDITION(cpu_online(raw_smp_processor_id()))
> +);
> +
> +#endif /* CONFIG_PPC_RTAS */
> +
>  #ifdef CONFIG_PPC_POWERNV
>  extern int opal_tracepoint_regfunc(void);
>  extern void opal_tracepoint_unregfunc(void);
> -- 
> 2.37.1

[PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()

[Nathan Lynch]

Call the just-added rtas tracepoints in do_enter_rtas(), taking care
to avoid function name lookups in the CPU offline path.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 198366d641d0..3487b42cfbf7 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -38,6 +38,7 @@
 #include <asm/page.h>
 #include <asm/rtas.h>
 #include <asm/time.h>
+#include <asm/trace.h>
 #include <asm/udbg.h>
 
 enum rtas_function_flags {
@@ -525,6 +526,7 @@ void enter_rtas(unsigned long);
 static void do_enter_rtas(struct rtas_args *args)
 {
 	unsigned long msr;
+	const char *name = NULL;
 
 	/*
 	 * Make sure MSR[RI] is currently enabled as it will be forced later
@@ -537,9 +539,30 @@ static void do_enter_rtas(struct rtas_args *args)
 
 	hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
 
+	if ((trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
+		/*
+		 * rtas_token_to_function() uses xarray which uses RCU,
+		 * but this code can run in the CPU offline path
+		 * (e.g. stop-self), after it's become invalid to call
+		 * RCU APIs.
+		 */
+		if (cpu_online(smp_processor_id())) {
+			const s32 token = be32_to_cpu(args->token);
+			const struct rtas_function *func = rtas_token_to_function(token);
+
+			name = func->name;
+		}
+	}
+
+	trace_rtas_input(args, name);
+	trace_rtas_ll_entry(args);
+
 	enter_rtas(__pa(args));
 
 	srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
+
+	trace_rtas_ll_exit(args);
+	trace_rtas_output(args, name);
 }
 
 struct rtas_t rtas = {
-- 
2.37.1

Re: [PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()

[Nicholas Piggin]

On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
> Call the just-added rtas tracepoints in do_enter_rtas(), taking care
> to avoid function name lookups in the CPU offline path.
>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/kernel/rtas.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 198366d641d0..3487b42cfbf7 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -38,6 +38,7 @@
>  #include <asm/page.h>
>  #include <asm/rtas.h>
>  #include <asm/time.h>
> +#include <asm/trace.h>
>  #include <asm/udbg.h>
>  
>  enum rtas_function_flags {
> @@ -525,6 +526,7 @@ void enter_rtas(unsigned long);
>  static void do_enter_rtas(struct rtas_args *args)
>  {
>  	unsigned long msr;
> +	const char *name = NULL;
>  
>  	/*
>  	 * Make sure MSR[RI] is currently enabled as it will be forced later
> @@ -537,9 +539,30 @@ static void do_enter_rtas(struct rtas_args *args)
>  
>  	hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
>  
> +	if ((trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
> +		/*
> +		 * rtas_token_to_function() uses xarray which uses RCU,
> +		 * but this code can run in the CPU offline path
> +		 * (e.g. stop-self), after it's become invalid to call
> +		 * RCU APIs.
> +		 */

We can call this in real-mode via pseries_machine_check_realmode
-> fwnmi_release_errinfo, so tracing should be disabled for that
case too... Does this_cpu_set_ftrace_enabled(0) in the early
machine check handler cover that sufficiently?

Thanks,
Nick

Re: [PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()

[Nathan Lynch]

"Nicholas Piggin" <npiggin@gmail.com> writes:

> On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
>> Call the just-added rtas tracepoints in do_enter_rtas(), taking care
>> to avoid function name lookups in the CPU offline path.
>>
>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>> ---
>>  arch/powerpc/kernel/rtas.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>> index 198366d641d0..3487b42cfbf7 100644
>> --- a/arch/powerpc/kernel/rtas.c
>> +++ b/arch/powerpc/kernel/rtas.c
>> @@ -38,6 +38,7 @@
>>  #include <asm/page.h>
>>  #include <asm/rtas.h>
>>  #include <asm/time.h>
>> +#include <asm/trace.h>
>>  #include <asm/udbg.h>
>>  
>>  enum rtas_function_flags {
>> @@ -525,6 +526,7 @@ void enter_rtas(unsigned long);
>>  static void do_enter_rtas(struct rtas_args *args)
>>  {
>>  	unsigned long msr;
>> +	const char *name = NULL;
>>  
>>  	/*
>>  	 * Make sure MSR[RI] is currently enabled as it will be forced later
>> @@ -537,9 +539,30 @@ static void do_enter_rtas(struct rtas_args *args)
>>  
>>  	hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
>>  
>> +	if ((trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
>> +		/*
>> +		 * rtas_token_to_function() uses xarray which uses RCU,
>> +		 * but this code can run in the CPU offline path
>> +		 * (e.g. stop-self), after it's become invalid to call
>> +		 * RCU APIs.
>> +		 */
>
> We can call this in real-mode via pseries_machine_check_realmode
> -> fwnmi_release_errinfo, so tracing should be disabled for that
> case too... Does this_cpu_set_ftrace_enabled(0) in the early
> machine check handler cover that sufficiently?

I suspect so, but I'd like to verify. Do you know how I could exercise
this path in qemu or LPAR?

Re: [PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()

[Michael Ellerman]

Nathan Lynch <nathanl@linux.ibm.com> writes:
> "Nicholas Piggin" <npiggin@gmail.com> writes:
>> On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
>>> Call the just-added rtas tracepoints in do_enter_rtas(), taking care
>>> to avoid function name lookups in the CPU offline path.
>>>
>>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>>> ---
>>>  arch/powerpc/kernel/rtas.c | 23 +++++++++++++++++++++++
>>>  1 file changed, 23 insertions(+)
>>>
>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>> index 198366d641d0..3487b42cfbf7 100644
>>> --- a/arch/powerpc/kernel/rtas.c
>>> +++ b/arch/powerpc/kernel/rtas.c
>>> @@ -38,6 +38,7 @@
>>>  #include <asm/page.h>
>>>  #include <asm/rtas.h>
>>>  #include <asm/time.h>
>>> +#include <asm/trace.h>
>>>  #include <asm/udbg.h>
>>>  
>>>  enum rtas_function_flags {
>>> @@ -525,6 +526,7 @@ void enter_rtas(unsigned long);
>>>  static void do_enter_rtas(struct rtas_args *args)
>>>  {
>>>  	unsigned long msr;
>>> +	const char *name = NULL;
>>>  
>>>  	/*
>>>  	 * Make sure MSR[RI] is currently enabled as it will be forced later
>>> @@ -537,9 +539,30 @@ static void do_enter_rtas(struct rtas_args *args)
>>>  
>>>  	hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
>>>  
>>> +	if ((trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
>>> +		/*
>>> +		 * rtas_token_to_function() uses xarray which uses RCU,
>>> +		 * but this code can run in the CPU offline path
>>> +		 * (e.g. stop-self), after it's become invalid to call
>>> +		 * RCU APIs.
>>> +		 */
>>
>> We can call this in real-mode via pseries_machine_check_realmode
>> -> fwnmi_release_errinfo, so tracing should be disabled for that
>> case too... Does this_cpu_set_ftrace_enabled(0) in the early
>> machine check handler cover that sufficiently?
>
> I suspect so, but I'd like to verify. Do you know how I could exercise
> this path in qemu or LPAR?

On a P9 or P10 LPAR you should be able to use tools/testing/selftests/powerpc/mce/inject-ra-err

cheers

Re: [PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()

[Nathan Lynch]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Nathan Lynch <nathanl@linux.ibm.com> writes:
>> "Nicholas Piggin" <npiggin@gmail.com> writes:
>>> On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
>>>> Call the just-added rtas tracepoints in do_enter_rtas(), taking care
>>>> to avoid function name lookups in the CPU offline path.
>>>>
>>>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>>>> ---
>>>>  arch/powerpc/kernel/rtas.c | 23 +++++++++++++++++++++++
>>>>  1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>>> index 198366d641d0..3487b42cfbf7 100644
>>>> --- a/arch/powerpc/kernel/rtas.c
>>>> +++ b/arch/powerpc/kernel/rtas.c
>>>> @@ -38,6 +38,7 @@
>>>>  #include <asm/page.h>
>>>>  #include <asm/rtas.h>
>>>>  #include <asm/time.h>
>>>> +#include <asm/trace.h>
>>>>  #include <asm/udbg.h>
>>>>  
>>>>  enum rtas_function_flags {
>>>> @@ -525,6 +526,7 @@ void enter_rtas(unsigned long);
>>>>  static void do_enter_rtas(struct rtas_args *args)
>>>>  {
>>>>  	unsigned long msr;
>>>> +	const char *name = NULL;
>>>>  
>>>>  	/*
>>>>  	 * Make sure MSR[RI] is currently enabled as it will be forced later
>>>> @@ -537,9 +539,30 @@ static void do_enter_rtas(struct rtas_args *args)
>>>>  
>>>>  	hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
>>>>  
>>>> +	if ((trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
>>>> +		/*
>>>> +		 * rtas_token_to_function() uses xarray which uses RCU,
>>>> +		 * but this code can run in the CPU offline path
>>>> +		 * (e.g. stop-self), after it's become invalid to call
>>>> +		 * RCU APIs.
>>>> +		 */
>>>
>>> We can call this in real-mode via pseries_machine_check_realmode
>>> -> fwnmi_release_errinfo, so tracing should be disabled for that
>>> case too... Does this_cpu_set_ftrace_enabled(0) in the early
>>> machine check handler cover that sufficiently?
>>
>> I suspect so, but I'd like to verify. Do you know how I could exercise
>> this path in qemu or LPAR?
>
> On a P9 or P10 LPAR you should be able to use tools/testing/selftests/powerpc/mce/inject-ra-err

Nice. Looks like I was too optimistic. From a P10 LPAR:

# trace-cmd record -T -e powerpc:rtas_input -- \
  sh -c 'sleep 10; ./inject-ra-err' && trace-cmd report
     kworker/7:1-73    [007]    72.882159: rtas_input:           event-scan arguments: 4294967295 0 80419368 2048
     kworker/7:1-73    [007]    72.882165: kernel_stack:         <stack trace >
=> do_enter_rtas (c000000000045180)
=> rtas_call (c000000000045da8)
=> rtas_event_scan (c000000000049458)
=> process_one_work (c0000000001c7618)
=> worker_thread (c0000000001c7bd8)
=> kthread (c0000000001d6858)
=> ret_from_kernel_thread (c00000000000cf5c)
   inject-ra-err-1080  [001]    78.386947: rtas_input:           ibm,nmi-interlock arguments: 
   inject-ra-err-1080  [001]    78.386950: kernel_stack:         <stack trace >
=> do_enter_rtas (c000000000045180)
=> rtas_call_unlocked (c000000000046ff4)
=> pseries_machine_check_realmode (c0000000000e8db8)
=> machine_check_early (c0000000000400c4)
=> machine_check_early_common (c00000000000836c)

So... that's bad. (right?)

I guess this patch needs something like this?

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 998aab967400..3086b5f6c6fc 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -541,6 +541,7 @@ static void do_enter_rtas(struct rtas_args *args)
 {
        unsigned long msr;
        const char *name = NULL;
+       bool can_trace;
 
        /*
         * Make sure MSR[RI] is currently enabled as it will be forced later
@@ -553,7 +554,9 @@ static void do_enter_rtas(struct rtas_args *args)
 
        hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
 
-       if ((trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
+       can_trace = (msr & MSR_IR) && (msr & MSR_DR);
+
+       if (can_trace && (trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
                /*
                 * rtas_token_to_function() uses xarray which uses RCU,
                 * but this code can run in the CPU offline path
@@ -568,15 +571,19 @@ static void do_enter_rtas(struct rtas_args *args)
                }
        }
 
-       trace_rtas_input(args, name);
-       trace_rtas_ll_entry(args);
+       if (can_trace) {
+               trace_rtas_input(args, name);
+               trace_rtas_ll_entry(args);
+       }
 
        enter_rtas(__pa(args));
 
        srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
 
-       trace_rtas_ll_exit(args);
-       trace_rtas_output(args, name);
+       if (can_trace) {
+               trace_rtas_ll_exit(args);
+               trace_rtas_output(args, name);
+       }
 }
 
 struct rtas_t rtas = {

Re: [PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()

[Michael Ellerman]

Nathan Lynch <nathanl@linux.ibm.com> writes:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>> Nathan Lynch <nathanl@linux.ibm.com> writes:
>>> "Nicholas Piggin" <npiggin@gmail.com> writes:
>>>> On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
>>>>> Call the just-added rtas tracepoints in do_enter_rtas(), taking care
>>>>> to avoid function name lookups in the CPU offline path.
>>>>>
>>>>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>>>>> ---
>>>>>  arch/powerpc/kernel/rtas.c | 23 +++++++++++++++++++++++
>>>>>  1 file changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>>>> index 198366d641d0..3487b42cfbf7 100644
>>>>> --- a/arch/powerpc/kernel/rtas.c
>>>>> +++ b/arch/powerpc/kernel/rtas.c
>>>>> @@ -38,6 +38,7 @@
>>>>>  #include <asm/page.h>
>>>>>  #include <asm/rtas.h>
>>>>>  #include <asm/time.h>
>>>>> +#include <asm/trace.h>
>>>>>  #include <asm/udbg.h>
>>>>>  
>>>>>  enum rtas_function_flags {
>>>>> @@ -525,6 +526,7 @@ void enter_rtas(unsigned long);
>>>>>  static void do_enter_rtas(struct rtas_args *args)
>>>>>  {
>>>>>  	unsigned long msr;
>>>>> +	const char *name = NULL;
>>>>>  
>>>>>  	/*
>>>>>  	 * Make sure MSR[RI] is currently enabled as it will be forced later
>>>>> @@ -537,9 +539,30 @@ static void do_enter_rtas(struct rtas_args *args)
>>>>>  
>>>>>  	hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
>>>>>  
>>>>> +	if ((trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
>>>>> +		/*
>>>>> +		 * rtas_token_to_function() uses xarray which uses RCU,
>>>>> +		 * but this code can run in the CPU offline path
>>>>> +		 * (e.g. stop-self), after it's become invalid to call
>>>>> +		 * RCU APIs.
>>>>> +		 */
>>>>
>>>> We can call this in real-mode via pseries_machine_check_realmode
>>>> -> fwnmi_release_errinfo, so tracing should be disabled for that
>>>> case too... Does this_cpu_set_ftrace_enabled(0) in the early
>>>> machine check handler cover that sufficiently?
>>>
>>> I suspect so, but I'd like to verify. Do you know how I could exercise
>>> this path in qemu or LPAR?
>>
>> On a P9 or P10 LPAR you should be able to use tools/testing/selftests/powerpc/mce/inject-ra-err
>
> Nice. Looks like I was too optimistic. From a P10 LPAR:
>
> # trace-cmd record -T -e powerpc:rtas_input -- \
>   sh -c 'sleep 10; ./inject-ra-err' && trace-cmd report
>      kworker/7:1-73    [007]    72.882159: rtas_input:           event-scan arguments: 4294967295 0 80419368 2048
>      kworker/7:1-73    [007]    72.882165: kernel_stack:         <stack trace >
> => do_enter_rtas (c000000000045180)
> => rtas_call (c000000000045da8)
> => rtas_event_scan (c000000000049458)
> => process_one_work (c0000000001c7618)
> => worker_thread (c0000000001c7bd8)
> => kthread (c0000000001d6858)
> => ret_from_kernel_thread (c00000000000cf5c)
>    inject-ra-err-1080  [001]    78.386947: rtas_input:           ibm,nmi-interlock arguments: 
>    inject-ra-err-1080  [001]    78.386950: kernel_stack:         <stack trace >
> => do_enter_rtas (c000000000045180)
> => rtas_call_unlocked (c000000000046ff4)
> => pseries_machine_check_realmode (c0000000000e8db8)
> => machine_check_early (c0000000000400c4)
> => machine_check_early_common (c00000000000836c)
>
> So... that's bad. (right?)

Potentially. If you booted in hash mode it might crash, if the tracing
causes accesses outside the RMO, or to vmalloc etc.

> I guess this patch needs something like this?

Yeah I think that's probably the easiest solution.

You could split do_enter_rtas() into a tracing and non-tracing version,
but I don't think that would be any cleaner.

cheers


> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 998aab967400..3086b5f6c6fc 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -541,6 +541,7 @@ static void do_enter_rtas(struct rtas_args *args)
>  {
>         unsigned long msr;
>         const char *name = NULL;
> +       bool can_trace;
>  
>         /*
>          * Make sure MSR[RI] is currently enabled as it will be forced later
> @@ -553,7 +554,9 @@ static void do_enter_rtas(struct rtas_args *args)
>  
>         hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
>  
> -       if ((trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
> +       can_trace = (msr & MSR_IR) && (msr & MSR_DR);
> +
> +       if (can_trace && (trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
>                 /*
>                  * rtas_token_to_function() uses xarray which uses RCU,
>                  * but this code can run in the CPU offline path
> @@ -568,15 +571,19 @@ static void do_enter_rtas(struct rtas_args *args)
>                 }
>         }
>  
> -       trace_rtas_input(args, name);
> -       trace_rtas_ll_entry(args);
> +       if (can_trace) {
> +               trace_rtas_input(args, name);
> +               trace_rtas_ll_entry(args);
> +       }
>  
>         enter_rtas(__pa(args));
>  
>         srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
>  
> -       trace_rtas_ll_exit(args);
> -       trace_rtas_output(args, name);
> +       if (can_trace) {
> +               trace_rtas_ll_exit(args);
> +               trace_rtas_output(args, name);
> +       }
>  }
>  
>  struct rtas_t rtas = {

Re: [PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()

[Nicholas Piggin]

On Tue Nov 29, 2022 at 9:44 AM AEST, Nathan Lynch wrote:
> "Nicholas Piggin" <npiggin@gmail.com> writes:
>
> > On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote:
> >> Call the just-added rtas tracepoints in do_enter_rtas(), taking care
> >> to avoid function name lookups in the CPU offline path.
> >>
> >> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> >> ---
> >>  arch/powerpc/kernel/rtas.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>
> >> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> >> index 198366d641d0..3487b42cfbf7 100644
> >> --- a/arch/powerpc/kernel/rtas.c
> >> +++ b/arch/powerpc/kernel/rtas.c
> >> @@ -38,6 +38,7 @@
> >>  #include <asm/page.h>
> >>  #include <asm/rtas.h>
> >>  #include <asm/time.h>
> >> +#include <asm/trace.h>
> >>  #include <asm/udbg.h>
> >>  
> >>  enum rtas_function_flags {
> >> @@ -525,6 +526,7 @@ void enter_rtas(unsigned long);
> >>  static void do_enter_rtas(struct rtas_args *args)
> >>  {
> >>  	unsigned long msr;
> >> +	const char *name = NULL;
> >>  
> >>  	/*
> >>  	 * Make sure MSR[RI] is currently enabled as it will be forced later
> >> @@ -537,9 +539,30 @@ static void do_enter_rtas(struct rtas_args *args)
> >>  
> >>  	hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */
> >>  
> >> +	if ((trace_rtas_input_enabled() || trace_rtas_output_enabled())) {
> >> +		/*
> >> +		 * rtas_token_to_function() uses xarray which uses RCU,
> >> +		 * but this code can run in the CPU offline path
> >> +		 * (e.g. stop-self), after it's become invalid to call
> >> +		 * RCU APIs.
> >> +		 */
> >
> > We can call this in real-mode via pseries_machine_check_realmode
> > -> fwnmi_release_errinfo, so tracing should be disabled for that
> > case too... Does this_cpu_set_ftrace_enabled(0) in the early
> > machine check handler cover that sufficiently?
>
> I suspect so, but I'd like to verify. Do you know how I could exercise
> this path in qemu or LPAR?

I have some machine check injection patches for qemu but they never got
merged... I can point you to them if you need.

Should try get those merged again.

Thanks,
Nick

Re: [PATCH 00/13] RTAS maintenance

[Michael Ellerman]

On Fri, 18 Nov 2022 09:07:38 -0600, Nathan Lynch wrote:
> A collection of loosely-related RTAS code changes, most notably:
> 
> * Fixing misuses of rtas_token() for non-function properties.
> * The stronger validation for sys_rtas() offered by the
>   PPC_RTAS_FILTER config option is always enabled.
> * Improved function token lookups, including efficient "reverse"
>   token-to-name mappings.
> * Static tracepoints for RTAS entry and exit.
> 
> [...]

Patches 1-9 applied to powerpc/next.

[01/13] powerpc/rtas: document rtas_call()
        https://git.kernel.org/powerpc/c/336e2554ec99eb97616004c791ee89abe96bdab2
[02/13] powerpc/rtasd: use correct OF API for event scan rate
        https://git.kernel.org/powerpc/c/b10af504a2015d12c566b6b0a4c7e3b602949eeb
[03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()
        https://git.kernel.org/powerpc/c/ed2213bfb192ab51f09f12e9b49b5d482c6493f3
[04/13] powerpc/rtas: avoid scheduling in rtas_os_term()
        https://git.kernel.org/powerpc/c/6c606e57eecc37d6b36d732b1ff7e55b7dc32dd4
[05/13] powerpc/pseries/eeh: use correct API for error log size
        https://git.kernel.org/powerpc/c/9aafbfa5f57a4b75bafd3bed0191e8429c5fa618
[06/13] powerpc/rtas: clean up rtas_error_log_max initialization
        https://git.kernel.org/powerpc/c/c67a0e411d0ffe0648fe84e25e9f899ce770feb3
[07/13] powerpc/rtas: clean up includes
        https://git.kernel.org/powerpc/c/9581f8a00777a073fdd8146659a51ca007cae8d6
[08/13] powerpc/rtas: define pr_fmt and convert printk call sites
        https://git.kernel.org/powerpc/c/f975b6559bac510f1b1b39637997bb240f0a9969
[09/13] powerpc/rtas: mandate RTAS syscall filtering
        https://git.kernel.org/powerpc/c/98c738c8cee6e5a58d4060862e2f8cf3cdc8a328

cheers