Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 15 Feb 2022 21:55:52 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Subject: Re: [PATCH v2 09/13] powerpc/ftrace: Implement
 CONFIG_DYNAMIC_FTRACE_WITH_ARGS
To:     Christophe Leroy <christophe.leroy@csgroup.eu>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Jiri Kosina <jikos@kernel.org>,
        Joe Lawrence <joe.lawrence@redhat.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>,
        Ingo Molnar <mingo@redhat.com>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Petr Mladek <pmladek@suse.com>,
        Steven Rostedt <rostedt@goodmis.org>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "live-patching@vger.kernel.org" <live-patching@vger.kernel.org>
References: <cover.1640017960.git.christophe.leroy@csgroup.eu>
        <5831f711a778fcd6eb51eb5898f1faae4378b35b.1640017960.git.christophe.leroy@csgroup.eu>
        <1644852011.qg7ud9elo2.naveen@linux.ibm.com>
        <1b28f52a-f8b7-6b5c-e726-feac4123517d@csgroup.eu>
        <875ypgo0f3.fsf@mpe.ellerman.id.au>
        <1644930705.g64na2kgvd.naveen@linux.ibm.com>
        <6dc50f09-4d14-afa2-d2a1-34b72b880edf@csgroup.eu>
        <5c7b5334-6071-f131-a509-9a49ca3d628c@csgroup.eu>
In-Reply-To: <5c7b5334-6071-f131-a509-9a49ca3d628c@csgroup.eu>
User-Agent: astroid/4d6b06ad (https://github.com/astroidmail/astroid)
Message-Id: <1644941712.lqdstzo09z.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

Christophe Leroy wrote:
> + S390 people
>=20
> Le 15/02/2022 =C3=A0 15:28, Christophe Leroy a =C3=A9crit=C2=A0:
>>=20
>>=20
>> Le 15/02/2022 =C3=A0 14:36, Naveen N. Rao a =C3=A9crit=C2=A0:
>>> Michael Ellerman wrote:
>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>>> Le 14/02/2022 =C3=A0 16:25, Naveen N. Rao a =C3=A9crit=C2=A0:
>>>>>> 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>
>>>>>>> ---
>>>>>>> =C2=A0arch/powerpc/Kconfig=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 1 +
>>>>>>> =C2=A0arch/powerpc/include/asm/ftrace.h=C2=A0=C2=A0=C2=A0 | 17 ++++=
+++++++++++++
>>>>>>> =C2=A0arch/powerpc/include/asm/livepatch.h |=C2=A0 4 +---
>>>>>>> =C2=A03 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
>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_DEBUG_KMEMLEAK
>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_DEBUG_STACKOVERFLOW
>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_DYNAMIC_FTRACE
>>>>>>> +=C2=A0=C2=A0=C2=A0 select HAVE_DYNAMIC_FTRACE_WITH_ARGS=C2=A0=C2=
=A0=C2=A0 if MPROFILE_KERNEL ||=20
>>>>>>> PPC32
>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_DYNAMIC_FTRACE_WITH_REGS=C2=A0=
=C2=A0=C2=A0 if MPROFILE_KERNEL ||=20
>>>>>>> PPC32
>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_EBPF_JIT
>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_EFFICIENT_UNALIGNED_ACCESS=C2=
=A0=C2=A0=C2=A0 if=20
>>>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h=20
>>>>>>> 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=20
>>>>>>> ftrace_call_adjust(unsigned long addr)
>>>>>>> =C2=A0struct dyn_arch_ftrace {
>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 struct module *mod;
>>>>>>> =C2=A0};
>>>>>>> +
>>>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>>>> +struct ftrace_regs {
>>>>>>> +=C2=A0=C2=A0=C2=A0 struct pt_regs regs;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static __always_inline struct pt_regs=20
>>>>>>> *arch_ftrace_get_regs(struct ftrace_regs *fregs)
>>>>>>> +{
>>>>>>> +=C2=A0=C2=A0=C2=A0 return &fregs->regs;
>>>>>>> +}
>>>>>>
>>>>>> I think this is wrong. We need to differentiate between=20
>>>>>> ftrace_caller() and ftrace_regs_caller() here, and only return=20
>>>>>> pt_regs if coming in through ftrace_regs_caller() (i.e.,=20
>>>>>> FL_SAVE_REGS is set).
>>>>>
>>>>> Not sure I follow you.
>>>>>
>>>>> This is based on 5740a7c71ab6 ("s390/ftrace: add=20
>>>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>>>
>>>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs=20
>>>>> also with ftrace_caller().
>>>>>
>>>>> Sure you only have the params, but that's the same on s390, so what=20
>>>>> 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=20
>>> even for ftrace_caller:=20
>>> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
>>=20
>> It is not what I understand from their code, see=20
>> https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcoun=
t.S#L37=20
>>=20
>>=20
>> They have a common macro called with argument 'allregs' which is set to=
=20
>> 0 for ftrace_caller() and 1 for ftrace_regs_caller().
>> When allregs =3D=3D 1, the macro seems to save more.
>>=20
>> But ok, I can do like x86, but I need a trick to know whether=20
>> 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=20
ftrace_caller(). We can probably use regs->msr since we don't expect it=20
to be zero when saved from ftrace_regs_caller().

>>=20
>=20
> Finally, it looks like this change is done  via commit 894979689d3a=20
> ("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller=20
> implementations") four hours the same day after the implementation of=20
> arch_ftrace_get_regs()
>=20
> They may have forgotten to change arch_ftrace_get_regs() which was added=
=20
> in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS=20
> support") with the assumption that ftrace_caller and ftrace_regs_caller=20
> where identical.

Indeed, good find!


Thanks,
Naveen