Nathan Lynch <nath...@linux.ibm.com> writes:
> Michael Ellerman <m...@ellerman.id.au> writes:
>> Nathan Lynch <nath...@linux.ibm.com> writes:
>>> "Nicholas Piggin" <npig...@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 <nath...@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 = {
