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

[Nathan Lynch <nathanl@linux.ibm.com>]

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 = {