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,
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
