| Message ID | 5831f711a778fcd6eb51eb5898f1faae4378b35b.1640017960.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Implement livepatch on PPC32 and more | expand |
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.leroy@csgroup.eu/ Miroslav
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 <christophe.leroy@csgroup.eu> > --- > 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 &fregs->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(&fregs->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
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 <christophe.leroy@csgroup.eu> >> --- >> 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 &fregs->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(&fregs->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
Christophe Leroy <christophe.leroy@csgroup.eu> 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 <christophe.leroy@csgroup.eu> >>> --- >>> 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 &fregs->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(&fregs->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
Michael Ellerman wrote: > Christophe Leroy <christophe.leroy@csgroup.eu> 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 <christophe.leroy@csgroup.eu> >>>> --- >>>> 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 &fregs->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
Le 15/02/2022 à 14:36, Naveen N. Rao a écrit : > Michael Ellerman wrote: >> Christophe Leroy <christophe.leroy@csgroup.eu> 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 <christophe.leroy@csgroup.eu> >>>>> --- >>>>> 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 &fregs->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
On Tue, 15 Feb 2022 19:06:48 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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
+ 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 <christophe.leroy@csgroup.eu> 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 <christophe.leroy@csgroup.eu> >>>>>> --- >>>>>> 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 &fregs->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
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 <christophe.leroy@csgroup.eu> 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 <christophe.leroy@csgroup.eu> >>>>>>> --- >>>>>>> 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 &fregs->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.556d5444@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
Steven Rostedt wrote: > On Tue, 15 Feb 2022 19:06:48 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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
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.556d5444@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?
Heiko Carstens <hca@linux.ibm.com> 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.
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 &fregs->regs; +} + +static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *fregs, + unsigned long ip) +{ + regs_set_return_ip(&fregs->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
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 <christophe.leroy@csgroup.eu> --- 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(-)