Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
Heiko Carstens writes: > So, the in both variants s390 provides nearly identical data. The only > difference is that for FL_SAVE_REGS the program status word mask is > missing; therefore it is not possible to figure out the condition code > or if interrupts were enabled/disabled. > > Vasily, Sven, I think we have two options here: > > - don't provide sane psw mask contents at all and say (again) that > ptregs contents are identical > > - provide (finally) a full psw mask contents using epsw, and indicate > validity with a flags bit in pt_regs > > I would vote for the second option, even though epsw is slow. But this > is about the third or fourth time this came up in different > contexts. So I'd guess we should go for the slow but complete > solution. Opinions? Given that this only affects ftrace_regs_caller, i would also vote for the second option.
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
On Tue, Feb 15, 2022 at 09:55:52PM +0530, Naveen N. Rao wrote:
> > > > > > > I think this is wrong. We need to differentiate
> > > > > > > between ftrace_caller() and ftrace_regs_caller()
> > > > > > > here, and only return pt_regs if coming in through
> > > > > > > ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
> > > > > >
> > > > > > Not sure I follow you.
> > > > > >
> > > > > > This is based on 5740a7c71ab6 ("s390/ftrace: add
> > > > > > HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
> > > > > >
> > > > > > It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS,
> > > > > > have the regs also with ftrace_caller().
> > > > > >
> > > > > > Sure you only have the params, but that's the same on
> > > > > > s390, so what did I miss ?
>
> Steven has explained the rationale for this in his other response:
> https://lore.kernel.org/all/20220215093849.556d5...@gandalf.local.home/
Thanks for this pointer, this clarifies a couple of things!
> > > > It looks like s390 is special since it apparently saves all
> > > > registers even for ftrace_caller:
> > > > https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
> > >
> > > It is not what I understand from their code, see
> > > https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
> > >
> > >
> > > They have a common macro called with argument 'allregs' which is set
> > > to 0 for ftrace_caller() and 1 for ftrace_regs_caller().
> > > When allregs == 1, the macro seems to save more.
> > >
> > > But ok, I can do like x86, but I need a trick to know whether
> > > FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
> > > Any idea what the condition can be for powerpc ?
>
> We'll need to explicitly zero-out something in pt_regs in ftrace_caller().
> We can probably use regs->msr since we don't expect it to be zero when saved
> from ftrace_regs_caller().
> >
> > Finally, it looks like this change is done via commit 894979689d3a
> > ("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
> > implementations") four hours the same day after the implementation of
> > arch_ftrace_get_regs()
> >
> > They may have forgotten to change arch_ftrace_get_regs() which was added
> > in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > support") with the assumption that ftrace_caller and ftrace_regs_caller
> > where identical.
>
> Indeed, good find!
Thank you for bringing this up!
So, the in both variants s390 provides nearly identical data. The only
difference is that for FL_SAVE_REGS the program status word mask is
missing; therefore it is not possible to figure out the condition code
or if interrupts were enabled/disabled.
Vasily, Sven, I think we have two options here:
- don't provide sane psw mask contents at all and say (again) that
ptregs contents are identical
- provide (finally) a full psw mask contents using epsw, and indicate
validity with a flags bit in pt_regs
I would vote for the second option, even though epsw is slow. But this
is about the third or fourth time this came up in different
contexts. So I'd guess we should go for the slow but complete
solution. Opinions?
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
Steven Rostedt wrote:
On Tue, 15 Feb 2022 19:06:48 +0530
"Naveen N. Rao" wrote:
As I understand it, the reason ftrace_get_regs() was introduced was to
be able to only return the pt_regs, if _all_ registers were saved into
it, which we don't do when coming in through ftrace_caller(). See the
x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for
arguments to be passed in to ftrace_regs by default"), which returns
pt_regs conditionally.
I can give you the history of ftrace_caller and ftrace_regs_caller.
ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
The new fentry which happens at the start of the function, whereas mcount
happens after the stack frame is set up, may change the rules on some
architectures.
As for ftrace_regs_caller, that was created for kprobes. As the majority of
kprobes were added at the start of the function, it made sense to hook into
ftrace as the ftrace trampoline call is much faster than taking a
breakpoint interrupt. But to keep compatibility with breakpoint
interrupts, we needed to fill in all the registers, and make it act just
like a breakpoint interrupt.
I've been wanting to record function parameters, and because the ftrace
trampoline must at a minimum save the function parameters before calling
the ftrace callbacks, all the information for those parameters were being
saved but were never exposed to the ftrace callbacks. I created the the
DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
just the parameters filled in, but that was criticized as it could be
confusing where the non filled in pt_regs might be used and thinking they
are legitimate. So I created ftrace_regs that would give you just the
function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
not, it will give you a NULL pointer.
The first user to use the args was live kernel patching, as they only need
that and the return pointer.
Thanks, that helps.
- Naveen
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
Christophe Leroy wrote:
+ S390 people
Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
Michael Ellerman wrote:
Christophe Leroy writes:
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
Christophe Leroy wrote:
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
select HAVE_DYNAMIC_FTRACE
+ select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL ||
PPC32
select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL ||
PPC32
select HAVE_EBPF_JIT
select HAVE_EFFICIENT_UNALIGNED_ACCESS if
!(CPU_LITTLE_ENDIAN && POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h
b/arch/powerpc/include/asm/ftrace.h
index b3f6184f77ea..45c3d6f11daa 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -22,6 +22,23 @@ static inline unsigned long
ftrace_call_adjust(unsigned long addr)
struct dyn_arch_ftrace {
struct module *mod;
};
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+ struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs
*arch_ftrace_get_regs(struct ftrace_regs *fregs)
+{
+ return >regs;
+}
I think this is wrong. We need to differentiate between
ftrace_caller() and ftrace_regs_caller() here, and only return
pt_regs if coming in through ftrace_regs_caller() (i.e.,
FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs
also with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what
did I miss ?
Steven has explained the rationale for this in his other response:
https://lore.kernel.org/all/20220215093849.556d5...@gandalf.local.home/
It looks like s390 is special since it apparently saves all registers
even for ftrace_caller:
https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
It is not what I understand from their code, see
https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
They have a common macro called with argument 'allregs' which is set to
0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.
But ok, I can do like x86, but I need a trick to know whether
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?
We'll need to explicitly zero-out something in pt_regs in
ftrace_caller(). We can probably use regs->msr since we don't expect it
to be zero when saved from ftrace_regs_caller().
Finally, it looks like this change is done via commit 894979689d3a
("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
implementations") four hours the same day after the implementation of
arch_ftrace_get_regs()
They may have forgotten to change arch_ftrace_get_regs() which was added
in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
support") with the assumption that ftrace_caller and ftrace_regs_caller
where identical.
Indeed, good find!
Thanks,
Naveen
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+ S390 people
Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
Michael Ellerman wrote:
Christophe Leroy writes:
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
Christophe Leroy wrote:
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
select HAVE_DYNAMIC_FTRACE
+ select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL ||
PPC32
select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL ||
PPC32
select HAVE_EBPF_JIT
select HAVE_EFFICIENT_UNALIGNED_ACCESS if
!(CPU_LITTLE_ENDIAN && POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h
b/arch/powerpc/include/asm/ftrace.h
index b3f6184f77ea..45c3d6f11daa 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -22,6 +22,23 @@ static inline unsigned long
ftrace_call_adjust(unsigned long addr)
struct dyn_arch_ftrace {
struct module *mod;
};
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+ struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs
*arch_ftrace_get_regs(struct ftrace_regs *fregs)
+{
+ return >regs;
+}
I think this is wrong. We need to differentiate between
ftrace_caller() and ftrace_regs_caller() here, and only return
pt_regs if coming in through ftrace_regs_caller() (i.e.,
FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs
also with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what
did I miss ?
It looks like s390 is special since it apparently saves all registers
even for ftrace_caller:
https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
It is not what I understand from their code, see
https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
They have a common macro called with argument 'allregs' which is set to
0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.
But ok, I can do like x86, but I need a trick to know whether
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?
Finally, it looks like this change is done via commit 894979689d3a
("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
implementations") four hours the same day after the implementation of
arch_ftrace_get_regs()
They may have forgotten to change arch_ftrace_get_regs() which was added
in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
support") with the assumption that ftrace_caller and ftrace_regs_caller
where identical.
Christophe
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
On Tue, 15 Feb 2022 19:06:48 +0530
"Naveen N. Rao" wrote:
> As I understand it, the reason ftrace_get_regs() was introduced was to
> be able to only return the pt_regs, if _all_ registers were saved into
> it, which we don't do when coming in through ftrace_caller(). See the
> x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for
> arguments to be passed in to ftrace_regs by default"), which returns
> pt_regs conditionally.
I can give you the history of ftrace_caller and ftrace_regs_caller.
ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
The new fentry which happens at the start of the function, whereas mcount
happens after the stack frame is set up, may change the rules on some
architectures.
As for ftrace_regs_caller, that was created for kprobes. As the majority of
kprobes were added at the start of the function, it made sense to hook into
ftrace as the ftrace trampoline call is much faster than taking a
breakpoint interrupt. But to keep compatibility with breakpoint
interrupts, we needed to fill in all the registers, and make it act just
like a breakpoint interrupt.
I've been wanting to record function parameters, and because the ftrace
trampoline must at a minimum save the function parameters before calling
the ftrace callbacks, all the information for those parameters were being
saved but were never exposed to the ftrace callbacks. I created the the
DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
just the parameters filled in, but that was criticized as it could be
confusing where the non filled in pt_regs might be used and thinking they
are legitimate. So I created ftrace_regs that would give you just the
function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
not, it will give you a NULL pointer.
The first user to use the args was live kernel patching, as they only need
that and the return pointer.
-- Steve
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
> Michael Ellerman wrote:
>> Christophe Leroy writes:
>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
Christophe Leroy wrote:
> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
> of livepatching.
>
> Also note that powerpc being the last one to convert to
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
> klp_arch_set_pc() on all architectures.
>
> Signed-off-by: Christophe Leroy
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/ftrace.h | 17 +
> arch/powerpc/include/asm/livepatch.h | 4 +---
> 3 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index cdac2115eb00..e2b1792b2aae 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -210,6 +210,7 @@ config PPC
> select HAVE_DEBUG_KMEMLEAK
> select HAVE_DEBUG_STACKOVERFLOW
> select HAVE_DYNAMIC_FTRACE
> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL ||
> PPC32
> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL ||
> PPC32
> select HAVE_EBPF_JIT
> select HAVE_EFFICIENT_UNALIGNED_ACCESS if
> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
> diff --git a/arch/powerpc/include/asm/ftrace.h
> b/arch/powerpc/include/asm/ftrace.h
> index b3f6184f77ea..45c3d6f11daa 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -22,6 +22,23 @@ static inline unsigned long
> ftrace_call_adjust(unsigned long addr)
> struct dyn_arch_ftrace {
> struct module *mod;
> };
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> +struct ftrace_regs {
> + struct pt_regs regs;
> +};
> +
> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
> ftrace_regs *fregs)
> +{
> + return >regs;
> +}
I think this is wrong. We need to differentiate between
ftrace_caller() and ftrace_regs_caller() here, and only return
pt_regs if coming in through ftrace_regs_caller() (i.e.,
FL_SAVE_REGS is set).
>>>
>>> Not sure I follow you.
>>>
>>> This is based on 5740a7c71ab6 ("s390/ftrace: add
>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>
>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs
>>> also with ftrace_caller().
>>>
>>> Sure you only have the params, but that's the same on s390, so what
>>> did I miss ?
>
> It looks like s390 is special since it apparently saves all registers
> even for ftrace_caller:
> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
It is not what I understand from their code, see
https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
They have a common macro called with argument 'allregs' which is set to
0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.
But ok, I can do like x86, but I need a trick to know whether
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?
Thanks
Christophe
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
Michael Ellerman wrote:
Christophe Leroy writes:
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
Christophe Leroy wrote:
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
select HAVE_DYNAMIC_FTRACE
+ select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
select HAVE_EBPF_JIT
select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN
&& POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h
b/arch/powerpc/include/asm/ftrace.h
index b3f6184f77ea..45c3d6f11daa 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -22,6 +22,23 @@ static inline unsigned long
ftrace_call_adjust(unsigned long addr)
struct dyn_arch_ftrace {
struct module *mod;
};
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+ struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
ftrace_regs *fregs)
+{
+ return >regs;
+}
I think this is wrong. We need to differentiate between ftrace_caller()
and ftrace_regs_caller() here, and only return pt_regs if coming in
through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what did
I miss ?
It looks like s390 is special since it apparently saves all registers
even for ftrace_caller:
https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
As I understand it, the reason ftrace_get_regs() was introduced was to
be able to only return the pt_regs, if _all_ registers were saved into
it, which we don't do when coming in through ftrace_caller(). See the
x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for
arguments to be passed in to ftrace_regs by default"), which returns
pt_regs conditionally.
I already have this series in next, I can pull it out, but I'd rather
not.
Yeah, I'm sorry about the late review on this one.
I'll leave it in for now, hopefully you two can agree overnight my time
whether this is a big problem or something we can fix with a fixup
patch.
I think changes to this particular patch can be added as an incremental
patch. If anything, pt_regs won't have all valid registers, but no one
should depend on it without also setting FL_SAVE_REGS anyway.
I was concerned about patch 8 though, where we are missing saving r1
into pt_regs. That gets used in patch 11, and will be used during
unwinding when the function_graph tracer is active. But, this should
still just result in us being unable to unwind the stack, so I think
that can also be an incremental patch.
Thanks,
Naveen
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
Christophe Leroy writes:
> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>> of livepatching.
>>>
>>> Also note that powerpc being the last one to convert to
>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>> klp_arch_set_pc() on all architectures.
>>>
>>> Signed-off-by: Christophe Leroy
>>> ---
>>> arch/powerpc/Kconfig | 1 +
>>> arch/powerpc/include/asm/ftrace.h | 17 +
>>> arch/powerpc/include/asm/livepatch.h | 4 +---
>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index cdac2115eb00..e2b1792b2aae 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -210,6 +210,7 @@ config PPC
>>> select HAVE_DEBUG_KMEMLEAK
>>> select HAVE_DEBUG_STACKOVERFLOW
>>> select HAVE_DYNAMIC_FTRACE
>>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
>>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
>>> select HAVE_EBPF_JIT
>>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN
>>> && POWER7_CPU)
>>> diff --git a/arch/powerpc/include/asm/ftrace.h
>>> b/arch/powerpc/include/asm/ftrace.h
>>> index b3f6184f77ea..45c3d6f11daa 100644
>>> --- a/arch/powerpc/include/asm/ftrace.h
>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>> @@ -22,6 +22,23 @@ static inline unsigned long
>>> ftrace_call_adjust(unsigned long addr)
>>> struct dyn_arch_ftrace {
>>> struct module *mod;
>>> };
>>> +
>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>> +struct ftrace_regs {
>>> + struct pt_regs regs;
>>> +};
>>> +
>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
>>> ftrace_regs *fregs)
>>> +{
>>> + return >regs;
>>> +}
>>
>> I think this is wrong. We need to differentiate between ftrace_caller()
>> and ftrace_regs_caller() here, and only return pt_regs if coming in
>> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
>
> Not sure I follow you.
>
> This is based on 5740a7c71ab6 ("s390/ftrace: add
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>
> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
> with ftrace_caller().
>
> Sure you only have the params, but that's the same on s390, so what did
> I miss ?
I already have this series in next, I can pull it out, but I'd rather
not.
I'll leave it in for now, hopefully you two can agree overnight my time
whether this is a big problem or something we can fix with a fixup
patch.
>>> +static __always_inline void ftrace_instruction_pointer_set(struct
>>> ftrace_regs *fregs,
>>> + unsigned long ip)
>>> +{
>>> + regs_set_return_ip(>regs, ip);
>>
>> Should we use that helper here? regs_set_return_ip() also updates some
>> other state related to taking interrupts and I don't think it makes
>> sense for use with ftrace.
>
>
> Today we have:
>
> static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
> long ip)
> {
> struct pt_regs *regs = ftrace_get_regs(fregs);
>
> regs_set_return_ip(regs, ip);
> }
>
>
> Which like x86 and s390 becomes:
>
> static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
> long ip)
> {
> ftrace_instruction_pointer_set(fregs, ip);
> }
>
>
>
> That's the reason why I've been using regs_set_return_ip(). Do you think
> it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
>
> That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR
> registers if they are still valid")
It's not wrong, but I think it's unnecessary. We need to use
regs_set_return_ip() if we're changing the regs->ip of an interrupt
frame, so that the interrupt return code will reload it.
But AIUI in this case we're not doing that, we're changing the regs->ip
of a pt_regs provided by ftrace, which shouldn't ever be an interrupt
frame.
So it's not a bug to use regs_set_return_ip(), but it is unncessary and
means we'll reload the interrupt state unnecessarily on the next
interrupt return.
cheers
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>> of livepatching.
>>
>> Also note that powerpc being the last one to convert to
>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>> klp_arch_set_pc() on all architectures.
>>
>> Signed-off-by: Christophe Leroy
>> ---
>> arch/powerpc/Kconfig | 1 +
>> arch/powerpc/include/asm/ftrace.h | 17 +
>> arch/powerpc/include/asm/livepatch.h | 4 +---
>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index cdac2115eb00..e2b1792b2aae 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -210,6 +210,7 @@ config PPC
>> select HAVE_DEBUG_KMEMLEAK
>> select HAVE_DEBUG_STACKOVERFLOW
>> select HAVE_DYNAMIC_FTRACE
>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
>> select HAVE_EBPF_JIT
>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN
>> && POWER7_CPU)
>> diff --git a/arch/powerpc/include/asm/ftrace.h
>> b/arch/powerpc/include/asm/ftrace.h
>> index b3f6184f77ea..45c3d6f11daa 100644
>> --- a/arch/powerpc/include/asm/ftrace.h
>> +++ b/arch/powerpc/include/asm/ftrace.h
>> @@ -22,6 +22,23 @@ static inline unsigned long
>> ftrace_call_adjust(unsigned long addr)
>> struct dyn_arch_ftrace {
>> struct module *mod;
>> };
>> +
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>> +struct ftrace_regs {
>> + struct pt_regs regs;
>> +};
>> +
>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
>> ftrace_regs *fregs)
>> +{
>> + return >regs;
>> +}
>
> I think this is wrong. We need to differentiate between ftrace_caller()
> and ftrace_regs_caller() here, and only return pt_regs if coming in
> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what did
I miss ?
>
>> +
>> +static __always_inline void ftrace_instruction_pointer_set(struct
>> ftrace_regs *fregs,
>> + unsigned long ip)
>> +{
>> + regs_set_return_ip(>regs, ip);
>
> Should we use that helper here? regs_set_return_ip() also updates some
> other state related to taking interrupts and I don't think it makes
> sense for use with ftrace.
>
Today we have:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
struct pt_regs *regs = ftrace_get_regs(fregs);
regs_set_return_ip(regs, ip);
}
Which like x86 and s390 becomes:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
ftrace_instruction_pointer_set(fregs, ip);
}
That's the reason why I've been using regs_set_return_ip(). Do you think
it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR
registers if they are still valid")
Christophe
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
Christophe Leroy wrote:
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h| 17 +
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
select HAVE_DYNAMIC_FTRACE
+ select HAVE_DYNAMIC_FTRACE_WITH_ARGSif MPROFILE_KERNEL || PPC32
select HAVE_DYNAMIC_FTRACE_WITH_REGSif MPROFILE_KERNEL || PPC32
select HAVE_EBPF_JIT
select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN &&
POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h
b/arch/powerpc/include/asm/ftrace.h
index b3f6184f77ea..45c3d6f11daa 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long
addr)
struct dyn_arch_ftrace {
struct module *mod;
};
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+ struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs
*fregs)
+{
+ return >regs;
+}
I think this is wrong. We need to differentiate between ftrace_caller()
and ftrace_regs_caller() here, and only return pt_regs if coming in
through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
+
+static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs
*fregs,
+ unsigned long ip)
+{
+ regs_set_return_ip(>regs, ip);
Should we use that helper here? regs_set_return_ip() also updates some
other state related to taking interrupts and I don't think it makes
sense for use with ftrace.
- Naveen
Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
On Mon, 20 Dec 2021, Christophe Leroy wrote: > Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call > of livepatching. > > Also note that powerpc being the last one to convert to > CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove > klp_arch_set_pc() on all architectures. Correct. We could replace it ftrace_instruction_pointer_set() and that is it. In fact, livepatch.h in both arch/x86/include/asm/ and arch/s390/include/asm/ could be removed with that. On the other hand, there is arm64 live patching support being worked on and I am not sure what their plans about DYNAMIC_FTRACE_WITH_ARGS are. The above would make it a prerequisite. Adding CCs... you can find the whole thread at https://lore.kernel.org/all/cover.1640017960.git.christophe.le...@csgroup.eu/ Miroslav
[PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h| 17 +
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
select HAVE_DYNAMIC_FTRACE
+ select HAVE_DYNAMIC_FTRACE_WITH_ARGSif MPROFILE_KERNEL || PPC32
select HAVE_DYNAMIC_FTRACE_WITH_REGSif MPROFILE_KERNEL || PPC32
select HAVE_EBPF_JIT
select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN &&
POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h
b/arch/powerpc/include/asm/ftrace.h
index b3f6184f77ea..45c3d6f11daa 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long
addr)
struct dyn_arch_ftrace {
struct module *mod;
};
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+ struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs
*fregs)
+{
+ return >regs;
+}
+
+static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs
*fregs,
+ unsigned long ip)
+{
+ regs_set_return_ip(>regs, ip);
+}
+#endif
#endif /* __ASSEMBLY__ */
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
diff --git a/arch/powerpc/include/asm/livepatch.h
b/arch/powerpc/include/asm/livepatch.h
index 37af961eb74c..6f10de6af6e3 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -14,9 +14,7 @@
#ifdef CONFIG_LIVEPATCH
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
{
- struct pt_regs *regs = ftrace_get_regs(fregs);
-
- regs_set_return_ip(regs, ip);
+ ftrace_instruction_pointer_set(fregs, ip);
}
#define klp_get_ftrace_location klp_get_ftrace_location
--
2.33.1
