Re: [PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()
Nathan Lynch writes: > Michael Ellerman writes: >> Nathan Lynch writes: >>> "Nicholas Piggin" 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 > --- > 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 > #include > #include > +#include > #include > > 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: > > => do_enter_rtas (c0045180) > => rtas_call (c0045da8) > => rtas_event_scan (c0049458) > => process_one_work (c01c7618) > => worker_thread (c01c7bd8) > => kthread (c01d6858) > => ret_from_kernel_thread (c000cf5c) >inject-ra-err-1080 [001]78.386947: rtas_input: > ibm,nmi-interlock arguments: >inject-ra-err-1080 [001]78.386950: kernel_stack: > > => do_enter_rtas (c0045180) > => rtas_call_unlocked (c0046ff4) > => pseries_machine_check_realmode (c00e8db8) > => machine_check_early (c00400c4) > => machine_check_early_common (c000836c) > > 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);
Re: [PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()
Michael Ellerman writes: > Nathan Lynch writes: >> "Nicholas Piggin" 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 --- 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 #include #include +#include #include 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: => do_enter_rtas (c0045180) => rtas_call (c0045da8) => rtas_event_scan (c0049458) => process_one_work (c01c7618) => worker_thread (c01c7bd8) => kthread (c01d6858) => ret_from_kernel_thread (c000cf5c) inject-ra-err-1080 [001]78.386947: rtas_input: ibm,nmi-interlock arguments: inject-ra-err-1080 [001]78.386950: kernel_stack: => do_enter_rtas (c0045180) => rtas_call_unlocked (c0046ff4) => pseries_machine_check_realmode (c00e8db8) => machine_check_early (c00400c4) => machine_check_early_common (c000836c) 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()
On Tue Nov 29, 2022 at 9:44 AM AEST, Nathan Lynch wrote: > "Nicholas Piggin" 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 > >> --- > >> 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 > >> #include > >> #include > >> +#include > >> #include > >> > >> 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 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()
Nathan Lynch writes: > "Nicholas Piggin" 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 >>> --- >>> 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 >>> #include >>> #include >>> +#include >>> #include >>> >>> 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()
"Nicholas Piggin" 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 >> --- >> 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 >> #include >> #include >> +#include >> #include >> >> 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()
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 > --- > 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 > #include > #include > +#include > #include > > 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
[PATCH 13/13] powerpc/rtas: place tracepoints in do_enter_rtas()
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 --- 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 #include #include +#include #include 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