| Message ID | 20221118150751.469393-14-nathanl@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RTAS maintenance | expand |
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
"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?
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
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
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 = {
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 = {
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 = {
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(+)