Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

+ 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

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

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

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Heiko Carstens]

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?

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Sven Schnelle]

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.