* [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption
2023-01-12 9:05 [PATCH -next V7 0/7] riscv: Optimize function trace guoren
@ 2023-01-12 9:05 ` guoren
2023-01-12 12:16 ` Mark Rutland
2023-01-12 9:05 ` [PATCH -next V7 2/7] riscv: ftrace: Remove wasted nops for !RISCV_ISA_C guoren
` (7 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: guoren @ 2023-01-12 9:05 UTC (permalink / raw)
To: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, guoren
Cc: linux-riscv, linux-kernel
From: Andy Chiu <andy.chiu@sifive.com>
In RISCV, we must use an AUIPC + JALR pair to encode an immediate,
forming a jump that jumps to an address over 4K. This may cause errors
if we want to enable kernel preemption and remove dependency from
patching code with stop_machine(). For example, if a task was switched
out on auipc. And, if we changed the ftrace function before it was
switched back, then it would jump to an address that has updated 11:0
bits mixing with previous XLEN:12 part.
p: patched area performed by dynamic ftrace
ftrace_prologue:
p| REG_S ra, -SZREG(sp)
p| auipc ra, 0x? ------------> preempted
...
change ftrace function
...
p| jalr -?(ra) <------------- switched back
p| REG_L ra, -SZREG(sp)
func:
xxx
ret
Fixes: afc76b8b8011 ("riscv: Using PATCHABLE_FUNCTION_ENTRY instead of MCOUNT")
Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
arch/riscv/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index e2b656043abf..ee0d39b26794 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -138,7 +138,7 @@ config RISCV
select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
select HAVE_FUNCTION_GRAPH_TRACER
- select HAVE_FUNCTION_TRACER if !XIP_KERNEL
+ select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION
config ARCH_MMAP_RND_BITS_MIN
default 18 if 64BIT
--
2.36.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption
2023-01-12 9:05 ` [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption guoren
@ 2023-01-12 12:16 ` Mark Rutland
2023-01-12 12:57 ` Mark Rutland
2023-01-28 9:37 ` Guo Ren
0 siblings, 2 replies; 44+ messages in thread
From: Mark Rutland @ 2023-01-12 12:16 UTC (permalink / raw)
To: guoren
Cc: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, linux-riscv, linux-kernel
Hi Guo,
On Thu, Jan 12, 2023 at 04:05:57AM -0500, guoren@kernel.org wrote:
> From: Andy Chiu <andy.chiu@sifive.com>
>
> In RISCV, we must use an AUIPC + JALR pair to encode an immediate,
> forming a jump that jumps to an address over 4K. This may cause errors
> if we want to enable kernel preemption and remove dependency from
> patching code with stop_machine(). For example, if a task was switched
> out on auipc. And, if we changed the ftrace function before it was
> switched back, then it would jump to an address that has updated 11:0
> bits mixing with previous XLEN:12 part.
>
> p: patched area performed by dynamic ftrace
> ftrace_prologue:
> p| REG_S ra, -SZREG(sp)
> p| auipc ra, 0x? ------------> preempted
> ...
> change ftrace function
> ...
> p| jalr -?(ra) <------------- switched back
> p| REG_L ra, -SZREG(sp)
> func:
> xxx
> ret
As mentioned on the last posting, I don't think this is sufficient to fix the
issue. I've replied with more detail there:
https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/
Even in a non-preemptible SMP kernel, if one CPU can be in the middle of
executing the ftrace_prologue while another CPU is patching the
ftrace_prologue, you have the exact same issue.
For example, if CPU X is in the prologue fetches the old AUIPC and the new
JALR (because it races with CPU Y modifying those), CPU X will branch to the
wrong address. The race window is much smaller in the absence of preemption,
but it's still there (and will be exacerbated in virtual machines since the
hypervisor can preempt a vCPU at any time).
Note that the above is even assuming that instruction fetches are atomic, which
I'm not sure is the case; for example arm64 has special CMODX / "Concurrent
MODification and eXecutuion of instructions" rules which mean only certain
instructions can be patched atomically.
Either I'm missing something that provides mutual exclusion between the
patching and execution of the ftrace_prologue, or this patch is not sufficient.
Thanks,
Mark.
> Fixes: afc76b8b8011 ("riscv: Using PATCHABLE_FUNCTION_ENTRY instead of MCOUNT")
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
> arch/riscv/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index e2b656043abf..ee0d39b26794 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -138,7 +138,7 @@ config RISCV
> select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
> select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> select HAVE_FUNCTION_GRAPH_TRACER
> - select HAVE_FUNCTION_TRACER if !XIP_KERNEL
> + select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION
>
> config ARCH_MMAP_RND_BITS_MIN
> default 18 if 64BIT
> --
> 2.36.1
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption
2023-01-12 12:16 ` Mark Rutland
@ 2023-01-12 12:57 ` Mark Rutland
2023-01-28 9:45 ` Guo Ren
2023-01-28 9:37 ` Guo Ren
1 sibling, 1 reply; 44+ messages in thread
From: Mark Rutland @ 2023-01-12 12:57 UTC (permalink / raw)
To: guoren
Cc: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, linux-riscv, linux-kernel
On Thu, Jan 12, 2023 at 12:16:02PM +0000, Mark Rutland wrote:
> Hi Guo,
>
> On Thu, Jan 12, 2023 at 04:05:57AM -0500, guoren@kernel.org wrote:
> > From: Andy Chiu <andy.chiu@sifive.com>
> >
> > In RISCV, we must use an AUIPC + JALR pair to encode an immediate,
> > forming a jump that jumps to an address over 4K. This may cause errors
> > if we want to enable kernel preemption and remove dependency from
> > patching code with stop_machine(). For example, if a task was switched
> > out on auipc. And, if we changed the ftrace function before it was
> > switched back, then it would jump to an address that has updated 11:0
> > bits mixing with previous XLEN:12 part.
> >
> > p: patched area performed by dynamic ftrace
> > ftrace_prologue:
> > p| REG_S ra, -SZREG(sp)
> > p| auipc ra, 0x? ------------> preempted
> > ...
> > change ftrace function
> > ...
> > p| jalr -?(ra) <------------- switched back
> > p| REG_L ra, -SZREG(sp)
> > func:
> > xxx
> > ret
>
> As mentioned on the last posting, I don't think this is sufficient to fix the
> issue. I've replied with more detail there:
>
> https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/
>
> Even in a non-preemptible SMP kernel, if one CPU can be in the middle of
> executing the ftrace_prologue while another CPU is patching the
> ftrace_prologue, you have the exact same issue.
>
> For example, if CPU X is in the prologue fetches the old AUIPC and the new
> JALR (because it races with CPU Y modifying those), CPU X will branch to the
> wrong address. The race window is much smaller in the absence of preemption,
> but it's still there (and will be exacerbated in virtual machines since the
> hypervisor can preempt a vCPU at any time).
With that in mind, I think your current implementation of ftrace_make_call()
and ftrace_make_nop() have a simlar bug. A caller might execute:
NOP // not yet patched to AUIPC
< AUIPC and JALR instructions both patched >
JALR
... and go to the wrong place.
Assuming individual instruction fetches are atomic, and that you only ever
branch to the same trampoline, you could fix that by always leaving the AUIPC
in place, so that you only patch the JALR to enable/disable the callsite.
Depending on your calling convention, if you have two free GPRs, you might be
able to avoid the stacking of RA by always saving it to a GPR in the callsite,
using a different GPR for the address generation, and having the ftrace
trampoline restore the original RA value, e.g.
MV GPR1, ra
AUIPC GPR2, high_bits_of(ftrace_caller)
JALR ra, high_bits(GPR2) // only patch this
... which'd save an instruction per callsite.
Thanks,
Mark.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption
2023-01-12 12:57 ` Mark Rutland
@ 2023-01-28 9:45 ` Guo Ren
0 siblings, 0 replies; 44+ messages in thread
From: Guo Ren @ 2023-01-28 9:45 UTC (permalink / raw)
To: Mark Rutland
Cc: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, linux-riscv, linux-kernel
On Thu, Jan 12, 2023 at 8:57 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Thu, Jan 12, 2023 at 12:16:02PM +0000, Mark Rutland wrote:
> > Hi Guo,
> >
> > On Thu, Jan 12, 2023 at 04:05:57AM -0500, guoren@kernel.org wrote:
> > > From: Andy Chiu <andy.chiu@sifive.com>
> > >
> > > In RISCV, we must use an AUIPC + JALR pair to encode an immediate,
> > > forming a jump that jumps to an address over 4K. This may cause errors
> > > if we want to enable kernel preemption and remove dependency from
> > > patching code with stop_machine(). For example, if a task was switched
> > > out on auipc. And, if we changed the ftrace function before it was
> > > switched back, then it would jump to an address that has updated 11:0
> > > bits mixing with previous XLEN:12 part.
> > >
> > > p: patched area performed by dynamic ftrace
> > > ftrace_prologue:
> > > p| REG_S ra, -SZREG(sp)
> > > p| auipc ra, 0x? ------------> preempted
> > > ...
> > > change ftrace function
> > > ...
> > > p| jalr -?(ra) <------------- switched back
> > > p| REG_L ra, -SZREG(sp)
> > > func:
> > > xxx
> > > ret
> >
> > As mentioned on the last posting, I don't think this is sufficient to fix the
> > issue. I've replied with more detail there:
> >
> > https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/
> >
> > Even in a non-preemptible SMP kernel, if one CPU can be in the middle of
> > executing the ftrace_prologue while another CPU is patching the
> > ftrace_prologue, you have the exact same issue.
> >
> > For example, if CPU X is in the prologue fetches the old AUIPC and the new
> > JALR (because it races with CPU Y modifying those), CPU X will branch to the
> > wrong address. The race window is much smaller in the absence of preemption,
> > but it's still there (and will be exacerbated in virtual machines since the
> > hypervisor can preempt a vCPU at any time).
>
> With that in mind, I think your current implementation of ftrace_make_call()
> and ftrace_make_nop() have a simlar bug. A caller might execute:
>
> NOP // not yet patched to AUIPC
>
> < AUIPC and JALR instructions both patched >
>
> JALR
>
> ... and go to the wrong place.
>
> Assuming individual instruction fetches are atomic, and that you only ever
> branch to the same trampoline, you could fix that by always leaving the AUIPC
> in place, so that you only patch the JALR to enable/disable the callsite.
Yes, the same trampoline is one of the antidotes.
>
> Depending on your calling convention, if you have two free GPRs, you might be
> able to avoid the stacking of RA by always saving it to a GPR in the callsite,
> using a different GPR for the address generation, and having the ftrace
> trampoline restore the original RA value, e.g.
>
> MV GPR1, ra
> AUIPC GPR2, high_bits_of(ftrace_caller)
> JALR ra, high_bits(GPR2) // only patch this
I think you mean temp registers here. We are at the prologue of a
function, so we have all of them.
But why do you need another "MV GPR1, ra"
AUIPC GPR2, high_bits_of(ftrace_caller)
JALR GPR2, high_bits(GPR2) // only patch this
We could reserve ra on the trampoline.
MV XX, ra
>
> ... which'd save an instruction per callsite.
>
> Thanks,
> Mark.
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption
2023-01-12 12:16 ` Mark Rutland
2023-01-12 12:57 ` Mark Rutland
@ 2023-01-28 9:37 ` Guo Ren
2023-01-30 10:54 ` Mark Rutland
1 sibling, 1 reply; 44+ messages in thread
From: Guo Ren @ 2023-01-28 9:37 UTC (permalink / raw)
To: Mark Rutland
Cc: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, linux-riscv, linux-kernel
On Thu, Jan 12, 2023 at 8:16 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Guo,
>
> On Thu, Jan 12, 2023 at 04:05:57AM -0500, guoren@kernel.org wrote:
> > From: Andy Chiu <andy.chiu@sifive.com>
> >
> > In RISCV, we must use an AUIPC + JALR pair to encode an immediate,
> > forming a jump that jumps to an address over 4K. This may cause errors
> > if we want to enable kernel preemption and remove dependency from
> > patching code with stop_machine(). For example, if a task was switched
> > out on auipc. And, if we changed the ftrace function before it was
> > switched back, then it would jump to an address that has updated 11:0
> > bits mixing with previous XLEN:12 part.
> >
> > p: patched area performed by dynamic ftrace
> > ftrace_prologue:
> > p| REG_S ra, -SZREG(sp)
> > p| auipc ra, 0x? ------------> preempted
> > ...
> > change ftrace function
> > ...
> > p| jalr -?(ra) <------------- switched back
> > p| REG_L ra, -SZREG(sp)
> > func:
> > xxx
> > ret
>
> As mentioned on the last posting, I don't think this is sufficient to fix the
> issue. I've replied with more detail there:
>
> https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/
>
> Even in a non-preemptible SMP kernel, if one CPU can be in the middle of
> executing the ftrace_prologue while another CPU is patching the
> ftrace_prologue, you have the exact same issue.
>
> For example, if CPU X is in the prologue fetches the old AUIPC and the new
> JALR (because it races with CPU Y modifying those), CPU X will branch to the
> wrong address. The race window is much smaller in the absence of preemption,
> but it's still there (and will be exacerbated in virtual machines since the
> hypervisor can preempt a vCPU at any time).
>
> Note that the above is even assuming that instruction fetches are atomic, which
> I'm not sure is the case; for example arm64 has special CMODX / "Concurrent
> MODification and eXecutuion of instructions" rules which mean only certain
> instructions can be patched atomically.
>
> Either I'm missing something that provides mutual exclusion between the
> patching and execution of the ftrace_prologue, or this patch is not sufficient.
This patch is sufficient because riscv isn't the same as arm64. It
uses default arch_ftrace_update_code, which uses stop_machine.
See kernel/trace/ftrace.c:
void __weak arch_ftrace_update_code(int command)
{
ftrace_run_stop_machine(command);
}
ps:
Yes, it's not good, and it's expensive.
>
> Thanks,
> Mark.
>
> > Fixes: afc76b8b8011 ("riscv: Using PATCHABLE_FUNCTION_ENTRY instead of MCOUNT")
> > Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > ---
> > arch/riscv/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index e2b656043abf..ee0d39b26794 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -138,7 +138,7 @@ config RISCV
> > select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
> > select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> > select HAVE_FUNCTION_GRAPH_TRACER
> > - select HAVE_FUNCTION_TRACER if !XIP_KERNEL
> > + select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION
> >
> > config ARCH_MMAP_RND_BITS_MIN
> > default 18 if 64BIT
> > --
> > 2.36.1
> >
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption
2023-01-28 9:37 ` Guo Ren
@ 2023-01-30 10:54 ` Mark Rutland
2023-02-04 1:19 ` Guo Ren
0 siblings, 1 reply; 44+ messages in thread
From: Mark Rutland @ 2023-01-30 10:54 UTC (permalink / raw)
To: Guo Ren
Cc: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, linux-riscv, linux-kernel
On Sat, Jan 28, 2023 at 05:37:46PM +0800, Guo Ren wrote:
> On Thu, Jan 12, 2023 at 8:16 PM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Hi Guo,
> >
> > On Thu, Jan 12, 2023 at 04:05:57AM -0500, guoren@kernel.org wrote:
> > > From: Andy Chiu <andy.chiu@sifive.com>
> > >
> > > In RISCV, we must use an AUIPC + JALR pair to encode an immediate,
> > > forming a jump that jumps to an address over 4K. This may cause errors
> > > if we want to enable kernel preemption and remove dependency from
> > > patching code with stop_machine(). For example, if a task was switched
> > > out on auipc. And, if we changed the ftrace function before it was
> > > switched back, then it would jump to an address that has updated 11:0
> > > bits mixing with previous XLEN:12 part.
> > >
> > > p: patched area performed by dynamic ftrace
> > > ftrace_prologue:
> > > p| REG_S ra, -SZREG(sp)
> > > p| auipc ra, 0x? ------------> preempted
> > > ...
> > > change ftrace function
> > > ...
> > > p| jalr -?(ra) <------------- switched back
> > > p| REG_L ra, -SZREG(sp)
> > > func:
> > > xxx
> > > ret
> >
> > As mentioned on the last posting, I don't think this is sufficient to fix the
> > issue. I've replied with more detail there:
> >
> > https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/
> >
> > Even in a non-preemptible SMP kernel, if one CPU can be in the middle of
> > executing the ftrace_prologue while another CPU is patching the
> > ftrace_prologue, you have the exact same issue.
> >
> > For example, if CPU X is in the prologue fetches the old AUIPC and the new
> > JALR (because it races with CPU Y modifying those), CPU X will branch to the
> > wrong address. The race window is much smaller in the absence of preemption,
> > but it's still there (and will be exacerbated in virtual machines since the
> > hypervisor can preempt a vCPU at any time).
> >
> > Note that the above is even assuming that instruction fetches are atomic, which
> > I'm not sure is the case; for example arm64 has special CMODX / "Concurrent
> > MODification and eXecutuion of instructions" rules which mean only certain
> > instructions can be patched atomically.
> >
> > Either I'm missing something that provides mutual exclusion between the
> > patching and execution of the ftrace_prologue, or this patch is not sufficient.
> This patch is sufficient because riscv isn't the same as arm64. It
> uses default arch_ftrace_update_code, which uses stop_machine.
> See kernel/trace/ftrace.c:
> void __weak arch_ftrace_update_code(int command)
> {
> ftrace_run_stop_machine(command);
> }
Ah; sorry, I had misunderstood here, since the commit message spoke in terms of
removing that.
As long as stop_machine() is used I agree this is safe; sorry for the noise.
> ps:
> Yes, it's not good, and it's expensive.
We can't have everything! :)
Thanks,
Mark.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption
2023-01-30 10:54 ` Mark Rutland
@ 2023-02-04 1:19 ` Guo Ren
0 siblings, 0 replies; 44+ messages in thread
From: Guo Ren @ 2023-02-04 1:19 UTC (permalink / raw)
To: palmer, Mark Rutland
Cc: anup, paul.walmsley, conor.dooley, heiko, rostedt, mhiramat,
jolsa, bp, jpoimboe, suagrfillet, andy.chiu, e.shatokhin,
linux-riscv, linux-kernel
On Mon, Jan 30, 2023 at 6:54 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Sat, Jan 28, 2023 at 05:37:46PM +0800, Guo Ren wrote:
> > On Thu, Jan 12, 2023 at 8:16 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > Hi Guo,
> > >
> > > On Thu, Jan 12, 2023 at 04:05:57AM -0500, guoren@kernel.org wrote:
> > > > From: Andy Chiu <andy.chiu@sifive.com>
> > > >
> > > > In RISCV, we must use an AUIPC + JALR pair to encode an immediate,
> > > > forming a jump that jumps to an address over 4K. This may cause errors
> > > > if we want to enable kernel preemption and remove dependency from
> > > > patching code with stop_machine(). For example, if a task was switched
> > > > out on auipc. And, if we changed the ftrace function before it was
> > > > switched back, then it would jump to an address that has updated 11:0
> > > > bits mixing with previous XLEN:12 part.
> > > >
> > > > p: patched area performed by dynamic ftrace
> > > > ftrace_prologue:
> > > > p| REG_S ra, -SZREG(sp)
> > > > p| auipc ra, 0x? ------------> preempted
> > > > ...
> > > > change ftrace function
> > > > ...
> > > > p| jalr -?(ra) <------------- switched back
> > > > p| REG_L ra, -SZREG(sp)
> > > > func:
> > > > xxx
> > > > ret
> > >
> > > As mentioned on the last posting, I don't think this is sufficient to fix the
> > > issue. I've replied with more detail there:
> > >
> > > https://lore.kernel.org/lkml/Y7%2F3hoFjS49yy52W@FVFF77S0Q05N/
> > >
> > > Even in a non-preemptible SMP kernel, if one CPU can be in the middle of
> > > executing the ftrace_prologue while another CPU is patching the
> > > ftrace_prologue, you have the exact same issue.
> > >
> > > For example, if CPU X is in the prologue fetches the old AUIPC and the new
> > > JALR (because it races with CPU Y modifying those), CPU X will branch to the
> > > wrong address. The race window is much smaller in the absence of preemption,
> > > but it's still there (and will be exacerbated in virtual machines since the
> > > hypervisor can preempt a vCPU at any time).
> > >
> > > Note that the above is even assuming that instruction fetches are atomic, which
> > > I'm not sure is the case; for example arm64 has special CMODX / "Concurrent
> > > MODification and eXecutuion of instructions" rules which mean only certain
> > > instructions can be patched atomically.
> > >
> > > Either I'm missing something that provides mutual exclusion between the
> > > patching and execution of the ftrace_prologue, or this patch is not sufficient.
> > This patch is sufficient because riscv isn't the same as arm64. It
> > uses default arch_ftrace_update_code, which uses stop_machine.
> > See kernel/trace/ftrace.c:
> > void __weak arch_ftrace_update_code(int command)
> > {
> > ftrace_run_stop_machine(command);
> > }
>
> Ah; sorry, I had misunderstood here, since the commit message spoke in terms of
> removing that.
>
> As long as stop_machine() is used I agree this is safe; sorry for the noise.
Okay.
Hi Palmer,
Please take Andy's fixup patch. We would continue to find a way for PREEMPTION.
>
> > ps:
> > Yes, it's not good, and it's expensive.
>
> We can't have everything! :)
>
> Thanks,
> Mark.
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH -next V7 2/7] riscv: ftrace: Remove wasted nops for !RISCV_ISA_C
2023-01-12 9:05 [PATCH -next V7 0/7] riscv: Optimize function trace guoren
2023-01-12 9:05 ` [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption guoren
@ 2023-01-12 9:05 ` guoren
2023-01-12 9:05 ` [PATCH -next V7 3/7] riscv: ftrace: Reduce the detour code size to half guoren
` (6 subsequent siblings)
8 siblings, 0 replies; 44+ messages in thread
From: guoren @ 2023-01-12 9:05 UTC (permalink / raw)
To: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, guoren
Cc: linux-riscv, linux-kernel, Guo Ren
From: Guo Ren <guoren@linux.alibaba.com>
When CONFIG_RISCV_ISA_C=n, -fpatchable-function-entry=8 would generate
more nops than we expect. Because it treat nop opcode as 0x00000013
instead of 0x0001.
Dump of assembler code for function dw_pcie_free_msi:
0xffffffff806fce94 <+0>: sd ra,-8(sp)
0xffffffff806fce98 <+4>: auipc ra,0xff90f
0xffffffff806fce9c <+8>: jalr -684(ra) # 0xffffffff8000bbec
<ftrace_caller>
0xffffffff806fcea0 <+12>: ld ra,-8(sp)
0xffffffff806fcea4 <+16>: nop /* wasted */
0xffffffff806fcea8 <+20>: nop /* wasted */
0xffffffff806fceac <+24>: nop /* wasted */
0xffffffff806fceb0 <+28>: nop /* wasted */
0xffffffff806fceb4 <+0>: addi sp,sp,-48
0xffffffff806fceb8 <+4>: sd s0,32(sp)
0xffffffff806fcebc <+8>: sd s1,24(sp)
0xffffffff806fcec0 <+12>: sd s2,16(sp)
0xffffffff806fcec4 <+16>: sd s3,8(sp)
0xffffffff806fcec8 <+20>: sd ra,40(sp)
0xffffffff806fcecc <+24>: addi s0,sp,48
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
arch/riscv/Makefile | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 12d91b0a73d8..ea5a91da6897 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -11,7 +11,11 @@ LDFLAGS_vmlinux :=
ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
LDFLAGS_vmlinux := --no-relax
KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
+ifeq ($(CONFIG_RISCV_ISA_C),y)
CC_FLAGS_FTRACE := -fpatchable-function-entry=8
+else
+ CC_FLAGS_FTRACE := -fpatchable-function-entry=4
+endif
endif
ifeq ($(CONFIG_CMODEL_MEDLOW),y)
--
2.36.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH -next V7 3/7] riscv: ftrace: Reduce the detour code size to half
2023-01-12 9:05 [PATCH -next V7 0/7] riscv: Optimize function trace guoren
2023-01-12 9:05 ` [PATCH -next V7 1/7] riscv: ftrace: Fixup panic by disabling preemption guoren
2023-01-12 9:05 ` [PATCH -next V7 2/7] riscv: ftrace: Remove wasted nops for !RISCV_ISA_C guoren
@ 2023-01-12 9:05 ` guoren
2023-01-16 14:11 ` Evgenii Shatokhin
2023-01-12 9:06 ` [PATCH -next V7 4/7] riscv: ftrace: Add ftrace_graph_func guoren
` (5 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: guoren @ 2023-01-12 9:05 UTC (permalink / raw)
To: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, guoren
Cc: linux-riscv, linux-kernel, Guo Ren
From: Guo Ren <guoren@linux.alibaba.com>
Use a temporary register to reduce the size of detour code from 16 bytes to
8 bytes. The previous implementation is from 'commit afc76b8b8011 ("riscv:
Using PATCHABLE_FUNCTION_ENTRY instead of MCOUNT")'.
Before the patch:
<func_prolog>:
0: REG_S ra, -SZREG(sp)
4: auipc ra, ?
8: jalr ?(ra)
12: REG_L ra, -SZREG(sp)
(func_boddy)
After the patch:
<func_prolog>:
0: auipc t0, ?
4: jalr t0, ?(t0)
(func_boddy)
This patch not just reduces the size of detour code, but also fixes an
important issue:
An Ftrace callback registered with FTRACE_OPS_FL_IPMODIFY flag can
actually change the instruction pointer, e.g. to "replace" the given
kernel function with a new one, which is needed for livepatching, etc.
In this case, the trampoline (ftrace_regs_caller) would not return to
<func_prolog+12> but would rather jump to the new function. So, "REG_L
ra, -SZREG(sp)" would not run and the original return address would not
be restored. The kernel is likely to hang or crash as a result.
This can be easily demonstrated if one tries to "replace", say,
cmdline_proc_show() with a new function with the same signature using
instruction_pointer_set(&fregs->regs, new_func_addr) in the Ftrace
callback.
Link: https://lore.kernel.org/linux-riscv/20221122075440.1165172-1-suagrfillet@gmail.com/
Link: https://lore.kernel.org/linux-riscv/d7d5730b-ebef-68e5-5046-e763e1ee6164@yadro.com/
Co-developed-by: Song Shuai <suagrfillet@gmail.com>
Signed-off-by: Song Shuai <suagrfillet@gmail.com>
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
Cc: Evgenii Shatokhin <e.shatokhin@yadro.com>
---
arch/riscv/Makefile | 4 +-
arch/riscv/include/asm/ftrace.h | 50 +++++++++++++++++++------
arch/riscv/kernel/ftrace.c | 65 ++++++++++-----------------------
arch/riscv/kernel/mcount-dyn.S | 42 ++++++++-------------
4 files changed, 75 insertions(+), 86 deletions(-)
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index ea5a91da6897..3c9aaf67ed79 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -12,9 +12,9 @@ ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
LDFLAGS_vmlinux := --no-relax
KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
ifeq ($(CONFIG_RISCV_ISA_C),y)
- CC_FLAGS_FTRACE := -fpatchable-function-entry=8
-else
CC_FLAGS_FTRACE := -fpatchable-function-entry=4
+else
+ CC_FLAGS_FTRACE := -fpatchable-function-entry=2
endif
endif
diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
index 04dad3380041..9e73922e1e2e 100644
--- a/arch/riscv/include/asm/ftrace.h
+++ b/arch/riscv/include/asm/ftrace.h
@@ -42,6 +42,14 @@ struct dyn_arch_ftrace {
* 2) jalr: setting low-12 offset to ra, jump to ra, and set ra to
* return address (original pc + 4)
*
+ *<ftrace enable>:
+ * 0: auipc t0/ra, 0x?
+ * 4: jalr t0/ra, ?(t0/ra)
+ *
+ *<ftrace disable>:
+ * 0: nop
+ * 4: nop
+ *
* Dynamic ftrace generates probes to call sites, so we must deal with
* both auipc and jalr at the same time.
*/
@@ -52,25 +60,43 @@ struct dyn_arch_ftrace {
#define AUIPC_OFFSET_MASK (0xfffff000)
#define AUIPC_PAD (0x00001000)
#define JALR_SHIFT 20
-#define JALR_BASIC (0x000080e7)
-#define AUIPC_BASIC (0x00000097)
+#define JALR_RA (0x000080e7)
+#define AUIPC_RA (0x00000097)
+#define JALR_T0 (0x000282e7)
+#define AUIPC_T0 (0x00000297)
#define NOP4 (0x00000013)
-#define make_call(caller, callee, call) \
+#define to_jalr_t0(offset) \
+ (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0)
+
+#define to_auipc_t0(offset) \
+ ((offset & JALR_SIGN_MASK) ? \
+ (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_T0) : \
+ ((offset & AUIPC_OFFSET_MASK) | AUIPC_T0))
+
+#define make_call_t0(caller, callee, call) \
do { \
- call[0] = to_auipc_insn((unsigned int)((unsigned long)callee - \
- (unsigned long)caller)); \
- call[1] = to_jalr_insn((unsigned int)((unsigned long)callee - \
- (unsigned long)caller)); \
+ unsigned int offset = \
+ (unsigned long) callee - (unsigned long) caller; \
+ call[0] = to_auipc_t0(offset); \
+ call[1] = to_jalr_t0(offset); \
} while (0)
-#define to_jalr_insn(offset) \
- (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_BASIC)
+#define to_jalr_ra(offset) \
+ (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_RA)
-#define to_auipc_insn(offset) \
+#define to_auipc_ra(offset) \
((offset & JALR_SIGN_MASK) ? \
- (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_BASIC) : \
- ((offset & AUIPC_OFFSET_MASK) | AUIPC_BASIC))
+ (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_RA) : \
+ ((offset & AUIPC_OFFSET_MASK) | AUIPC_RA))
+
+#define make_call_ra(caller, callee, call) \
+do { \
+ unsigned int offset = \
+ (unsigned long) callee - (unsigned long) caller; \
+ call[0] = to_auipc_ra(offset); \
+ call[1] = to_jalr_ra(offset); \
+} while (0)
/*
* Let auipc+jalr be the basic *mcount unit*, so we make it 8 bytes here.
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index 2086f6585773..5bff37af4770 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -55,12 +55,15 @@ static int ftrace_check_current_call(unsigned long hook_pos,
}
static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
- bool enable)
+ bool enable, bool ra)
{
unsigned int call[2];
unsigned int nops[2] = {NOP4, NOP4};
- make_call(hook_pos, target, call);
+ if (ra)
+ make_call_ra(hook_pos, target, call);
+ else
+ make_call_t0(hook_pos, target, call);
/* Replace the auipc-jalr pair at once. Return -EPERM on write error. */
if (patch_text_nosync
@@ -70,42 +73,13 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
return 0;
}
-/*
- * Put 5 instructions with 16 bytes at the front of function within
- * patchable function entry nops' area.
- *
- * 0: REG_S ra, -SZREG(sp)
- * 1: auipc ra, 0x?
- * 2: jalr -?(ra)
- * 3: REG_L ra, -SZREG(sp)
- *
- * So the opcodes is:
- * 0: 0xfe113c23 (sd)/0xfe112e23 (sw)
- * 1: 0x???????? -> auipc
- * 2: 0x???????? -> jalr
- * 3: 0xff813083 (ld)/0xffc12083 (lw)
- */
-#if __riscv_xlen == 64
-#define INSN0 0xfe113c23
-#define INSN3 0xff813083
-#elif __riscv_xlen == 32
-#define INSN0 0xfe112e23
-#define INSN3 0xffc12083
-#endif
-
-#define FUNC_ENTRY_SIZE 16
-#define FUNC_ENTRY_JMP 4
-
int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
- unsigned int call[4] = {INSN0, 0, 0, INSN3};
- unsigned long target = addr;
- unsigned long caller = rec->ip + FUNC_ENTRY_JMP;
+ unsigned int call[2];
- call[1] = to_auipc_insn((unsigned int)(target - caller));
- call[2] = to_jalr_insn((unsigned int)(target - caller));
+ make_call_t0(rec->ip, addr, call);
- if (patch_text_nosync((void *)rec->ip, call, FUNC_ENTRY_SIZE))
+ if (patch_text_nosync((void *)rec->ip, call, MCOUNT_INSN_SIZE))
return -EPERM;
return 0;
@@ -114,15 +88,14 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
unsigned long addr)
{
- unsigned int nops[4] = {NOP4, NOP4, NOP4, NOP4};
+ unsigned int nops[2] = {NOP4, NOP4};
- if (patch_text_nosync((void *)rec->ip, nops, FUNC_ENTRY_SIZE))
+ if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
return -EPERM;
return 0;
}
-
/*
* This is called early on, and isn't wrapped by
* ftrace_arch_code_modify_{prepare,post_process}() and therefor doesn't hold
@@ -144,10 +117,10 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
int ftrace_update_ftrace_func(ftrace_func_t func)
{
int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
- (unsigned long)func, true);
+ (unsigned long)func, true, true);
if (!ret) {
ret = __ftrace_modify_call((unsigned long)&ftrace_regs_call,
- (unsigned long)func, true);
+ (unsigned long)func, true, true);
}
return ret;
@@ -159,16 +132,16 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
unsigned long addr)
{
unsigned int call[2];
- unsigned long caller = rec->ip + FUNC_ENTRY_JMP;
+ unsigned long caller = rec->ip;
int ret;
- make_call(caller, old_addr, call);
+ make_call_t0(caller, old_addr, call);
ret = ftrace_check_current_call(caller, call);
if (ret)
return ret;
- return __ftrace_modify_call(caller, addr, true);
+ return __ftrace_modify_call(caller, addr, true, false);
}
#endif
@@ -203,12 +176,12 @@ int ftrace_enable_ftrace_graph_caller(void)
int ret;
ret = __ftrace_modify_call((unsigned long)&ftrace_graph_call,
- (unsigned long)&prepare_ftrace_return, true);
+ (unsigned long)&prepare_ftrace_return, true, true);
if (ret)
return ret;
return __ftrace_modify_call((unsigned long)&ftrace_graph_regs_call,
- (unsigned long)&prepare_ftrace_return, true);
+ (unsigned long)&prepare_ftrace_return, true, true);
}
int ftrace_disable_ftrace_graph_caller(void)
@@ -216,12 +189,12 @@ int ftrace_disable_ftrace_graph_caller(void)
int ret;
ret = __ftrace_modify_call((unsigned long)&ftrace_graph_call,
- (unsigned long)&prepare_ftrace_return, false);
+ (unsigned long)&prepare_ftrace_return, false, true);
if (ret)
return ret;
return __ftrace_modify_call((unsigned long)&ftrace_graph_regs_call,
- (unsigned long)&prepare_ftrace_return, false);
+ (unsigned long)&prepare_ftrace_return, false, true);
}
#endif /* CONFIG_DYNAMIC_FTRACE */
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
index d171eca623b6..125de818d1ba 100644
--- a/arch/riscv/kernel/mcount-dyn.S
+++ b/arch/riscv/kernel/mcount-dyn.S
@@ -13,8 +13,8 @@
.text
-#define FENTRY_RA_OFFSET 12
-#define ABI_SIZE_ON_STACK 72
+#define FENTRY_RA_OFFSET 8
+#define ABI_SIZE_ON_STACK 80
#define ABI_A0 0
#define ABI_A1 8
#define ABI_A2 16
@@ -23,10 +23,10 @@
#define ABI_A5 40
#define ABI_A6 48
#define ABI_A7 56
-#define ABI_RA 64
+#define ABI_T0 64
+#define ABI_RA 72
.macro SAVE_ABI
- addi sp, sp, -SZREG
addi sp, sp, -ABI_SIZE_ON_STACK
REG_S a0, ABI_A0(sp)
@@ -37,6 +37,7 @@
REG_S a5, ABI_A5(sp)
REG_S a6, ABI_A6(sp)
REG_S a7, ABI_A7(sp)
+ REG_S t0, ABI_T0(sp)
REG_S ra, ABI_RA(sp)
.endm
@@ -49,24 +50,18 @@
REG_L a5, ABI_A5(sp)
REG_L a6, ABI_A6(sp)
REG_L a7, ABI_A7(sp)
+ REG_L t0, ABI_T0(sp)
REG_L ra, ABI_RA(sp)
addi sp, sp, ABI_SIZE_ON_STACK
- addi sp, sp, SZREG
.endm
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
.macro SAVE_ALL
- addi sp, sp, -SZREG
addi sp, sp, -PT_SIZE_ON_STACK
- REG_S x1, PT_EPC(sp)
- addi sp, sp, PT_SIZE_ON_STACK
- REG_L x1, (sp)
- addi sp, sp, -PT_SIZE_ON_STACK
+ REG_S t0, PT_EPC(sp)
REG_S x1, PT_RA(sp)
- REG_L x1, PT_EPC(sp)
-
REG_S x2, PT_SP(sp)
REG_S x3, PT_GP(sp)
REG_S x4, PT_TP(sp)
@@ -100,15 +95,11 @@
.endm
.macro RESTORE_ALL
+ REG_L t0, PT_EPC(sp)
REG_L x1, PT_RA(sp)
- addi sp, sp, PT_SIZE_ON_STACK
- REG_S x1, (sp)
- addi sp, sp, -PT_SIZE_ON_STACK
- REG_L x1, PT_EPC(sp)
REG_L x2, PT_SP(sp)
REG_L x3, PT_GP(sp)
REG_L x4, PT_TP(sp)
- REG_L x5, PT_T0(sp)
REG_L x6, PT_T1(sp)
REG_L x7, PT_T2(sp)
REG_L x8, PT_S0(sp)
@@ -137,17 +128,16 @@
REG_L x31, PT_T6(sp)
addi sp, sp, PT_SIZE_ON_STACK
- addi sp, sp, SZREG
.endm
#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
ENTRY(ftrace_caller)
SAVE_ABI
- addi a0, ra, -FENTRY_RA_OFFSET
+ addi a0, t0, -FENTRY_RA_OFFSET
la a1, function_trace_op
REG_L a2, 0(a1)
- REG_L a1, ABI_SIZE_ON_STACK(sp)
+ mv a1, ra
mv a3, sp
ftrace_call:
@@ -155,8 +145,8 @@ ftrace_call:
call ftrace_stub
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- addi a0, sp, ABI_SIZE_ON_STACK
- REG_L a1, ABI_RA(sp)
+ addi a0, sp, ABI_RA
+ REG_L a1, ABI_T0(sp)
addi a1, a1, -FENTRY_RA_OFFSET
#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
mv a2, s0
@@ -166,17 +156,17 @@ ftrace_graph_call:
call ftrace_stub
#endif
RESTORE_ABI
- ret
+ jr t0
ENDPROC(ftrace_caller)
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
ENTRY(ftrace_regs_caller)
SAVE_ALL
- addi a0, ra, -FENTRY_RA_OFFSET
+ addi a0, t0, -FENTRY_RA_OFFSET
la a1, function_trace_op
REG_L a2, 0(a1)
- REG_L a1, PT_SIZE_ON_STACK(sp)
+ mv a1, ra
mv a3, sp
ftrace_regs_call:
@@ -196,6 +186,6 @@ ftrace_graph_regs_call:
#endif
RESTORE_ALL
- ret
+ jr t0
ENDPROC(ftrace_regs_caller)
#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
--
2.36.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 3/7] riscv: ftrace: Reduce the detour code size to half
2023-01-12 9:05 ` [PATCH -next V7 3/7] riscv: ftrace: Reduce the detour code size to half guoren
@ 2023-01-16 14:11 ` Evgenii Shatokhin
0 siblings, 0 replies; 44+ messages in thread
From: Evgenii Shatokhin @ 2023-01-16 14:11 UTC (permalink / raw)
To: guoren
Cc: linux-riscv, linux-kernel, Guo Ren, anup, paul.walmsley, palmer,
conor.dooley, heiko, rostedt, mhiramat, jolsa, bp, jpoimboe,
suagrfillet, andy.chiu, linux
Hi,
On 12.01.2023 12:05, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
>
> Use a temporary register to reduce the size of detour code from 16 bytes to
> 8 bytes. The previous implementation is from 'commit afc76b8b8011 ("riscv:
> Using PATCHABLE_FUNCTION_ENTRY instead of MCOUNT")'.
>
> Before the patch:
> <func_prolog>:
> 0: REG_S ra, -SZREG(sp)
> 4: auipc ra, ?
> 8: jalr ?(ra)
> 12: REG_L ra, -SZREG(sp)
> (func_boddy)
>
> After the patch:
> <func_prolog>:
> 0: auipc t0, ?
> 4: jalr t0, ?(t0)
> (func_boddy)
>
> This patch not just reduces the size of detour code, but also fixes an
> important issue:
>
> An Ftrace callback registered with FTRACE_OPS_FL_IPMODIFY flag can
> actually change the instruction pointer, e.g. to "replace" the given
> kernel function with a new one, which is needed for livepatching, etc.
>
> In this case, the trampoline (ftrace_regs_caller) would not return to
> <func_prolog+12> but would rather jump to the new function. So, "REG_L
> ra, -SZREG(sp)" would not run and the original return address would not
> be restored. The kernel is likely to hang or crash as a result.
>
> This can be easily demonstrated if one tries to "replace", say,
> cmdline_proc_show() with a new function with the same signature using
> instruction_pointer_set(&fregs->regs, new_func_addr) in the Ftrace
> callback.
>
> Link: https://lore.kernel.org/linux-riscv/20221122075440.1165172-1-suagrfillet@gmail.com/
> Link: https://lore.kernel.org/linux-riscv/d7d5730b-ebef-68e5-5046-e763e1ee6164@yadro.com/
> Co-developed-by: Song Shuai <suagrfillet@gmail.com>
> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> Cc: Evgenii Shatokhin <e.shatokhin@yadro.com>
> ---
> arch/riscv/Makefile | 4 +-
> arch/riscv/include/asm/ftrace.h | 50 +++++++++++++++++++------
> arch/riscv/kernel/ftrace.c | 65 ++++++++++-----------------------
> arch/riscv/kernel/mcount-dyn.S | 42 ++++++++-------------
> 4 files changed, 75 insertions(+), 86 deletions(-)
>
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index ea5a91da6897..3c9aaf67ed79 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -12,9 +12,9 @@ ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
> LDFLAGS_vmlinux := --no-relax
> KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
> ifeq ($(CONFIG_RISCV_ISA_C),y)
> - CC_FLAGS_FTRACE := -fpatchable-function-entry=8
> -else
> CC_FLAGS_FTRACE := -fpatchable-function-entry=4
> +else
> + CC_FLAGS_FTRACE := -fpatchable-function-entry=2
> endif
> endif
>
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index 04dad3380041..9e73922e1e2e 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -42,6 +42,14 @@ struct dyn_arch_ftrace {
> * 2) jalr: setting low-12 offset to ra, jump to ra, and set ra to
> * return address (original pc + 4)
> *
> + *<ftrace enable>:
> + * 0: auipc t0/ra, 0x?
> + * 4: jalr t0/ra, ?(t0/ra)
> + *
> + *<ftrace disable>:
> + * 0: nop
> + * 4: nop
> + *
> * Dynamic ftrace generates probes to call sites, so we must deal with
> * both auipc and jalr at the same time.
> */
> @@ -52,25 +60,43 @@ struct dyn_arch_ftrace {
> #define AUIPC_OFFSET_MASK (0xfffff000)
> #define AUIPC_PAD (0x00001000)
> #define JALR_SHIFT 20
> -#define JALR_BASIC (0x000080e7)
> -#define AUIPC_BASIC (0x00000097)
> +#define JALR_RA (0x000080e7)
> +#define AUIPC_RA (0x00000097)
> +#define JALR_T0 (0x000282e7)
> +#define AUIPC_T0 (0x00000297)
> #define NOP4 (0x00000013)
>
> -#define make_call(caller, callee, call) \
> +#define to_jalr_t0(offset) \
> + (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0)
> +
> +#define to_auipc_t0(offset) \
> + ((offset & JALR_SIGN_MASK) ? \
> + (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_T0) : \
> + ((offset & AUIPC_OFFSET_MASK) | AUIPC_T0))
> +
> +#define make_call_t0(caller, callee, call) \
> do { \
> - call[0] = to_auipc_insn((unsigned int)((unsigned long)callee - \
> - (unsigned long)caller)); \
> - call[1] = to_jalr_insn((unsigned int)((unsigned long)callee - \
> - (unsigned long)caller)); \
> + unsigned int offset = \
> + (unsigned long) callee - (unsigned long) caller; \
> + call[0] = to_auipc_t0(offset); \
> + call[1] = to_jalr_t0(offset); \
> } while (0)
>
> -#define to_jalr_insn(offset) \
> - (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_BASIC)
> +#define to_jalr_ra(offset) \
> + (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_RA)
>
> -#define to_auipc_insn(offset) \
> +#define to_auipc_ra(offset) \
> ((offset & JALR_SIGN_MASK) ? \
> - (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_BASIC) : \
> - ((offset & AUIPC_OFFSET_MASK) | AUIPC_BASIC))
> + (((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) | AUIPC_RA) : \
> + ((offset & AUIPC_OFFSET_MASK) | AUIPC_RA))
> +
> +#define make_call_ra(caller, callee, call) \
> +do { \
> + unsigned int offset = \
> + (unsigned long) callee - (unsigned long) caller; \
> + call[0] = to_auipc_ra(offset); \
> + call[1] = to_jalr_ra(offset); \
> +} while (0)
>
> /*
> * Let auipc+jalr be the basic *mcount unit*, so we make it 8 bytes here.
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index 2086f6585773..5bff37af4770 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -55,12 +55,15 @@ static int ftrace_check_current_call(unsigned long hook_pos,
> }
>
> static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
> - bool enable)
> + bool enable, bool ra)
> {
> unsigned int call[2];
> unsigned int nops[2] = {NOP4, NOP4};
>
> - make_call(hook_pos, target, call);
> + if (ra)
> + make_call_ra(hook_pos, target, call);
> + else
> + make_call_t0(hook_pos, target, call);
>
> /* Replace the auipc-jalr pair at once. Return -EPERM on write error. */
> if (patch_text_nosync
> @@ -70,42 +73,13 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
> return 0;
> }
>
> -/*
> - * Put 5 instructions with 16 bytes at the front of function within
> - * patchable function entry nops' area.
> - *
> - * 0: REG_S ra, -SZREG(sp)
> - * 1: auipc ra, 0x?
> - * 2: jalr -?(ra)
> - * 3: REG_L ra, -SZREG(sp)
> - *
> - * So the opcodes is:
> - * 0: 0xfe113c23 (sd)/0xfe112e23 (sw)
> - * 1: 0x???????? -> auipc
> - * 2: 0x???????? -> jalr
> - * 3: 0xff813083 (ld)/0xffc12083 (lw)
> - */
> -#if __riscv_xlen == 64
> -#define INSN0 0xfe113c23
> -#define INSN3 0xff813083
> -#elif __riscv_xlen == 32
> -#define INSN0 0xfe112e23
> -#define INSN3 0xffc12083
> -#endif
> -
> -#define FUNC_ENTRY_SIZE 16
> -#define FUNC_ENTRY_JMP 4
> -
> int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> {
> - unsigned int call[4] = {INSN0, 0, 0, INSN3};
> - unsigned long target = addr;
> - unsigned long caller = rec->ip + FUNC_ENTRY_JMP;
> + unsigned int call[2];
>
> - call[1] = to_auipc_insn((unsigned int)(target - caller));
> - call[2] = to_jalr_insn((unsigned int)(target - caller));
> + make_call_t0(rec->ip, addr, call);
>
> - if (patch_text_nosync((void *)rec->ip, call, FUNC_ENTRY_SIZE))
> + if (patch_text_nosync((void *)rec->ip, call, MCOUNT_INSN_SIZE))
> return -EPERM;
>
> return 0;
> @@ -114,15 +88,14 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
> unsigned long addr)
> {
> - unsigned int nops[4] = {NOP4, NOP4, NOP4, NOP4};
> + unsigned int nops[2] = {NOP4, NOP4};
>
> - if (patch_text_nosync((void *)rec->ip, nops, FUNC_ENTRY_SIZE))
> + if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
> return -EPERM;
>
> return 0;
> }
>
> -
> /*
> * This is called early on, and isn't wrapped by
> * ftrace_arch_code_modify_{prepare,post_process}() and therefor doesn't hold
> @@ -144,10 +117,10 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> int ftrace_update_ftrace_func(ftrace_func_t func)
> {
> int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
> - (unsigned long)func, true);
> + (unsigned long)func, true, true);
> if (!ret) {
> ret = __ftrace_modify_call((unsigned long)&ftrace_regs_call,
> - (unsigned long)func, true);
> + (unsigned long)func, true, true);
> }
>
> return ret;
> @@ -159,16 +132,16 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
> unsigned long addr)
> {
> unsigned int call[2];
> - unsigned long caller = rec->ip + FUNC_ENTRY_JMP;
> + unsigned long caller = rec->ip;
> int ret;
>
> - make_call(caller, old_addr, call);
> + make_call_t0(caller, old_addr, call);
> ret = ftrace_check_current_call(caller, call);
>
> if (ret)
> return ret;
>
> - return __ftrace_modify_call(caller, addr, true);
> + return __ftrace_modify_call(caller, addr, true, false);
> }
> #endif
>
> @@ -203,12 +176,12 @@ int ftrace_enable_ftrace_graph_caller(void)
> int ret;
>
> ret = __ftrace_modify_call((unsigned long)&ftrace_graph_call,
> - (unsigned long)&prepare_ftrace_return, true);
> + (unsigned long)&prepare_ftrace_return, true, true);
> if (ret)
> return ret;
>
> return __ftrace_modify_call((unsigned long)&ftrace_graph_regs_call,
> - (unsigned long)&prepare_ftrace_return, true);
> + (unsigned long)&prepare_ftrace_return, true, true);
> }
>
> int ftrace_disable_ftrace_graph_caller(void)
> @@ -216,12 +189,12 @@ int ftrace_disable_ftrace_graph_caller(void)
> int ret;
>
> ret = __ftrace_modify_call((unsigned long)&ftrace_graph_call,
> - (unsigned long)&prepare_ftrace_return, false);
> + (unsigned long)&prepare_ftrace_return, false, true);
> if (ret)
> return ret;
>
> return __ftrace_modify_call((unsigned long)&ftrace_graph_regs_call,
> - (unsigned long)&prepare_ftrace_return, false);
> + (unsigned long)&prepare_ftrace_return, false, true);
> }
> #endif /* CONFIG_DYNAMIC_FTRACE */
> #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
> index d171eca623b6..125de818d1ba 100644
> --- a/arch/riscv/kernel/mcount-dyn.S
> +++ b/arch/riscv/kernel/mcount-dyn.S
> @@ -13,8 +13,8 @@
>
> .text
>
> -#define FENTRY_RA_OFFSET 12
> -#define ABI_SIZE_ON_STACK 72
> +#define FENTRY_RA_OFFSET 8
> +#define ABI_SIZE_ON_STACK 80
> #define ABI_A0 0
> #define ABI_A1 8
> #define ABI_A2 16
> @@ -23,10 +23,10 @@
> #define ABI_A5 40
> #define ABI_A6 48
> #define ABI_A7 56
> -#define ABI_RA 64
> +#define ABI_T0 64
> +#define ABI_RA 72
>
> .macro SAVE_ABI
> - addi sp, sp, -SZREG
> addi sp, sp, -ABI_SIZE_ON_STACK
>
> REG_S a0, ABI_A0(sp)
> @@ -37,6 +37,7 @@
> REG_S a5, ABI_A5(sp)
> REG_S a6, ABI_A6(sp)
> REG_S a7, ABI_A7(sp)
> + REG_S t0, ABI_T0(sp)
> REG_S ra, ABI_RA(sp)
> .endm
>
> @@ -49,24 +50,18 @@
> REG_L a5, ABI_A5(sp)
> REG_L a6, ABI_A6(sp)
> REG_L a7, ABI_A7(sp)
> + REG_L t0, ABI_T0(sp)
> REG_L ra, ABI_RA(sp)
>
> addi sp, sp, ABI_SIZE_ON_STACK
> - addi sp, sp, SZREG
> .endm
>
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> .macro SAVE_ALL
> - addi sp, sp, -SZREG
> addi sp, sp, -PT_SIZE_ON_STACK
>
> - REG_S x1, PT_EPC(sp)
> - addi sp, sp, PT_SIZE_ON_STACK
> - REG_L x1, (sp)
> - addi sp, sp, -PT_SIZE_ON_STACK
> + REG_S t0, PT_EPC(sp)
> REG_S x1, PT_RA(sp)
> - REG_L x1, PT_EPC(sp)
> -
> REG_S x2, PT_SP(sp)
> REG_S x3, PT_GP(sp)
> REG_S x4, PT_TP(sp)
> @@ -100,15 +95,11 @@
> .endm
>
> .macro RESTORE_ALL
> + REG_L t0, PT_EPC(sp)
> REG_L x1, PT_RA(sp)
> - addi sp, sp, PT_SIZE_ON_STACK
> - REG_S x1, (sp)
> - addi sp, sp, -PT_SIZE_ON_STACK
> - REG_L x1, PT_EPC(sp)
> REG_L x2, PT_SP(sp)
> REG_L x3, PT_GP(sp)
> REG_L x4, PT_TP(sp)
> - REG_L x5, PT_T0(sp)
> REG_L x6, PT_T1(sp)
> REG_L x7, PT_T2(sp)
> REG_L x8, PT_S0(sp)
> @@ -137,17 +128,16 @@
> REG_L x31, PT_T6(sp)
>
> addi sp, sp, PT_SIZE_ON_STACK
> - addi sp, sp, SZREG
> .endm
> #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>
> ENTRY(ftrace_caller)
> SAVE_ABI
>
> - addi a0, ra, -FENTRY_RA_OFFSET
> + addi a0, t0, -FENTRY_RA_OFFSET
> la a1, function_trace_op
> REG_L a2, 0(a1)
> - REG_L a1, ABI_SIZE_ON_STACK(sp)
> + mv a1, ra
> mv a3, sp
>
> ftrace_call:
> @@ -155,8 +145,8 @@ ftrace_call:
> call ftrace_stub
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> - addi a0, sp, ABI_SIZE_ON_STACK
> - REG_L a1, ABI_RA(sp)
> + addi a0, sp, ABI_RA
> + REG_L a1, ABI_T0(sp)
> addi a1, a1, -FENTRY_RA_OFFSET
> #ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> mv a2, s0
> @@ -166,17 +156,17 @@ ftrace_graph_call:
> call ftrace_stub
> #endif
> RESTORE_ABI
> - ret
> + jr t0
> ENDPROC(ftrace_caller)
>
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> ENTRY(ftrace_regs_caller)
> SAVE_ALL
>
> - addi a0, ra, -FENTRY_RA_OFFSET
> + addi a0, t0, -FENTRY_RA_OFFSET
> la a1, function_trace_op
> REG_L a2, 0(a1)
> - REG_L a1, PT_SIZE_ON_STACK(sp)
> + mv a1, ra
> mv a3, sp
>
> ftrace_regs_call:
> @@ -196,6 +186,6 @@ ftrace_graph_regs_call:
> #endif
>
> RESTORE_ALL
> - ret
> + jr t0
> ENDPROC(ftrace_regs_caller)
> #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> --
> 2.36.1
>
>
Looks good to me.
I also re-checked if "replacement" of cmdline_proc_show() with a custom
function via Ftrace works in this case - it does. Rollback to the
original cmdline_proc_show() seems to work OK too.
Reviewed-by: Evgenii Shatokhin <e.shatokhin@yadro.com>
Regards,
Evgenii
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH -next V7 4/7] riscv: ftrace: Add ftrace_graph_func
2023-01-12 9:05 [PATCH -next V7 0/7] riscv: Optimize function trace guoren
` (2 preceding siblings ...)
2023-01-12 9:05 ` [PATCH -next V7 3/7] riscv: ftrace: Reduce the detour code size to half guoren
@ 2023-01-12 9:06 ` guoren
2023-01-12 9:06 ` [PATCH -next V7 5/7] riscv: ftrace: Add DYNAMIC_FTRACE_WITH_DIRECT_CALLS support guoren
` (4 subsequent siblings)
8 siblings, 0 replies; 44+ messages in thread
From: guoren @ 2023-01-12 9:06 UTC (permalink / raw)
To: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, guoren
Cc: linux-riscv, linux-kernel
From: Song Shuai <suagrfillet@gmail.com>
Here implements ftrace_graph_func as the function graph tracing function
with FTRACE_WITH_REGS defined.
function_graph_func gets the point of the parent IP and the frame pointer
from fregs and call prepare_ftrace_return for function graph tracing.
If FTRACE_WITH_REGS isn't defined, the enable/disable helpers of
ftrace_graph_[regs]_call are revised for serving only ftrace_graph_call
in the !FTRACE_WITH_REGS version ftrace_caller.
Signed-off-by: Song Shuai <suagrfillet@gmail.com>
Tested-by: Guo Ren <guoren@kernel.org>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
arch/riscv/include/asm/ftrace.h | 13 ++-
arch/riscv/kernel/ftrace.c | 30 +++----
arch/riscv/kernel/mcount-dyn.S | 139 +++++++++++++++++++++++---------
3 files changed, 126 insertions(+), 56 deletions(-)
diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
index 9e73922e1e2e..84f856a3286e 100644
--- a/arch/riscv/include/asm/ftrace.h
+++ b/arch/riscv/include/asm/ftrace.h
@@ -107,8 +107,17 @@ do { \
struct dyn_ftrace;
int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
#define ftrace_init_nop ftrace_init_nop
-#endif
-#endif
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+struct ftrace_ops;
+struct ftrace_regs;
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *op, struct ftrace_regs *fregs);
+#define ftrace_graph_func ftrace_graph_func
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* CONFIG_DYNAMIC_FTRACE */
#endif /* _ASM_RISCV_FTRACE_H */
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index 5bff37af4770..95e14d8161a4 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -169,32 +169,28 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
}
#ifdef CONFIG_DYNAMIC_FTRACE
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+ struct pt_regs *regs = arch_ftrace_get_regs(fregs);
+ unsigned long *parent = (unsigned long *)®s->ra;
+
+ prepare_ftrace_return(parent, ip, frame_pointer(regs));
+}
+#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
extern void ftrace_graph_call(void);
-extern void ftrace_graph_regs_call(void);
int ftrace_enable_ftrace_graph_caller(void)
{
- int ret;
-
- ret = __ftrace_modify_call((unsigned long)&ftrace_graph_call,
- (unsigned long)&prepare_ftrace_return, true, true);
- if (ret)
- return ret;
-
- return __ftrace_modify_call((unsigned long)&ftrace_graph_regs_call,
+ return __ftrace_modify_call((unsigned long)&ftrace_graph_call,
(unsigned long)&prepare_ftrace_return, true, true);
}
int ftrace_disable_ftrace_graph_caller(void)
{
- int ret;
-
- ret = __ftrace_modify_call((unsigned long)&ftrace_graph_call,
- (unsigned long)&prepare_ftrace_return, false, true);
- if (ret)
- return ret;
-
- return __ftrace_modify_call((unsigned long)&ftrace_graph_regs_call,
+ return __ftrace_modify_call((unsigned long)&ftrace_graph_call,
(unsigned long)&prepare_ftrace_return, false, true);
}
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
#endif /* CONFIG_DYNAMIC_FTRACE */
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
index 125de818d1ba..f26e9f6e2fed 100644
--- a/arch/riscv/kernel/mcount-dyn.S
+++ b/arch/riscv/kernel/mcount-dyn.S
@@ -57,19 +57,52 @@
.endm
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
- .macro SAVE_ALL
+
+/**
+* SAVE_ABI_REGS - save regs against the pt_regs struct
+*
+* @all: tell if saving all the regs
+*
+* If all is set, all the regs will be saved, otherwise only ABI
+* related regs (a0-a7,epc,ra and optional s0) will be saved.
+*
+* After the stack is established,
+*
+* 0(sp) stores the PC of the traced function which can be accessed
+* by &(fregs)->regs->epc in tracing function. Note that the real
+* function entry address should be computed with -FENTRY_RA_OFFSET.
+*
+* 8(sp) stores the function return address (i.e. parent IP) that
+* can be accessed by &(fregs)->regs->ra in tracing function.
+*
+* The other regs are saved at the respective localtion and accessed
+* by the respective pt_regs member.
+*
+* Here is the layout of stack for your reference.
+*
+* PT_SIZE_ON_STACK -> +++++++++
+* + ..... +
+* + t3-t6 +
+* + s2-s11+
+* + a0-a7 + --++++-> ftrace_caller saved
+* + s1 + +
+* + s0 + --+
+* + t0-t2 + +
+* + tp + +
+* + gp + +
+* + sp + +
+* + ra + --+ // parent IP
+* sp -> + epc + --+ // PC
+* +++++++++
+**/
+ .macro SAVE_ABI_REGS, all=0
addi sp, sp, -PT_SIZE_ON_STACK
REG_S t0, PT_EPC(sp)
REG_S x1, PT_RA(sp)
- REG_S x2, PT_SP(sp)
- REG_S x3, PT_GP(sp)
- REG_S x4, PT_TP(sp)
- REG_S x5, PT_T0(sp)
- REG_S x6, PT_T1(sp)
- REG_S x7, PT_T2(sp)
- REG_S x8, PT_S0(sp)
- REG_S x9, PT_S1(sp)
+
+ // always save the ABI regs
+
REG_S x10, PT_A0(sp)
REG_S x11, PT_A1(sp)
REG_S x12, PT_A2(sp)
@@ -78,6 +111,18 @@
REG_S x15, PT_A5(sp)
REG_S x16, PT_A6(sp)
REG_S x17, PT_A7(sp)
+
+ // save the leftover regs
+
+ .if \all == 1
+ REG_S x2, PT_SP(sp)
+ REG_S x3, PT_GP(sp)
+ REG_S x4, PT_TP(sp)
+ REG_S x5, PT_T0(sp)
+ REG_S x6, PT_T1(sp)
+ REG_S x7, PT_T2(sp)
+ REG_S x8, PT_S0(sp)
+ REG_S x9, PT_S1(sp)
REG_S x18, PT_S2(sp)
REG_S x19, PT_S3(sp)
REG_S x20, PT_S4(sp)
@@ -92,18 +137,19 @@
REG_S x29, PT_T4(sp)
REG_S x30, PT_T5(sp)
REG_S x31, PT_T6(sp)
+
+ // save s0 if FP_TEST defined
+
+ .else
+#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
+ REG_S x8, PT_S0(sp)
+#endif
+ .endif
.endm
- .macro RESTORE_ALL
+ .macro RESTORE_ABI_REGS, all=0
REG_L t0, PT_EPC(sp)
REG_L x1, PT_RA(sp)
- REG_L x2, PT_SP(sp)
- REG_L x3, PT_GP(sp)
- REG_L x4, PT_TP(sp)
- REG_L x6, PT_T1(sp)
- REG_L x7, PT_T2(sp)
- REG_L x8, PT_S0(sp)
- REG_L x9, PT_S1(sp)
REG_L x10, PT_A0(sp)
REG_L x11, PT_A1(sp)
REG_L x12, PT_A2(sp)
@@ -112,6 +158,15 @@
REG_L x15, PT_A5(sp)
REG_L x16, PT_A6(sp)
REG_L x17, PT_A7(sp)
+
+ .if \all == 1
+ REG_L x2, PT_SP(sp)
+ REG_L x3, PT_GP(sp)
+ REG_L x4, PT_TP(sp)
+ REG_L x6, PT_T1(sp)
+ REG_L x7, PT_T2(sp)
+ REG_L x8, PT_S0(sp)
+ REG_L x9, PT_S1(sp)
REG_L x18, PT_S2(sp)
REG_L x19, PT_S3(sp)
REG_L x20, PT_S4(sp)
@@ -127,10 +182,25 @@
REG_L x30, PT_T5(sp)
REG_L x31, PT_T6(sp)
+ .else
+#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
+ REG_L x8, PT_S0(sp)
+#endif
+ .endif
addi sp, sp, PT_SIZE_ON_STACK
.endm
+
+ .macro PREPARE_ARGS
+ addi a0, t0, -FENTRY_RA_OFFSET // ip
+ la a1, function_trace_op
+ REG_L a2, 0(a1) // op
+ mv a1, ra // parent_ip
+ mv a3, sp // fregs
+ .endm
+
#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS
ENTRY(ftrace_caller)
SAVE_ABI
@@ -159,33 +229,28 @@ ftrace_graph_call:
jr t0
ENDPROC(ftrace_caller)
-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
ENTRY(ftrace_regs_caller)
- SAVE_ALL
-
- addi a0, t0, -FENTRY_RA_OFFSET
- la a1, function_trace_op
- REG_L a2, 0(a1)
- mv a1, ra
- mv a3, sp
+ SAVE_ABI_REGS 1
+ PREPARE_ARGS
ftrace_regs_call:
.global ftrace_regs_call
call ftrace_stub
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- addi a0, sp, PT_RA
- REG_L a1, PT_EPC(sp)
- addi a1, a1, -FENTRY_RA_OFFSET
-#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
- mv a2, s0
-#endif
-ftrace_graph_regs_call:
- .global ftrace_graph_regs_call
+ RESTORE_ABI_REGS 1
+ jr t0
+ENDPROC(ftrace_regs_caller)
+
+ENTRY(ftrace_caller)
+ SAVE_ABI_REGS 0
+ PREPARE_ARGS
+
+ftrace_call:
+ .global ftrace_call
call ftrace_stub
-#endif
- RESTORE_ALL
+ RESTORE_ABI_REGS 0
jr t0
-ENDPROC(ftrace_regs_caller)
+ENDPROC(ftrace_caller)
#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
--
2.36.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH -next V7 5/7] riscv: ftrace: Add DYNAMIC_FTRACE_WITH_DIRECT_CALLS support
2023-01-12 9:05 [PATCH -next V7 0/7] riscv: Optimize function trace guoren
` (3 preceding siblings ...)
2023-01-12 9:06 ` [PATCH -next V7 4/7] riscv: ftrace: Add ftrace_graph_func guoren
@ 2023-01-12 9:06 ` guoren
2023-01-12 9:06 ` [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI] guoren
` (3 subsequent siblings)
8 siblings, 0 replies; 44+ messages in thread
From: guoren @ 2023-01-12 9:06 UTC (permalink / raw)
To: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, guoren
Cc: linux-riscv, linux-kernel
From: Song Shuai <suagrfillet@gmail.com>
This patch adds DYNAMIC_FTRACE_WITH_DIRECT_CALLS support for RISC-V.
select the DYNAMIC_FTRACE_WITH_DIRECT_CALLS to provide the
register_ftrace_direct[_multi] interfaces allowing users to register
the customed trampoline (direct_caller) as the mcount for one or
more target functions. And modify_ftrace_direct[_multi] are also
provided for modifying direct_caller.
To make the direct_caller and the other ftrace hooks (eg. function/fgraph
tracer, k[ret]probes) co-exist, a temporary register is nominated to
store the address of direct_caller in ftrace_regs_caller. After the
setting of the address direct_caller by direct_ops->func and the
RESTORE_REGS in ftrace_regs_caller, direct_caller will be jumped to
by the `jr` inst.
Signed-off-by: Song Shuai <suagrfillet@gmail.com>
Tested-by: Guo Ren <guoren@kernel.org>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
arch/riscv/Kconfig | 1 +
arch/riscv/include/asm/ftrace.h | 8 ++++++++
arch/riscv/kernel/mcount-dyn.S | 4 ++++
3 files changed, 13 insertions(+)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index ee0d39b26794..307a9f413edd 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -135,6 +135,7 @@ config RISCV
select UACCESS_MEMCPY if !MMU
select ZONE_DMA32 if 64BIT
select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && $(cc-option,-fpatchable-function-entry=8)
+ select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
select HAVE_FUNCTION_GRAPH_TRACER
diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
index 84f856a3286e..84904c1e4369 100644
--- a/arch/riscv/include/asm/ftrace.h
+++ b/arch/riscv/include/asm/ftrace.h
@@ -114,6 +114,14 @@ struct ftrace_regs;
void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
struct ftrace_ops *op, struct ftrace_regs *fregs);
#define ftrace_graph_func ftrace_graph_func
+
+static inline void
+__arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned long addr)
+{
+ regs->t1 = addr;
+}
+#define arch_ftrace_set_direct_caller(fregs, addr) \
+ __arch_ftrace_set_direct_caller(&(fregs)->regs, addr)
#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
#endif /* __ASSEMBLY__ */
diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
index f26e9f6e2fed..7801c1c8bb5a 100644
--- a/arch/riscv/kernel/mcount-dyn.S
+++ b/arch/riscv/kernel/mcount-dyn.S
@@ -231,6 +231,7 @@ ENDPROC(ftrace_caller)
#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
ENTRY(ftrace_regs_caller)
+ move t1, zero
SAVE_ABI_REGS 1
PREPARE_ARGS
@@ -239,7 +240,10 @@ ftrace_regs_call:
call ftrace_stub
RESTORE_ABI_REGS 1
+ bnez t1,.Ldirect
jr t0
+.Ldirect:
+ jr t1
ENDPROC(ftrace_regs_caller)
ENTRY(ftrace_caller)
--
2.36.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-01-12 9:05 [PATCH -next V7 0/7] riscv: Optimize function trace guoren
` (4 preceding siblings ...)
2023-01-12 9:06 ` [PATCH -next V7 5/7] riscv: ftrace: Add DYNAMIC_FTRACE_WITH_DIRECT_CALLS support guoren
@ 2023-01-12 9:06 ` guoren
2023-01-16 14:30 ` Evgenii Shatokhin
2023-01-12 9:06 ` [PATCH -next V7 7/7] riscv : select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY guoren
` (2 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: guoren @ 2023-01-12 9:06 UTC (permalink / raw)
To: anup, paul.walmsley, palmer, conor.dooley, heiko, rostedt,
mhiramat, jolsa, bp, jpoimboe, suagrfillet, andy.chiu,
e.shatokhin, guoren
Cc: linux-riscv, linux-kernel
From: Song Shuai <suagrfillet@gmail.com>
select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
the ftrace-direct*.c files in samples/ftrace/.
Signed-off-by: Song Shuai <suagrfillet@gmail.com>
Tested-by: Guo Ren <guoren@kernel.org>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
arch/riscv/Kconfig | 2 ++
samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
samples/ftrace/ftrace-direct-multi-modify.c | 37 +++++++++++++++++++++
samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
samples/ftrace/ftrace-direct.c | 22 ++++++++++++
6 files changed, 142 insertions(+)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 307a9f413edd..e944af44f681 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -112,6 +112,8 @@ config RISCV
select HAVE_POSIX_CPU_TIMERS_TASK_WORK
select HAVE_REGS_AND_STACK_ACCESS_API
select HAVE_FUNCTION_ARG_ACCESS_API
+ select HAVE_SAMPLE_FTRACE_DIRECT
+ select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
select HAVE_STACKPROTECTOR
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_RSEQ
diff --git a/samples/ftrace/ftrace-direct-modify.c b/samples/ftrace/ftrace-direct-modify.c
index de5a0f67f320..be7bf472c3c7 100644
--- a/samples/ftrace/ftrace-direct-modify.c
+++ b/samples/ftrace/ftrace-direct-modify.c
@@ -23,6 +23,39 @@ extern void my_tramp2(void *);
static unsigned long my_ip = (unsigned long)schedule;
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection .text, \"ax\", @progbits\n"
+" .type my_tramp1, @function\n"
+" .globl my_tramp1\n"
+" my_tramp1:\n"
+" addi sp,sp,-16\n"
+" sd t0,0(sp)\n"
+" sd ra,8(sp)\n"
+" call my_direct_func1\n"
+" ld t0,0(sp)\n"
+" ld ra,8(sp)\n"
+" addi sp,sp,16\n"
+" jr t0\n"
+" .size my_tramp1, .-my_tramp1\n"
+
+" .type my_tramp2, @function\n"
+" .globl my_tramp2\n"
+" my_tramp2:\n"
+" addi sp,sp,-16\n"
+" sd t0,0(sp)\n"
+" sd ra,8(sp)\n"
+" call my_direct_func2\n"
+" ld t0,0(sp)\n"
+" ld ra,8(sp)\n"
+" addi sp,sp,16\n"
+" jr t0\n"
+" .size my_tramp2, .-my_tramp2\n"
+" .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
#ifdef CONFIG_X86_64
#include <asm/ibt.h>
diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
index d52370cad0b6..10884bf418f7 100644
--- a/samples/ftrace/ftrace-direct-multi-modify.c
+++ b/samples/ftrace/ftrace-direct-multi-modify.c
@@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
extern void my_tramp1(void *);
extern void my_tramp2(void *);
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection .text, \"ax\", @progbits\n"
+" .type my_tramp1, @function\n"
+" .globl my_tramp1\n"
+" my_tramp1:\n"
+" addi sp,sp,-24\n"
+" sd a0,0(sp)\n"
+" sd t0,8(sp)\n"
+" sd ra,16(sp)\n"
+" call my_direct_func1\n"
+" ld a0,0(sp)\n"
+" ld t0,8(sp)\n"
+" ld ra,16(sp)\n"
+" addi sp,sp,24\n"
+" jr t0\n"
+" .size my_tramp1, .-my_tramp1\n"
+
+" .type my_tramp2, @function\n"
+" .globl my_tramp2\n"
+" my_tramp2:\n"
+" addi sp,sp,-24\n"
+" sd a0,0(sp)\n"
+" sd t0,8(sp)\n"
+" sd ra,16(sp)\n"
+" call my_direct_func2\n"
+" ld a0,0(sp)\n"
+" ld t0,8(sp)\n"
+" ld ra,16(sp)\n"
+" addi sp,sp,24\n"
+" jr t0\n"
+" .size my_tramp2, .-my_tramp2\n"
+" .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
#ifdef CONFIG_X86_64
#include <asm/ibt.h>
diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
index ec1088922517..a35bf43bf6d7 100644
--- a/samples/ftrace/ftrace-direct-multi.c
+++ b/samples/ftrace/ftrace-direct-multi.c
@@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
extern void my_tramp(void *);
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection .text, \"ax\", @progbits\n"
+" .type my_tramp, @function\n"
+" .globl my_tramp\n"
+" my_tramp:\n"
+" addi sp,sp,-24\n"
+" sd a0,0(sp)\n"
+" sd t0,8(sp)\n"
+" sd ra,16(sp)\n"
+" call my_direct_func\n"
+" ld a0,0(sp)\n"
+" ld t0,8(sp)\n"
+" ld ra,16(sp)\n"
+" addi sp,sp,24\n"
+" jr t0\n"
+" .size my_tramp, .-my_tramp\n"
+" .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
#ifdef CONFIG_X86_64
#include <asm/ibt.h>
diff --git a/samples/ftrace/ftrace-direct-too.c b/samples/ftrace/ftrace-direct-too.c
index e13fb59a2b47..3b62e33c2e6d 100644
--- a/samples/ftrace/ftrace-direct-too.c
+++ b/samples/ftrace/ftrace-direct-too.c
@@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
extern void my_tramp(void *);
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection .text, \"ax\", @progbits\n"
+" .type my_tramp, @function\n"
+" .globl my_tramp\n"
+" my_tramp:\n"
+" addi sp,sp,-40\n"
+" sd a0,0(sp)\n"
+" sd a1,8(sp)\n"
+" sd a2,16(sp)\n"
+" sd t0,24(sp)\n"
+" sd ra,32(sp)\n"
+" call my_direct_func\n"
+" ld a0,0(sp)\n"
+" ld a1,8(sp)\n"
+" ld a2,16(sp)\n"
+" ld t0,24(sp)\n"
+" ld ra,32(sp)\n"
+" addi sp,sp,40\n"
+" jr t0\n"
+" .size my_tramp, .-my_tramp\n"
+" .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
#ifdef CONFIG_X86_64
#include <asm/ibt.h>
diff --git a/samples/ftrace/ftrace-direct.c b/samples/ftrace/ftrace-direct.c
index 1f769d0db20f..2cfe5a7d2d70 100644
--- a/samples/ftrace/ftrace-direct.c
+++ b/samples/ftrace/ftrace-direct.c
@@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
extern void my_tramp(void *);
+#ifdef CONFIG_RISCV
+
+asm (" .pushsection .text, \"ax\", @progbits\n"
+" .type my_tramp, @function\n"
+" .globl my_tramp\n"
+" my_tramp:\n"
+" addi sp,sp,-24\n"
+" sd a0,0(sp)\n"
+" sd t0,8(sp)\n"
+" sd ra,16(sp)\n"
+" call my_direct_func\n"
+" ld a0,0(sp)\n"
+" ld t0,8(sp)\n"
+" ld ra,16(sp)\n"
+" addi sp,sp,24\n"
+" jr t0\n"
+" .size my_tramp, .-my_tramp\n"
+" .popsection\n"
+);
+
+#endif /* CONFIG_RISCV */
+
#ifdef CONFIG_X86_64
#include <asm/ibt.h>
--
2.36.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-01-12 9:06 ` [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI] guoren
@ 2023-01-16 14:30 ` Evgenii Shatokhin
2023-01-17 9:32 ` Song Shuai
0 siblings, 1 reply; 44+ messages in thread
From: Evgenii Shatokhin @ 2023-01-16 14:30 UTC (permalink / raw)
To: guoren, suagrfillet
Cc: linux-riscv, linux-kernel, anup, paul.walmsley, palmer,
conor.dooley, heiko, rostedt, mhiramat, jolsa, bp, jpoimboe,
andy.chiu, linux
Hi,
On 12.01.2023 12:06, guoren@kernel.org wrote:
> From: Song Shuai <suagrfillet@gmail.com>
>
> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
> the ftrace-direct*.c files in samples/ftrace/.
>
> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> Tested-by: Guo Ren <guoren@kernel.org>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
> arch/riscv/Kconfig | 2 ++
> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
> samples/ftrace/ftrace-direct-multi-modify.c | 37 +++++++++++++++++++++
> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
> 6 files changed, 142 insertions(+)
The samples were built OK now, but ftrace-direct-multi and
ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
the messages in the trace:
# TASK-PID CPU# ||||| TIMESTAMP FUNCTION
# | | | ||||| | |
migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
direct func1 ip 0
migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
direct func1 ip ff60000001ba9600
migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
direct func1 ip ff60000003334d00
migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
direct func1 ip 0
rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
direct func1 ip 0
insmod-415 [000] ..... 3858.532270: my_direct_func1: my
direct func1 ip 7fffffffffffffff
<idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
direct func1 ip ff60000001ba9600
<idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
direct func1 ip ff60000001ba9600
rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
direct func1 ip 0
[...]
If I understand it right, my_direct_func1() should print the address of
some location in the code, probably - at the beginning of the traced
functions.
The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
addresses.
The same issue is with ftrace-direct-multi-modify.ko.
Is anything missing here?
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 307a9f413edd..e944af44f681 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -112,6 +112,8 @@ config RISCV
> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
> select HAVE_REGS_AND_STACK_ACCESS_API
> select HAVE_FUNCTION_ARG_ACCESS_API
> + select HAVE_SAMPLE_FTRACE_DIRECT
> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> select HAVE_STACKPROTECTOR
> select HAVE_SYSCALL_TRACEPOINTS
> select HAVE_RSEQ
> diff --git a/samples/ftrace/ftrace-direct-modify.c b/samples/ftrace/ftrace-direct-modify.c
> index de5a0f67f320..be7bf472c3c7 100644
> --- a/samples/ftrace/ftrace-direct-modify.c
> +++ b/samples/ftrace/ftrace-direct-modify.c
> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
>
> static unsigned long my_ip = (unsigned long)schedule;
>
> +#ifdef CONFIG_RISCV
> +
> +asm (" .pushsection .text, \"ax\", @progbits\n"
> +" .type my_tramp1, @function\n"
> +" .globl my_tramp1\n"
> +" my_tramp1:\n"
> +" addi sp,sp,-16\n"
> +" sd t0,0(sp)\n"
> +" sd ra,8(sp)\n"
> +" call my_direct_func1\n"
> +" ld t0,0(sp)\n"
> +" ld ra,8(sp)\n"
> +" addi sp,sp,16\n"
> +" jr t0\n"
> +" .size my_tramp1, .-my_tramp1\n"
> +
> +" .type my_tramp2, @function\n"
> +" .globl my_tramp2\n"
> +" my_tramp2:\n"
> +" addi sp,sp,-16\n"
> +" sd t0,0(sp)\n"
> +" sd ra,8(sp)\n"
> +" call my_direct_func2\n"
> +" ld t0,0(sp)\n"
> +" ld ra,8(sp)\n"
> +" addi sp,sp,16\n"
> +" jr t0\n"
> +" .size my_tramp2, .-my_tramp2\n"
> +" .popsection\n"
> +);
> +
> +#endif /* CONFIG_RISCV */
> +
> #ifdef CONFIG_X86_64
>
> #include <asm/ibt.h>
> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
> index d52370cad0b6..10884bf418f7 100644
> --- a/samples/ftrace/ftrace-direct-multi-modify.c
> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
> extern void my_tramp1(void *);
> extern void my_tramp2(void *);
>
> +#ifdef CONFIG_RISCV
> +
> +asm (" .pushsection .text, \"ax\", @progbits\n"
> +" .type my_tramp1, @function\n"
> +" .globl my_tramp1\n"
> +" my_tramp1:\n"
> +" addi sp,sp,-24\n"
> +" sd a0,0(sp)\n"
> +" sd t0,8(sp)\n"
> +" sd ra,16(sp)\n"
> +" call my_direct_func1\n"
> +" ld a0,0(sp)\n"
> +" ld t0,8(sp)\n"
> +" ld ra,16(sp)\n"
> +" addi sp,sp,24\n"
> +" jr t0\n"
> +" .size my_tramp1, .-my_tramp1\n"
> +
> +" .type my_tramp2, @function\n"
> +" .globl my_tramp2\n"
> +" my_tramp2:\n"
> +" addi sp,sp,-24\n"
> +" sd a0,0(sp)\n"
> +" sd t0,8(sp)\n"
> +" sd ra,16(sp)\n"
> +" call my_direct_func2\n"
> +" ld a0,0(sp)\n"
> +" ld t0,8(sp)\n"
> +" ld ra,16(sp)\n"
> +" addi sp,sp,24\n"
> +" jr t0\n"
> +" .size my_tramp2, .-my_tramp2\n"
> +" .popsection\n"
> +);
> +
> +#endif /* CONFIG_RISCV */
> +
> #ifdef CONFIG_X86_64
>
> #include <asm/ibt.h>
> diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
> index ec1088922517..a35bf43bf6d7 100644
> --- a/samples/ftrace/ftrace-direct-multi.c
> +++ b/samples/ftrace/ftrace-direct-multi.c
> @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
>
> extern void my_tramp(void *);
>
> +#ifdef CONFIG_RISCV
> +
> +asm (" .pushsection .text, \"ax\", @progbits\n"
> +" .type my_tramp, @function\n"
> +" .globl my_tramp\n"
> +" my_tramp:\n"
> +" addi sp,sp,-24\n"
> +" sd a0,0(sp)\n"
> +" sd t0,8(sp)\n"
> +" sd ra,16(sp)\n"
> +" call my_direct_func\n"
> +" ld a0,0(sp)\n"
> +" ld t0,8(sp)\n"
> +" ld ra,16(sp)\n"
> +" addi sp,sp,24\n"
> +" jr t0\n"
> +" .size my_tramp, .-my_tramp\n"
> +" .popsection\n"
> +);
> +
> +#endif /* CONFIG_RISCV */
> +
> #ifdef CONFIG_X86_64
>
> #include <asm/ibt.h>
> diff --git a/samples/ftrace/ftrace-direct-too.c b/samples/ftrace/ftrace-direct-too.c
> index e13fb59a2b47..3b62e33c2e6d 100644
> --- a/samples/ftrace/ftrace-direct-too.c
> +++ b/samples/ftrace/ftrace-direct-too.c
> @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
>
> extern void my_tramp(void *);
>
> +#ifdef CONFIG_RISCV
> +
> +asm (" .pushsection .text, \"ax\", @progbits\n"
> +" .type my_tramp, @function\n"
> +" .globl my_tramp\n"
> +" my_tramp:\n"
> +" addi sp,sp,-40\n"
> +" sd a0,0(sp)\n"
> +" sd a1,8(sp)\n"
> +" sd a2,16(sp)\n"
> +" sd t0,24(sp)\n"
> +" sd ra,32(sp)\n"
> +" call my_direct_func\n"
> +" ld a0,0(sp)\n"
> +" ld a1,8(sp)\n"
> +" ld a2,16(sp)\n"
> +" ld t0,24(sp)\n"
> +" ld ra,32(sp)\n"
> +" addi sp,sp,40\n"
> +" jr t0\n"
> +" .size my_tramp, .-my_tramp\n"
> +" .popsection\n"
> +);
> +
> +#endif /* CONFIG_RISCV */
> +
> #ifdef CONFIG_X86_64
>
> #include <asm/ibt.h>
> diff --git a/samples/ftrace/ftrace-direct.c b/samples/ftrace/ftrace-direct.c
> index 1f769d0db20f..2cfe5a7d2d70 100644
> --- a/samples/ftrace/ftrace-direct.c
> +++ b/samples/ftrace/ftrace-direct.c
> @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
>
> extern void my_tramp(void *);
>
> +#ifdef CONFIG_RISCV
> +
> +asm (" .pushsection .text, \"ax\", @progbits\n"
> +" .type my_tramp, @function\n"
> +" .globl my_tramp\n"
> +" my_tramp:\n"
> +" addi sp,sp,-24\n"
> +" sd a0,0(sp)\n"
> +" sd t0,8(sp)\n"
> +" sd ra,16(sp)\n"
> +" call my_direct_func\n"
> +" ld a0,0(sp)\n"
> +" ld t0,8(sp)\n"
> +" ld ra,16(sp)\n"
> +" addi sp,sp,24\n"
> +" jr t0\n"
> +" .size my_tramp, .-my_tramp\n"
> +" .popsection\n"
> +);
> +
> +#endif /* CONFIG_RISCV */
> +
> #ifdef CONFIG_X86_64
>
> #include <asm/ibt.h>
> --
> 2.36.1
>
>
Regards,
Evgenii
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-01-16 14:30 ` Evgenii Shatokhin
@ 2023-01-17 9:32 ` Song Shuai
2023-01-17 13:16 ` Evgenii Shatokhin
0 siblings, 1 reply; 44+ messages in thread
From: Song Shuai @ 2023-01-17 9:32 UTC (permalink / raw)
To: Evgenii Shatokhin
Cc: guoren, linux-riscv, linux-kernel, anup, paul.walmsley, palmer,
conor.dooley, heiko, rostedt, mhiramat, jolsa, bp, jpoimboe,
andy.chiu, linux
Hi, Evgenii:
Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
>
> Hi,
>
> On 12.01.2023 12:06, guoren@kernel.org wrote:
> > From: Song Shuai <suagrfillet@gmail.com>
> >
> > select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> > for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
> > the ftrace-direct*.c files in samples/ftrace/.
> >
> > Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> > Tested-by: Guo Ren <guoren@kernel.org>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > ---
> > arch/riscv/Kconfig | 2 ++
> > samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
> > samples/ftrace/ftrace-direct-multi-modify.c | 37 +++++++++++++++++++++
> > samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
> > samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
> > samples/ftrace/ftrace-direct.c | 22 ++++++++++++
> > 6 files changed, 142 insertions(+)
>
> The samples were built OK now, but ftrace-direct-multi and
> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
>
> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
> the messages in the trace:
>
> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> # | | | ||||| | |
> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
> direct func1 ip 0
> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
> direct func1 ip ff60000001ba9600
> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
> direct func1 ip ff60000003334d00
> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
> direct func1 ip 0
> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
> direct func1 ip 0
> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
> direct func1 ip 7fffffffffffffff
> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
> direct func1 ip ff60000001ba9600
> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
> direct func1 ip ff60000001ba9600
> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
> direct func1 ip 0
> [...]
>
> If I understand it right, my_direct_func1() should print the address of
> some location in the code, probably - at the beginning of the traced
> functions.
>
> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
> addresses.
>
The invalid code address is only printed by accessing the schedule()
function's first argument whose address stores in a0 register.
While schedule() actually has no parameter declared, so my_direct_func
just prints the a0 in the context of the schedule()'s caller and
the address maybe varies depending on the caller.
I can't really understand why tracing the first argument of the
schedule() function, but it seems nonsense at this point.
As for this patch, it just impls a simple mcount (direct_caller) to
trace kernel functions, and basically saves the necessary ABI,
call the tracing function, and restores the ABI, just like other arches do.
so It shouldn't be blamed.
I started an independent patch to replace schedule with kick_process
to make these samples more reasonable. And It has no conflict with the
current patch, so we can go on.
Link: https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
> The same issue is with ftrace-direct-multi-modify.ko.
>
> Is anything missing here?
>
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 307a9f413edd..e944af44f681 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -112,6 +112,8 @@ config RISCV
> > select HAVE_POSIX_CPU_TIMERS_TASK_WORK
> > select HAVE_REGS_AND_STACK_ACCESS_API
> > select HAVE_FUNCTION_ARG_ACCESS_API
> > + select HAVE_SAMPLE_FTRACE_DIRECT
> > + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> > select HAVE_STACKPROTECTOR
> > select HAVE_SYSCALL_TRACEPOINTS
> > select HAVE_RSEQ
> > diff --git a/samples/ftrace/ftrace-direct-modify.c b/samples/ftrace/ftrace-direct-modify.c
> > index de5a0f67f320..be7bf472c3c7 100644
> > --- a/samples/ftrace/ftrace-direct-modify.c
> > +++ b/samples/ftrace/ftrace-direct-modify.c
> > @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
> >
> > static unsigned long my_ip = (unsigned long)schedule;
> >
> > +#ifdef CONFIG_RISCV
> > +
> > +asm (" .pushsection .text, \"ax\", @progbits\n"
> > +" .type my_tramp1, @function\n"
> > +" .globl my_tramp1\n"
> > +" my_tramp1:\n"
> > +" addi sp,sp,-16\n"
> > +" sd t0,0(sp)\n"
> > +" sd ra,8(sp)\n"
> > +" call my_direct_func1\n"
> > +" ld t0,0(sp)\n"
> > +" ld ra,8(sp)\n"
> > +" addi sp,sp,16\n"
> > +" jr t0\n"
> > +" .size my_tramp1, .-my_tramp1\n"
> > +
> > +" .type my_tramp2, @function\n"
> > +" .globl my_tramp2\n"
> > +" my_tramp2:\n"
> > +" addi sp,sp,-16\n"
> > +" sd t0,0(sp)\n"
> > +" sd ra,8(sp)\n"
> > +" call my_direct_func2\n"
> > +" ld t0,0(sp)\n"
> > +" ld ra,8(sp)\n"
> > +" addi sp,sp,16\n"
> > +" jr t0\n"
> > +" .size my_tramp2, .-my_tramp2\n"
> > +" .popsection\n"
> > +);
> > +
> > +#endif /* CONFIG_RISCV */
> > +
> > #ifdef CONFIG_X86_64
> >
> > #include <asm/ibt.h>
> > diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
> > index d52370cad0b6..10884bf418f7 100644
> > --- a/samples/ftrace/ftrace-direct-multi-modify.c
> > +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> > @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
> > extern void my_tramp1(void *);
> > extern void my_tramp2(void *);
> >
> > +#ifdef CONFIG_RISCV
> > +
> > +asm (" .pushsection .text, \"ax\", @progbits\n"
> > +" .type my_tramp1, @function\n"
> > +" .globl my_tramp1\n"
> > +" my_tramp1:\n"
> > +" addi sp,sp,-24\n"
> > +" sd a0,0(sp)\n"
> > +" sd t0,8(sp)\n"
> > +" sd ra,16(sp)\n"
> > +" call my_direct_func1\n"
> > +" ld a0,0(sp)\n"
> > +" ld t0,8(sp)\n"
> > +" ld ra,16(sp)\n"
> > +" addi sp,sp,24\n"
> > +" jr t0\n"
> > +" .size my_tramp1, .-my_tramp1\n"
> > +
> > +" .type my_tramp2, @function\n"
> > +" .globl my_tramp2\n"
> > +" my_tramp2:\n"
> > +" addi sp,sp,-24\n"
> > +" sd a0,0(sp)\n"
> > +" sd t0,8(sp)\n"
> > +" sd ra,16(sp)\n"
> > +" call my_direct_func2\n"
> > +" ld a0,0(sp)\n"
> > +" ld t0,8(sp)\n"
> > +" ld ra,16(sp)\n"
> > +" addi sp,sp,24\n"
> > +" jr t0\n"
> > +" .size my_tramp2, .-my_tramp2\n"
> > +" .popsection\n"
> > +);
> > +
> > +#endif /* CONFIG_RISCV */
> > +
> > #ifdef CONFIG_X86_64
> >
> > #include <asm/ibt.h>
> > diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
> > index ec1088922517..a35bf43bf6d7 100644
> > --- a/samples/ftrace/ftrace-direct-multi.c
> > +++ b/samples/ftrace/ftrace-direct-multi.c
> > @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
> >
> > extern void my_tramp(void *);
> >
> > +#ifdef CONFIG_RISCV
> > +
> > +asm (" .pushsection .text, \"ax\", @progbits\n"
> > +" .type my_tramp, @function\n"
> > +" .globl my_tramp\n"
> > +" my_tramp:\n"
> > +" addi sp,sp,-24\n"
> > +" sd a0,0(sp)\n"
> > +" sd t0,8(sp)\n"
> > +" sd ra,16(sp)\n"
> > +" call my_direct_func\n"
> > +" ld a0,0(sp)\n"
> > +" ld t0,8(sp)\n"
> > +" ld ra,16(sp)\n"
> > +" addi sp,sp,24\n"
> > +" jr t0\n"
> > +" .size my_tramp, .-my_tramp\n"
> > +" .popsection\n"
> > +);
> > +
> > +#endif /* CONFIG_RISCV */
> > +
> > #ifdef CONFIG_X86_64
> >
> > #include <asm/ibt.h>
> > diff --git a/samples/ftrace/ftrace-direct-too.c b/samples/ftrace/ftrace-direct-too.c
> > index e13fb59a2b47..3b62e33c2e6d 100644
> > --- a/samples/ftrace/ftrace-direct-too.c
> > +++ b/samples/ftrace/ftrace-direct-too.c
> > @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
> >
> > extern void my_tramp(void *);
> >
> > +#ifdef CONFIG_RISCV
> > +
> > +asm (" .pushsection .text, \"ax\", @progbits\n"
> > +" .type my_tramp, @function\n"
> > +" .globl my_tramp\n"
> > +" my_tramp:\n"
> > +" addi sp,sp,-40\n"
> > +" sd a0,0(sp)\n"
> > +" sd a1,8(sp)\n"
> > +" sd a2,16(sp)\n"
> > +" sd t0,24(sp)\n"
> > +" sd ra,32(sp)\n"
> > +" call my_direct_func\n"
> > +" ld a0,0(sp)\n"
> > +" ld a1,8(sp)\n"
> > +" ld a2,16(sp)\n"
> > +" ld t0,24(sp)\n"
> > +" ld ra,32(sp)\n"
> > +" addi sp,sp,40\n"
> > +" jr t0\n"
> > +" .size my_tramp, .-my_tramp\n"
> > +" .popsection\n"
> > +);
> > +
> > +#endif /* CONFIG_RISCV */
> > +
> > #ifdef CONFIG_X86_64
> >
> > #include <asm/ibt.h>
> > diff --git a/samples/ftrace/ftrace-direct.c b/samples/ftrace/ftrace-direct.c
> > index 1f769d0db20f..2cfe5a7d2d70 100644
> > --- a/samples/ftrace/ftrace-direct.c
> > +++ b/samples/ftrace/ftrace-direct.c
> > @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
> >
> > extern void my_tramp(void *);
> >
> > +#ifdef CONFIG_RISCV
> > +
> > +asm (" .pushsection .text, \"ax\", @progbits\n"
> > +" .type my_tramp, @function\n"
> > +" .globl my_tramp\n"
> > +" my_tramp:\n"
> > +" addi sp,sp,-24\n"
> > +" sd a0,0(sp)\n"
> > +" sd t0,8(sp)\n"
> > +" sd ra,16(sp)\n"
> > +" call my_direct_func\n"
> > +" ld a0,0(sp)\n"
> > +" ld t0,8(sp)\n"
> > +" ld ra,16(sp)\n"
> > +" addi sp,sp,24\n"
> > +" jr t0\n"
> > +" .size my_tramp, .-my_tramp\n"
> > +" .popsection\n"
> > +);
> > +
> > +#endif /* CONFIG_RISCV */
> > +
> > #ifdef CONFIG_X86_64
> >
> > #include <asm/ibt.h>
> > --
> > 2.36.1
> >
> >
>
> Regards,
> Evgenii
>
>
--
Thanks,
Song
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-01-17 9:32 ` Song Shuai
@ 2023-01-17 13:16 ` Evgenii Shatokhin
2023-01-17 16:22 ` Evgenii Shatokhin
0 siblings, 1 reply; 44+ messages in thread
From: Evgenii Shatokhin @ 2023-01-17 13:16 UTC (permalink / raw)
To: Song Shuai
Cc: guoren, linux-riscv, linux-kernel, anup, paul.walmsley, palmer,
conor.dooley, heiko, rostedt, mhiramat, jolsa, bp, jpoimboe,
andy.chiu, linux
Hi, Song,
On 17.01.2023 12:32, Song Shuai wrote:
>
> Hi, Evgenii:
>
> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
>
>>
>> Hi,
>>
>> On 12.01.2023 12:06, guoren@kernel.org wrote:
>>> From: Song Shuai <suagrfillet@gmail.com>
>>>
>>> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
>>> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
>>> the ftrace-direct*.c files in samples/ftrace/.
>>>
>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
>>> Tested-by: Guo Ren <guoren@kernel.org>
>>> Signed-off-by: Guo Ren <guoren@kernel.org>
>>> ---
>>> arch/riscv/Kconfig | 2 ++
>>> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
>>> samples/ftrace/ftrace-direct-multi-modify.c | 37 +++++++++++++++++++++
>>> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
>>> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
>>> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
>>> 6 files changed, 142 insertions(+)
>>
>> The samples were built OK now, but ftrace-direct-multi and
>> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
>>
>> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
>> the messages in the trace:
>>
>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
>> # | | | ||||| | |
>> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
>> direct func1 ip 0
>> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
>> direct func1 ip ff60000001ba9600
>> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
>> direct func1 ip ff60000003334d00
>> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
>> direct func1 ip 0
>> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
>> direct func1 ip 0
>> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
>> direct func1 ip 7fffffffffffffff
>> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
>> direct func1 ip ff60000001ba9600
>> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
>> direct func1 ip ff60000001ba9600
>> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
>> direct func1 ip 0
>> [...]
>>
>> If I understand it right, my_direct_func1() should print the address of
>> some location in the code, probably - at the beginning of the traced
>> functions.
>>
>> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
>> addresses.
>>
> The invalid code address is only printed by accessing the schedule()
> function's first argument whose address stores in a0 register.
> While schedule() actually has no parameter declared, so my_direct_func
> just prints the a0 in the context of the schedule()'s caller and
> the address maybe varies depending on the caller.
>
> I can't really understand why tracing the first argument of the
> schedule() function, but it seems nonsense at this point.
The question is, what should be passed as the argument(s) of
my_direct_func() in this particular sample module. The kernel docs and
commit logs seem to contain no info on that.
With direct functions, I suppose, the trampoline can pass anything it
wants to my_direct_func(), not just the arguments of the traced function.
I'd check what these sample modules do on x86 and would try to match
that behaviour on RISC-V.
>
> As for this patch, it just impls a simple mcount (direct_caller) to
> trace kernel functions, and basically saves the necessary ABI,
> call the tracing function, and restores the ABI, just like other arches do.
> so It shouldn't be blamed.
>
> I started an independent patch to replace schedule with kick_process
> to make these samples more reasonable. And It has no conflict with the
> current patch, so we can go on.
>
> Link: https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
>
>> The same issue is with ftrace-direct-multi-modify.ko.
>>
>> Is anything missing here?
>>
>>>
>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>> index 307a9f413edd..e944af44f681 100644
>>> --- a/arch/riscv/Kconfig
>>> +++ b/arch/riscv/Kconfig
>>> @@ -112,6 +112,8 @@ config RISCV
>>> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
>>> select HAVE_REGS_AND_STACK_ACCESS_API
>>> select HAVE_FUNCTION_ARG_ACCESS_API
>>> + select HAVE_SAMPLE_FTRACE_DIRECT
>>> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
>>> select HAVE_STACKPROTECTOR
>>> select HAVE_SYSCALL_TRACEPOINTS
>>> select HAVE_RSEQ
>>> diff --git a/samples/ftrace/ftrace-direct-modify.c b/samples/ftrace/ftrace-direct-modify.c
>>> index de5a0f67f320..be7bf472c3c7 100644
>>> --- a/samples/ftrace/ftrace-direct-modify.c
>>> +++ b/samples/ftrace/ftrace-direct-modify.c
>>> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
>>>
>>> static unsigned long my_ip = (unsigned long)schedule;
>>>
>>> +#ifdef CONFIG_RISCV
>>> +
>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>> +" .type my_tramp1, @function\n"
>>> +" .globl my_tramp1\n"
>>> +" my_tramp1:\n"
>>> +" addi sp,sp,-16\n"
>>> +" sd t0,0(sp)\n"
>>> +" sd ra,8(sp)\n"
>>> +" call my_direct_func1\n"
>>> +" ld t0,0(sp)\n"
>>> +" ld ra,8(sp)\n"
>>> +" addi sp,sp,16\n"
>>> +" jr t0\n"
>>> +" .size my_tramp1, .-my_tramp1\n"
>>> +
>>> +" .type my_tramp2, @function\n"
>>> +" .globl my_tramp2\n"
>>> +" my_tramp2:\n"
>>> +" addi sp,sp,-16\n"
>>> +" sd t0,0(sp)\n"
>>> +" sd ra,8(sp)\n"
>>> +" call my_direct_func2\n"
>>> +" ld t0,0(sp)\n"
>>> +" ld ra,8(sp)\n"
>>> +" addi sp,sp,16\n"
>>> +" jr t0\n"
>>> +" .size my_tramp2, .-my_tramp2\n"
>>> +" .popsection\n"
>>> +);
>>> +
>>> +#endif /* CONFIG_RISCV */
>>> +
>>> #ifdef CONFIG_X86_64
>>>
>>> #include <asm/ibt.h>
>>> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
>>> index d52370cad0b6..10884bf418f7 100644
>>> --- a/samples/ftrace/ftrace-direct-multi-modify.c
>>> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
>>> @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
>>> extern void my_tramp1(void *);
>>> extern void my_tramp2(void *);
>>>
>>> +#ifdef CONFIG_RISCV
>>> +
>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>> +" .type my_tramp1, @function\n"
>>> +" .globl my_tramp1\n"
>>> +" my_tramp1:\n"
>>> +" addi sp,sp,-24\n"
>>> +" sd a0,0(sp)\n"
>>> +" sd t0,8(sp)\n"
>>> +" sd ra,16(sp)\n"
>>> +" call my_direct_func1\n"
>>> +" ld a0,0(sp)\n"
>>> +" ld t0,8(sp)\n"
>>> +" ld ra,16(sp)\n"
>>> +" addi sp,sp,24\n"
>>> +" jr t0\n"
>>> +" .size my_tramp1, .-my_tramp1\n"
>>> +
>>> +" .type my_tramp2, @function\n"
>>> +" .globl my_tramp2\n"
>>> +" my_tramp2:\n"
>>> +" addi sp,sp,-24\n"
>>> +" sd a0,0(sp)\n"
>>> +" sd t0,8(sp)\n"
>>> +" sd ra,16(sp)\n"
>>> +" call my_direct_func2\n"
>>> +" ld a0,0(sp)\n"
>>> +" ld t0,8(sp)\n"
>>> +" ld ra,16(sp)\n"
>>> +" addi sp,sp,24\n"
>>> +" jr t0\n"
>>> +" .size my_tramp2, .-my_tramp2\n"
>>> +" .popsection\n"
>>> +);
>>> +
>>> +#endif /* CONFIG_RISCV */
>>> +
>>> #ifdef CONFIG_X86_64
>>>
>>> #include <asm/ibt.h>
>>> diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
>>> index ec1088922517..a35bf43bf6d7 100644
>>> --- a/samples/ftrace/ftrace-direct-multi.c
>>> +++ b/samples/ftrace/ftrace-direct-multi.c
>>> @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
>>>
>>> extern void my_tramp(void *);
>>>
>>> +#ifdef CONFIG_RISCV
>>> +
>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>> +" .type my_tramp, @function\n"
>>> +" .globl my_tramp\n"
>>> +" my_tramp:\n"
>>> +" addi sp,sp,-24\n"
>>> +" sd a0,0(sp)\n"
>>> +" sd t0,8(sp)\n"
>>> +" sd ra,16(sp)\n"
>>> +" call my_direct_func\n"
>>> +" ld a0,0(sp)\n"
>>> +" ld t0,8(sp)\n"
>>> +" ld ra,16(sp)\n"
>>> +" addi sp,sp,24\n"
>>> +" jr t0\n"
>>> +" .size my_tramp, .-my_tramp\n"
>>> +" .popsection\n"
>>> +);
>>> +
>>> +#endif /* CONFIG_RISCV */
>>> +
>>> #ifdef CONFIG_X86_64
>>>
>>> #include <asm/ibt.h>
>>> diff --git a/samples/ftrace/ftrace-direct-too.c b/samples/ftrace/ftrace-direct-too.c
>>> index e13fb59a2b47..3b62e33c2e6d 100644
>>> --- a/samples/ftrace/ftrace-direct-too.c
>>> +++ b/samples/ftrace/ftrace-direct-too.c
>>> @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
>>>
>>> extern void my_tramp(void *);
>>>
>>> +#ifdef CONFIG_RISCV
>>> +
>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>> +" .type my_tramp, @function\n"
>>> +" .globl my_tramp\n"
>>> +" my_tramp:\n"
>>> +" addi sp,sp,-40\n"
>>> +" sd a0,0(sp)\n"
>>> +" sd a1,8(sp)\n"
>>> +" sd a2,16(sp)\n"
>>> +" sd t0,24(sp)\n"
>>> +" sd ra,32(sp)\n"
>>> +" call my_direct_func\n"
>>> +" ld a0,0(sp)\n"
>>> +" ld a1,8(sp)\n"
>>> +" ld a2,16(sp)\n"
>>> +" ld t0,24(sp)\n"
>>> +" ld ra,32(sp)\n"
>>> +" addi sp,sp,40\n"
>>> +" jr t0\n"
>>> +" .size my_tramp, .-my_tramp\n"
>>> +" .popsection\n"
>>> +);
>>> +
>>> +#endif /* CONFIG_RISCV */
>>> +
>>> #ifdef CONFIG_X86_64
>>>
>>> #include <asm/ibt.h>
>>> diff --git a/samples/ftrace/ftrace-direct.c b/samples/ftrace/ftrace-direct.c
>>> index 1f769d0db20f..2cfe5a7d2d70 100644
>>> --- a/samples/ftrace/ftrace-direct.c
>>> +++ b/samples/ftrace/ftrace-direct.c
>>> @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
>>>
>>> extern void my_tramp(void *);
>>>
>>> +#ifdef CONFIG_RISCV
>>> +
>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>> +" .type my_tramp, @function\n"
>>> +" .globl my_tramp\n"
>>> +" my_tramp:\n"
>>> +" addi sp,sp,-24\n"
>>> +" sd a0,0(sp)\n"
>>> +" sd t0,8(sp)\n"
>>> +" sd ra,16(sp)\n"
>>> +" call my_direct_func\n"
>>> +" ld a0,0(sp)\n"
>>> +" ld t0,8(sp)\n"
>>> +" ld ra,16(sp)\n"
>>> +" addi sp,sp,24\n"
>>> +" jr t0\n"
>>> +" .size my_tramp, .-my_tramp\n"
>>> +" .popsection\n"
>>> +);
>>> +
>>> +#endif /* CONFIG_RISCV */
>>> +
>>> #ifdef CONFIG_X86_64
>>>
>>> #include <asm/ibt.h>
>>> --
>>> 2.36.1
>
> --
> Thanks,
> Song
>
Regards,
Evgenii
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-01-17 13:16 ` Evgenii Shatokhin
@ 2023-01-17 16:22 ` Evgenii Shatokhin
2023-01-18 2:37 ` Song Shuai
0 siblings, 1 reply; 44+ messages in thread
From: Evgenii Shatokhin @ 2023-01-17 16:22 UTC (permalink / raw)
To: Song Shuai
Cc: guoren, linux-riscv, linux-kernel, anup, paul.walmsley, palmer,
conor.dooley, heiko, rostedt, mhiramat, jolsa, bp, jpoimboe,
andy.chiu, linux
On 17.01.2023 16:16, Evgenii Shatokhin wrote:
> Hi, Song,
>
> On 17.01.2023 12:32, Song Shuai wrote:
>>
>> Hi, Evgenii:
>>
>> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
>>
>>>
>>> Hi,
>>>
>>> On 12.01.2023 12:06, guoren@kernel.org wrote:
>>>> From: Song Shuai <suagrfillet@gmail.com>
>>>>
>>>> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
>>>> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
>>>> the ftrace-direct*.c files in samples/ftrace/.
>>>>
>>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
>>>> Tested-by: Guo Ren <guoren@kernel.org>
>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
>>>> ---
>>>> arch/riscv/Kconfig | 2 ++
>>>> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
>>>> samples/ftrace/ftrace-direct-multi-modify.c | 37
>>>> +++++++++++++++++++++
>>>> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
>>>> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
>>>> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
>>>> 6 files changed, 142 insertions(+)
>>>
>>> The samples were built OK now, but ftrace-direct-multi and
>>> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
>>>
>>> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
>>> the messages in the trace:
>>>
>>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
>>> # | | | ||||| | |
>>> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
>>> direct func1 ip 0
>>> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
>>> direct func1 ip ff60000001ba9600
>>> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
>>> direct func1 ip ff60000003334d00
>>> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
>>> direct func1 ip 0
>>> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
>>> direct func1 ip 0
>>> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
>>> direct func1 ip 7fffffffffffffff
>>> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
>>> direct func1 ip ff60000001ba9600
>>> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
>>> direct func1 ip ff60000001ba9600
>>> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
>>> direct func1 ip 0
>>> [...]
>>>
>>> If I understand it right, my_direct_func1() should print the address of
>>> some location in the code, probably - at the beginning of the traced
>>> functions.
>>>
>>> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
>>> addresses.
>>>
>> The invalid code address is only printed by accessing the schedule()
>> function's first argument whose address stores in a0 register.
>> While schedule() actually has no parameter declared, so my_direct_func
>> just prints the a0 in the context of the schedule()'s caller and
>> the address maybe varies depending on the caller.
>>
>> I can't really understand why tracing the first argument of the
>> schedule() function, but it seems nonsense at this point.
>
> The question is, what should be passed as the argument(s) of
> my_direct_func() in this particular sample module. The kernel docs and
> commit logs seem to contain no info on that.
>
> With direct functions, I suppose, the trampoline can pass anything it
> wants to my_direct_func(), not just the arguments of the traced function.
>
> I'd check what these sample modules do on x86 and would try to match
> that behaviour on RISC-V.
I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko
on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should
be passed to my_direct_func().
ftrace-direct-multi.ko:
# TASK-PID CPU# ||||| TIMESTAMP FUNCTION
# | | | ||||| | |
insmod-10829 [000] d.h1. 1719.518535: my_direct_func: ip
ffffffff87332f45 // wake_up_process+0x5
rcu_tasks_kthre-11 [000] ..... 1719.518696: my_direct_func: ip
ffffffff8828d935 // schedule+0x5
insmod-10829 [000] ..... 1719.518708: my_direct_func: ip
ffffffff8828d935
systemd-journal-293 [001] ..... 1719.518823: my_direct_func: ip
ffffffff8828d935
systemd-1 [000] ..... 1719.519141: my_direct_func: ip
ffffffff8828d935
<idle>-0 [001] ..s1. 1719.521889: my_direct_func: ip
ffffffff87332f45
<idle>-0 [000] d.s2. 1719.521901: my_direct_func: ip
ffffffff87332f45
rcu_preempt-15 [001] ..... 1719.521917: my_direct_func: ip
ffffffff8828d935
[...]
The ip values are wake_up_process+0x5 and schedule+0x5, the locations
where the execution of the traced functions resumes after the Ftrace
trampoline has finished.
The results with ftrace-direct-multi-modify.ko are similar to that.
The samples look like a demonstration, that one can pass anything
necessary to the handler in case of "direct" functions.
I suppose, the RISC-V-specific asm code in these two sample modules
could be updated to pass the saved pc value to my_direct_func() in a0.
>
>>
>> As for this patch, it just impls a simple mcount (direct_caller) to
>> trace kernel functions, and basically saves the necessary ABI,
>> call the tracing function, and restores the ABI, just like other
>> arches do.
>> so It shouldn't be blamed.
>>
>> I started an independent patch to replace schedule with kick_process
>> to make these samples more reasonable. And It has no conflict with the
>> current patch, so we can go on.
>>
>> Link:
>> https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
>>
>>> The same issue is with ftrace-direct-multi-modify.ko.
>>>
>>> Is anything missing here?
>>>
>>>>
>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>>> index 307a9f413edd..e944af44f681 100644
>>>> --- a/arch/riscv/Kconfig
>>>> +++ b/arch/riscv/Kconfig
>>>> @@ -112,6 +112,8 @@ config RISCV
>>>> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
>>>> select HAVE_REGS_AND_STACK_ACCESS_API
>>>> select HAVE_FUNCTION_ARG_ACCESS_API
>>>> + select HAVE_SAMPLE_FTRACE_DIRECT
>>>> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
>>>> select HAVE_STACKPROTECTOR
>>>> select HAVE_SYSCALL_TRACEPOINTS
>>>> select HAVE_RSEQ
>>>> diff --git a/samples/ftrace/ftrace-direct-modify.c
>>>> b/samples/ftrace/ftrace-direct-modify.c
>>>> index de5a0f67f320..be7bf472c3c7 100644
>>>> --- a/samples/ftrace/ftrace-direct-modify.c
>>>> +++ b/samples/ftrace/ftrace-direct-modify.c
>>>> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
>>>>
>>>> static unsigned long my_ip = (unsigned long)schedule;
>>>>
>>>> +#ifdef CONFIG_RISCV
>>>> +
>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>> +" .type my_tramp1, @function\n"
>>>> +" .globl my_tramp1\n"
>>>> +" my_tramp1:\n"
>>>> +" addi sp,sp,-16\n"
>>>> +" sd t0,0(sp)\n"
>>>> +" sd ra,8(sp)\n"
>>>> +" call my_direct_func1\n"
>>>> +" ld t0,0(sp)\n"
>>>> +" ld ra,8(sp)\n"
>>>> +" addi sp,sp,16\n"
>>>> +" jr t0\n"
>>>> +" .size my_tramp1, .-my_tramp1\n"
>>>> +
>>>> +" .type my_tramp2, @function\n"
>>>> +" .globl my_tramp2\n"
>>>> +" my_tramp2:\n"
>>>> +" addi sp,sp,-16\n"
>>>> +" sd t0,0(sp)\n"
>>>> +" sd ra,8(sp)\n"
>>>> +" call my_direct_func2\n"
>>>> +" ld t0,0(sp)\n"
>>>> +" ld ra,8(sp)\n"
>>>> +" addi sp,sp,16\n"
>>>> +" jr t0\n"
>>>> +" .size my_tramp2, .-my_tramp2\n"
>>>> +" .popsection\n"
>>>> +);
>>>> +
>>>> +#endif /* CONFIG_RISCV */
>>>> +
>>>> #ifdef CONFIG_X86_64
>>>>
>>>> #include <asm/ibt.h>
>>>> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c
>>>> b/samples/ftrace/ftrace-direct-multi-modify.c
>>>> index d52370cad0b6..10884bf418f7 100644
>>>> --- a/samples/ftrace/ftrace-direct-multi-modify.c
>>>> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
>>>> @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
>>>> extern void my_tramp1(void *);
>>>> extern void my_tramp2(void *);
>>>>
>>>> +#ifdef CONFIG_RISCV
>>>> +
>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>> +" .type my_tramp1, @function\n"
>>>> +" .globl my_tramp1\n"
>>>> +" my_tramp1:\n"
>>>> +" addi sp,sp,-24\n"
>>>> +" sd a0,0(sp)\n"
>>>> +" sd t0,8(sp)\n"
>>>> +" sd ra,16(sp)\n"
>>>> +" call my_direct_func1\n"
>>>> +" ld a0,0(sp)\n"
>>>> +" ld t0,8(sp)\n"
>>>> +" ld ra,16(sp)\n"
>>>> +" addi sp,sp,24\n"
>>>> +" jr t0\n"
>>>> +" .size my_tramp1, .-my_tramp1\n"
>>>> +
>>>> +" .type my_tramp2, @function\n"
>>>> +" .globl my_tramp2\n"
>>>> +" my_tramp2:\n"
>>>> +" addi sp,sp,-24\n"
>>>> +" sd a0,0(sp)\n"
>>>> +" sd t0,8(sp)\n"
>>>> +" sd ra,16(sp)\n"
>>>> +" call my_direct_func2\n"
>>>> +" ld a0,0(sp)\n"
>>>> +" ld t0,8(sp)\n"
>>>> +" ld ra,16(sp)\n"
>>>> +" addi sp,sp,24\n"
>>>> +" jr t0\n"
>>>> +" .size my_tramp2, .-my_tramp2\n"
>>>> +" .popsection\n"
>>>> +);
>>>> +
>>>> +#endif /* CONFIG_RISCV */
>>>> +
>>>> #ifdef CONFIG_X86_64
>>>>
>>>> #include <asm/ibt.h>
>>>> diff --git a/samples/ftrace/ftrace-direct-multi.c
>>>> b/samples/ftrace/ftrace-direct-multi.c
>>>> index ec1088922517..a35bf43bf6d7 100644
>>>> --- a/samples/ftrace/ftrace-direct-multi.c
>>>> +++ b/samples/ftrace/ftrace-direct-multi.c
>>>> @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
>>>>
>>>> extern void my_tramp(void *);
>>>>
>>>> +#ifdef CONFIG_RISCV
>>>> +
>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>> +" .type my_tramp, @function\n"
>>>> +" .globl my_tramp\n"
>>>> +" my_tramp:\n"
>>>> +" addi sp,sp,-24\n"
>>>> +" sd a0,0(sp)\n"
>>>> +" sd t0,8(sp)\n"
>>>> +" sd ra,16(sp)\n"
>>>> +" call my_direct_func\n"
>>>> +" ld a0,0(sp)\n"
>>>> +" ld t0,8(sp)\n"
>>>> +" ld ra,16(sp)\n"
>>>> +" addi sp,sp,24\n"
>>>> +" jr t0\n"
>>>> +" .size my_tramp, .-my_tramp\n"
>>>> +" .popsection\n"
>>>> +);
>>>> +
>>>> +#endif /* CONFIG_RISCV */
>>>> +
>>>> #ifdef CONFIG_X86_64
>>>>
>>>> #include <asm/ibt.h>
>>>> diff --git a/samples/ftrace/ftrace-direct-too.c
>>>> b/samples/ftrace/ftrace-direct-too.c
>>>> index e13fb59a2b47..3b62e33c2e6d 100644
>>>> --- a/samples/ftrace/ftrace-direct-too.c
>>>> +++ b/samples/ftrace/ftrace-direct-too.c
>>>> @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
>>>>
>>>> extern void my_tramp(void *);
>>>>
>>>> +#ifdef CONFIG_RISCV
>>>> +
>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>> +" .type my_tramp, @function\n"
>>>> +" .globl my_tramp\n"
>>>> +" my_tramp:\n"
>>>> +" addi sp,sp,-40\n"
>>>> +" sd a0,0(sp)\n"
>>>> +" sd a1,8(sp)\n"
>>>> +" sd a2,16(sp)\n"
>>>> +" sd t0,24(sp)\n"
>>>> +" sd ra,32(sp)\n"
>>>> +" call my_direct_func\n"
>>>> +" ld a0,0(sp)\n"
>>>> +" ld a1,8(sp)\n"
>>>> +" ld a2,16(sp)\n"
>>>> +" ld t0,24(sp)\n"
>>>> +" ld ra,32(sp)\n"
>>>> +" addi sp,sp,40\n"
>>>> +" jr t0\n"
>>>> +" .size my_tramp, .-my_tramp\n"
>>>> +" .popsection\n"
>>>> +);
>>>> +
>>>> +#endif /* CONFIG_RISCV */
>>>> +
>>>> #ifdef CONFIG_X86_64
>>>>
>>>> #include <asm/ibt.h>
>>>> diff --git a/samples/ftrace/ftrace-direct.c
>>>> b/samples/ftrace/ftrace-direct.c
>>>> index 1f769d0db20f..2cfe5a7d2d70 100644
>>>> --- a/samples/ftrace/ftrace-direct.c
>>>> +++ b/samples/ftrace/ftrace-direct.c
>>>> @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
>>>>
>>>> extern void my_tramp(void *);
>>>>
>>>> +#ifdef CONFIG_RISCV
>>>> +
>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>> +" .type my_tramp, @function\n"
>>>> +" .globl my_tramp\n"
>>>> +" my_tramp:\n"
>>>> +" addi sp,sp,-24\n"
>>>> +" sd a0,0(sp)\n"
>>>> +" sd t0,8(sp)\n"
>>>> +" sd ra,16(sp)\n"
>>>> +" call my_direct_func\n"
>>>> +" ld a0,0(sp)\n"
>>>> +" ld t0,8(sp)\n"
>>>> +" ld ra,16(sp)\n"
>>>> +" addi sp,sp,24\n"
>>>> +" jr t0\n"
>>>> +" .size my_tramp, .-my_tramp\n"
>>>> +" .popsection\n"
>>>> +);
>>>> +
>>>> +#endif /* CONFIG_RISCV */
>>>> +
>>>> #ifdef CONFIG_X86_64
>>>>
>>>> #include <asm/ibt.h>
>>>> --
>>>> 2.36.1
>>
>> --
>> Thanks,
>> Song
>>
>
> Regards,
> Evgenii
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-01-17 16:22 ` Evgenii Shatokhin
@ 2023-01-18 2:37 ` Song Shuai
2023-01-18 15:19 ` Evgenii Shatokhin
0 siblings, 1 reply; 44+ messages in thread
From: Song Shuai @ 2023-01-18 2:37 UTC (permalink / raw)
To: Evgenii Shatokhin, guoren
Cc: linux-riscv, linux-kernel, anup, paul.walmsley, palmer,
conor.dooley, heiko, rostedt, mhiramat, jolsa, bp, jpoimboe,
andy.chiu, linux
Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月17日周二 16:22写道：
>
> On 17.01.2023 16:16, Evgenii Shatokhin wrote:
> > Hi, Song,
> >
> > On 17.01.2023 12:32, Song Shuai wrote:
> >>
> >> Hi, Evgenii:
> >>
> >> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
> >>
> >>>
> >>> Hi,
> >>>
> >>> On 12.01.2023 12:06, guoren@kernel.org wrote:
> >>>> From: Song Shuai <suagrfillet@gmail.com>
> >>>>
> >>>> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> >>>> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
> >>>> the ftrace-direct*.c files in samples/ftrace/.
> >>>>
> >>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> >>>> Tested-by: Guo Ren <guoren@kernel.org>
> >>>> Signed-off-by: Guo Ren <guoren@kernel.org>
> >>>> ---
> >>>> arch/riscv/Kconfig | 2 ++
> >>>> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
> >>>> samples/ftrace/ftrace-direct-multi-modify.c | 37
> >>>> +++++++++++++++++++++
> >>>> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
> >>>> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
> >>>> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
> >>>> 6 files changed, 142 insertions(+)
> >>>
> >>> The samples were built OK now, but ftrace-direct-multi and
> >>> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
> >>>
> >>> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
> >>> the messages in the trace:
> >>>
> >>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> >>> # | | | ||||| | |
> >>> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
> >>> direct func1 ip 0
> >>> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
> >>> direct func1 ip ff60000001ba9600
> >>> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
> >>> direct func1 ip ff60000003334d00
> >>> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
> >>> direct func1 ip 0
> >>> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
> >>> direct func1 ip 0
> >>> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
> >>> direct func1 ip 7fffffffffffffff
> >>> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
> >>> direct func1 ip ff60000001ba9600
> >>> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
> >>> direct func1 ip ff60000001ba9600
> >>> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
> >>> direct func1 ip 0
> >>> [...]
> >>>
> >>> If I understand it right, my_direct_func1() should print the address of
> >>> some location in the code, probably - at the beginning of the traced
> >>> functions.
> >>>
> >>> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
> >>> addresses.
> >>>
> >> The invalid code address is only printed by accessing the schedule()
> >> function's first argument whose address stores in a0 register.
> >> While schedule() actually has no parameter declared, so my_direct_func
> >> just prints the a0 in the context of the schedule()'s caller and
> >> the address maybe varies depending on the caller.
> >>
> >> I can't really understand why tracing the first argument of the
> >> schedule() function, but it seems nonsense at this point.
> >
> > The question is, what should be passed as the argument(s) of
> > my_direct_func() in this particular sample module. The kernel docs and
> > commit logs seem to contain no info on that.
> >
> > With direct functions, I suppose, the trampoline can pass anything it
> > wants to my_direct_func(), not just the arguments of the traced function.
> >
> > I'd check what these sample modules do on x86 and would try to match
> > that behaviour on RISC-V.
>
> I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko
> on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should
> be passed to my_direct_func().
>
> ftrace-direct-multi.ko:
> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> # | | | ||||| | |
> insmod-10829 [000] d.h1. 1719.518535: my_direct_func: ip
> ffffffff87332f45 // wake_up_process+0x5
> rcu_tasks_kthre-11 [000] ..... 1719.518696: my_direct_func: ip
> ffffffff8828d935 // schedule+0x5
> insmod-10829 [000] ..... 1719.518708: my_direct_func: ip
> ffffffff8828d935
> systemd-journal-293 [001] ..... 1719.518823: my_direct_func: ip
> ffffffff8828d935
> systemd-1 [000] ..... 1719.519141: my_direct_func: ip
> ffffffff8828d935
> <idle>-0 [001] ..s1. 1719.521889: my_direct_func: ip
> ffffffff87332f45
> <idle>-0 [000] d.s2. 1719.521901: my_direct_func: ip
> ffffffff87332f45
> rcu_preempt-15 [001] ..... 1719.521917: my_direct_func: ip
> ffffffff8828d935
> [...]
>
> The ip values are wake_up_process+0x5 and schedule+0x5, the locations
> where the execution of the traced functions resumes after the Ftrace
> trampoline has finished.
>
> The results with ftrace-direct-multi-modify.ko are similar to that.
>
> The samples look like a demonstration, that one can pass anything
> necessary to the handler in case of "direct" functions.
>
> I suppose, the RISC-V-specific asm code in these two sample modules
> could be updated to pass the saved pc value to my_direct_func() in a0.
Yes, you're right.
I added 'mv a0,t0' in front of `call my_direct_func` to pass the address of
traced function with mcount offset.
Here is the updated patch for your reference.
https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318
>
> >
> >>
> >> As for this patch, it just impls a simple mcount (direct_caller) to
> >> trace kernel functions, and basically saves the necessary ABI,
> >> call the tracing function, and restores the ABI, just like other
> >> arches do.
> >> so It shouldn't be blamed.
> >>
> >> I started an independent patch to replace schedule with kick_process
> >> to make these samples more reasonable. And It has no conflict with the
> >> current patch, so we can go on.
> >>
> >> Link:
> >> https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
> >>
> >>> The same issue is with ftrace-direct-multi-modify.ko.
> >>>
> >>> Is anything missing here?
> >>>
> >>>>
> >>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> >>>> index 307a9f413edd..e944af44f681 100644
> >>>> --- a/arch/riscv/Kconfig
> >>>> +++ b/arch/riscv/Kconfig
> >>>> @@ -112,6 +112,8 @@ config RISCV
> >>>> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
> >>>> select HAVE_REGS_AND_STACK_ACCESS_API
> >>>> select HAVE_FUNCTION_ARG_ACCESS_API
> >>>> + select HAVE_SAMPLE_FTRACE_DIRECT
> >>>> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> >>>> select HAVE_STACKPROTECTOR
> >>>> select HAVE_SYSCALL_TRACEPOINTS
> >>>> select HAVE_RSEQ
> >>>> diff --git a/samples/ftrace/ftrace-direct-modify.c
> >>>> b/samples/ftrace/ftrace-direct-modify.c
> >>>> index de5a0f67f320..be7bf472c3c7 100644
> >>>> --- a/samples/ftrace/ftrace-direct-modify.c
> >>>> +++ b/samples/ftrace/ftrace-direct-modify.c
> >>>> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
> >>>>
> >>>> static unsigned long my_ip = (unsigned long)schedule;
> >>>>
> >>>> +#ifdef CONFIG_RISCV
> >>>> +
> >>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>> +" .type my_tramp1, @function\n"
> >>>> +" .globl my_tramp1\n"
> >>>> +" my_tramp1:\n"
> >>>> +" addi sp,sp,-16\n"
> >>>> +" sd t0,0(sp)\n"
> >>>> +" sd ra,8(sp)\n"
> >>>> +" call my_direct_func1\n"
> >>>> +" ld t0,0(sp)\n"
> >>>> +" ld ra,8(sp)\n"
> >>>> +" addi sp,sp,16\n"
> >>>> +" jr t0\n"
> >>>> +" .size my_tramp1, .-my_tramp1\n"
> >>>> +
> >>>> +" .type my_tramp2, @function\n"
> >>>> +" .globl my_tramp2\n"
> >>>> +" my_tramp2:\n"
> >>>> +" addi sp,sp,-16\n"
> >>>> +" sd t0,0(sp)\n"
> >>>> +" sd ra,8(sp)\n"
> >>>> +" call my_direct_func2\n"
> >>>> +" ld t0,0(sp)\n"
> >>>> +" ld ra,8(sp)\n"
> >>>> +" addi sp,sp,16\n"
> >>>> +" jr t0\n"
> >>>> +" .size my_tramp2, .-my_tramp2\n"
> >>>> +" .popsection\n"
> >>>> +);
> >>>> +
> >>>> +#endif /* CONFIG_RISCV */
> >>>> +
> >>>> #ifdef CONFIG_X86_64
> >>>>
> >>>> #include <asm/ibt.h>
> >>>> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c
> >>>> b/samples/ftrace/ftrace-direct-multi-modify.c
> >>>> index d52370cad0b6..10884bf418f7 100644
> >>>> --- a/samples/ftrace/ftrace-direct-multi-modify.c
> >>>> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> >>>> @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
> >>>> extern void my_tramp1(void *);
> >>>> extern void my_tramp2(void *);
> >>>>
> >>>> +#ifdef CONFIG_RISCV
> >>>> +
> >>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>> +" .type my_tramp1, @function\n"
> >>>> +" .globl my_tramp1\n"
> >>>> +" my_tramp1:\n"
> >>>> +" addi sp,sp,-24\n"
> >>>> +" sd a0,0(sp)\n"
> >>>> +" sd t0,8(sp)\n"
> >>>> +" sd ra,16(sp)\n"
> >>>> +" call my_direct_func1\n"
> >>>> +" ld a0,0(sp)\n"
> >>>> +" ld t0,8(sp)\n"
> >>>> +" ld ra,16(sp)\n"
> >>>> +" addi sp,sp,24\n"
> >>>> +" jr t0\n"
> >>>> +" .size my_tramp1, .-my_tramp1\n"
> >>>> +
> >>>> +" .type my_tramp2, @function\n"
> >>>> +" .globl my_tramp2\n"
> >>>> +" my_tramp2:\n"
> >>>> +" addi sp,sp,-24\n"
> >>>> +" sd a0,0(sp)\n"
> >>>> +" sd t0,8(sp)\n"
> >>>> +" sd ra,16(sp)\n"
> >>>> +" call my_direct_func2\n"
> >>>> +" ld a0,0(sp)\n"
> >>>> +" ld t0,8(sp)\n"
> >>>> +" ld ra,16(sp)\n"
> >>>> +" addi sp,sp,24\n"
> >>>> +" jr t0\n"
> >>>> +" .size my_tramp2, .-my_tramp2\n"
> >>>> +" .popsection\n"
> >>>> +);
> >>>> +
> >>>> +#endif /* CONFIG_RISCV */
> >>>> +
> >>>> #ifdef CONFIG_X86_64
> >>>>
> >>>> #include <asm/ibt.h>
> >>>> diff --git a/samples/ftrace/ftrace-direct-multi.c
> >>>> b/samples/ftrace/ftrace-direct-multi.c
> >>>> index ec1088922517..a35bf43bf6d7 100644
> >>>> --- a/samples/ftrace/ftrace-direct-multi.c
> >>>> +++ b/samples/ftrace/ftrace-direct-multi.c
> >>>> @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
> >>>>
> >>>> extern void my_tramp(void *);
> >>>>
> >>>> +#ifdef CONFIG_RISCV
> >>>> +
> >>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>> +" .type my_tramp, @function\n"
> >>>> +" .globl my_tramp\n"
> >>>> +" my_tramp:\n"
> >>>> +" addi sp,sp,-24\n"
> >>>> +" sd a0,0(sp)\n"
> >>>> +" sd t0,8(sp)\n"
> >>>> +" sd ra,16(sp)\n"
> >>>> +" call my_direct_func\n"
> >>>> +" ld a0,0(sp)\n"
> >>>> +" ld t0,8(sp)\n"
> >>>> +" ld ra,16(sp)\n"
> >>>> +" addi sp,sp,24\n"
> >>>> +" jr t0\n"
> >>>> +" .size my_tramp, .-my_tramp\n"
> >>>> +" .popsection\n"
> >>>> +);
> >>>> +
> >>>> +#endif /* CONFIG_RISCV */
> >>>> +
> >>>> #ifdef CONFIG_X86_64
> >>>>
> >>>> #include <asm/ibt.h>
> >>>> diff --git a/samples/ftrace/ftrace-direct-too.c
> >>>> b/samples/ftrace/ftrace-direct-too.c
> >>>> index e13fb59a2b47..3b62e33c2e6d 100644
> >>>> --- a/samples/ftrace/ftrace-direct-too.c
> >>>> +++ b/samples/ftrace/ftrace-direct-too.c
> >>>> @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
> >>>>
> >>>> extern void my_tramp(void *);
> >>>>
> >>>> +#ifdef CONFIG_RISCV
> >>>> +
> >>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>> +" .type my_tramp, @function\n"
> >>>> +" .globl my_tramp\n"
> >>>> +" my_tramp:\n"
> >>>> +" addi sp,sp,-40\n"
> >>>> +" sd a0,0(sp)\n"
> >>>> +" sd a1,8(sp)\n"
> >>>> +" sd a2,16(sp)\n"
> >>>> +" sd t0,24(sp)\n"
> >>>> +" sd ra,32(sp)\n"
> >>>> +" call my_direct_func\n"
> >>>> +" ld a0,0(sp)\n"
> >>>> +" ld a1,8(sp)\n"
> >>>> +" ld a2,16(sp)\n"
> >>>> +" ld t0,24(sp)\n"
> >>>> +" ld ra,32(sp)\n"
> >>>> +" addi sp,sp,40\n"
> >>>> +" jr t0\n"
> >>>> +" .size my_tramp, .-my_tramp\n"
> >>>> +" .popsection\n"
> >>>> +);
> >>>> +
> >>>> +#endif /* CONFIG_RISCV */
> >>>> +
> >>>> #ifdef CONFIG_X86_64
> >>>>
> >>>> #include <asm/ibt.h>
> >>>> diff --git a/samples/ftrace/ftrace-direct.c
> >>>> b/samples/ftrace/ftrace-direct.c
> >>>> index 1f769d0db20f..2cfe5a7d2d70 100644
> >>>> --- a/samples/ftrace/ftrace-direct.c
> >>>> +++ b/samples/ftrace/ftrace-direct.c
> >>>> @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
> >>>>
> >>>> extern void my_tramp(void *);
> >>>>
> >>>> +#ifdef CONFIG_RISCV
> >>>> +
> >>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>> +" .type my_tramp, @function\n"
> >>>> +" .globl my_tramp\n"
> >>>> +" my_tramp:\n"
> >>>> +" addi sp,sp,-24\n"
> >>>> +" sd a0,0(sp)\n"
> >>>> +" sd t0,8(sp)\n"
> >>>> +" sd ra,16(sp)\n"
> >>>> +" call my_direct_func\n"
> >>>> +" ld a0,0(sp)\n"
> >>>> +" ld t0,8(sp)\n"
> >>>> +" ld ra,16(sp)\n"
> >>>> +" addi sp,sp,24\n"
> >>>> +" jr t0\n"
> >>>> +" .size my_tramp, .-my_tramp\n"
> >>>> +" .popsection\n"
> >>>> +);
> >>>> +
> >>>> +#endif /* CONFIG_RISCV */
> >>>> +
> >>>> #ifdef CONFIG_X86_64
> >>>>
> >>>> #include <asm/ibt.h>
> >>>> --
> >>>> 2.36.1
> >>
> >> --
> >> Thanks,
> >> Song
> >>
> >
> > Regards,
> > Evgenii
>
>
--
Thanks,
Song
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-01-18 2:37 ` Song Shuai
@ 2023-01-18 15:19 ` Evgenii Shatokhin
2023-01-19 6:05 ` Guo Ren
0 siblings, 1 reply; 44+ messages in thread
From: Evgenii Shatokhin @ 2023-01-18 15:19 UTC (permalink / raw)
To: Song Shuai, guoren
Cc: linux-riscv, linux-kernel, anup, paul.walmsley, palmer,
conor.dooley, heiko, rostedt, mhiramat, jolsa, bp, jpoimboe,
andy.chiu, linux
On 18.01.2023 05:37, Song Shuai wrote:
> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月17日周二 16:22写道：
>>
>> On 17.01.2023 16:16, Evgenii Shatokhin wrote:
>>> Hi, Song,
>>>
>>> On 17.01.2023 12:32, Song Shuai wrote:
>>>>
>>>> Hi, Evgenii:
>>>>
>>>> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 12.01.2023 12:06, guoren@kernel.org wrote:
>>>>>> From: Song Shuai <suagrfillet@gmail.com>
>>>>>>
>>>>>> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
>>>>>> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
>>>>>> the ftrace-direct*.c files in samples/ftrace/.
>>>>>>
>>>>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
>>>>>> Tested-by: Guo Ren <guoren@kernel.org>
>>>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
>>>>>> ---
>>>>>> arch/riscv/Kconfig | 2 ++
>>>>>> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
>>>>>> samples/ftrace/ftrace-direct-multi-modify.c | 37
>>>>>> +++++++++++++++++++++
>>>>>> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
>>>>>> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
>>>>>> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
>>>>>> 6 files changed, 142 insertions(+)
>>>>>
>>>>> The samples were built OK now, but ftrace-direct-multi and
>>>>> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
>>>>>
>>>>> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
>>>>> the messages in the trace:
>>>>>
>>>>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
>>>>> # | | | ||||| | |
>>>>> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
>>>>> direct func1 ip 0
>>>>> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
>>>>> direct func1 ip ff60000001ba9600
>>>>> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
>>>>> direct func1 ip ff60000003334d00
>>>>> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
>>>>> direct func1 ip 0
>>>>> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
>>>>> direct func1 ip 0
>>>>> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
>>>>> direct func1 ip 7fffffffffffffff
>>>>> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
>>>>> direct func1 ip ff60000001ba9600
>>>>> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
>>>>> direct func1 ip ff60000001ba9600
>>>>> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
>>>>> direct func1 ip 0
>>>>> [...]
>>>>>
>>>>> If I understand it right, my_direct_func1() should print the address of
>>>>> some location in the code, probably - at the beginning of the traced
>>>>> functions.
>>>>>
>>>>> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
>>>>> addresses.
>>>>>
>>>> The invalid code address is only printed by accessing the schedule()
>>>> function's first argument whose address stores in a0 register.
>>>> While schedule() actually has no parameter declared, so my_direct_func
>>>> just prints the a0 in the context of the schedule()'s caller and
>>>> the address maybe varies depending on the caller.
>>>>
>>>> I can't really understand why tracing the first argument of the
>>>> schedule() function, but it seems nonsense at this point.
>>>
>>> The question is, what should be passed as the argument(s) of
>>> my_direct_func() in this particular sample module. The kernel docs and
>>> commit logs seem to contain no info on that.
>>>
>>> With direct functions, I suppose, the trampoline can pass anything it
>>> wants to my_direct_func(), not just the arguments of the traced function.
>>>
>>> I'd check what these sample modules do on x86 and would try to match
>>> that behaviour on RISC-V.
>>
>> I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko
>> on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should
>> be passed to my_direct_func().
>>
>> ftrace-direct-multi.ko:
>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
>> # | | | ||||| | |
>> insmod-10829 [000] d.h1. 1719.518535: my_direct_func: ip
>> ffffffff87332f45 // wake_up_process+0x5
>> rcu_tasks_kthre-11 [000] ..... 1719.518696: my_direct_func: ip
>> ffffffff8828d935 // schedule+0x5
>> insmod-10829 [000] ..... 1719.518708: my_direct_func: ip
>> ffffffff8828d935
>> systemd-journal-293 [001] ..... 1719.518823: my_direct_func: ip
>> ffffffff8828d935
>> systemd-1 [000] ..... 1719.519141: my_direct_func: ip
>> ffffffff8828d935
>> <idle>-0 [001] ..s1. 1719.521889: my_direct_func: ip
>> ffffffff87332f45
>> <idle>-0 [000] d.s2. 1719.521901: my_direct_func: ip
>> ffffffff87332f45
>> rcu_preempt-15 [001] ..... 1719.521917: my_direct_func: ip
>> ffffffff8828d935
>> [...]
>>
>> The ip values are wake_up_process+0x5 and schedule+0x5, the locations
>> where the execution of the traced functions resumes after the Ftrace
>> trampoline has finished.
>>
>> The results with ftrace-direct-multi-modify.ko are similar to that.
>>
>> The samples look like a demonstration, that one can pass anything
>> necessary to the handler in case of "direct" functions.
>>
>> I suppose, the RISC-V-specific asm code in these two sample modules
>> could be updated to pass the saved pc value to my_direct_func() in a0.
>
> Yes, you're right.
>
> I added 'mv a0,t0' in front of `call my_direct_func` to pass the address of
> traced function with mcount offset.
>
> Here is the updated patch for your reference.
> https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318
Thank you for the quick fix. This one looks good to me.
ftrace-direct-multi*.ko now report the ip values corresponding to
schedule+0x8 and wake_up_process+0x8, which is what was expected here.
One more thing: please change my "Co-developed-by:" into "Tested-by:" in
your patch, becase this is what I actually did: tested it and reported
the results. I cannot take your credit for development of this patch ;-)
Looking forward for v8 of the series.
>
>
>>
>>>
>>>>
>>>> As for this patch, it just impls a simple mcount (direct_caller) to
>>>> trace kernel functions, and basically saves the necessary ABI,
>>>> call the tracing function, and restores the ABI, just like other
>>>> arches do.
>>>> so It shouldn't be blamed.
>>>>
>>>> I started an independent patch to replace schedule with kick_process
>>>> to make these samples more reasonable. And It has no conflict with the
>>>> current patch, so we can go on.
>>>>
>>>> Link:
>>>> https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
>>>>
>>>>> The same issue is with ftrace-direct-multi-modify.ko.
>>>>>
>>>>> Is anything missing here?
>>>>>
>>>>>>
>>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>>>>> index 307a9f413edd..e944af44f681 100644
>>>>>> --- a/arch/riscv/Kconfig
>>>>>> +++ b/arch/riscv/Kconfig
>>>>>> @@ -112,6 +112,8 @@ config RISCV
>>>>>> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
>>>>>> select HAVE_REGS_AND_STACK_ACCESS_API
>>>>>> select HAVE_FUNCTION_ARG_ACCESS_API
>>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT
>>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
>>>>>> select HAVE_STACKPROTECTOR
>>>>>> select HAVE_SYSCALL_TRACEPOINTS
>>>>>> select HAVE_RSEQ
>>>>>> diff --git a/samples/ftrace/ftrace-direct-modify.c
>>>>>> b/samples/ftrace/ftrace-direct-modify.c
>>>>>> index de5a0f67f320..be7bf472c3c7 100644
>>>>>> --- a/samples/ftrace/ftrace-direct-modify.c
>>>>>> +++ b/samples/ftrace/ftrace-direct-modify.c
>>>>>> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
>>>>>>
>>>>>> static unsigned long my_ip = (unsigned long)schedule;
>>>>>>
>>>>>> +#ifdef CONFIG_RISCV
>>>>>> +
>>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>>>> +" .type my_tramp1, @function\n"
>>>>>> +" .globl my_tramp1\n"
>>>>>> +" my_tramp1:\n"
>>>>>> +" addi sp,sp,-16\n"
>>>>>> +" sd t0,0(sp)\n"
>>>>>> +" sd ra,8(sp)\n"
>>>>>> +" call my_direct_func1\n"
>>>>>> +" ld t0,0(sp)\n"
>>>>>> +" ld ra,8(sp)\n"
>>>>>> +" addi sp,sp,16\n"
>>>>>> +" jr t0\n"
>>>>>> +" .size my_tramp1, .-my_tramp1\n"
>>>>>> +
>>>>>> +" .type my_tramp2, @function\n"
>>>>>> +" .globl my_tramp2\n"
>>>>>> +" my_tramp2:\n"
>>>>>> +" addi sp,sp,-16\n"
>>>>>> +" sd t0,0(sp)\n"
>>>>>> +" sd ra,8(sp)\n"
>>>>>> +" call my_direct_func2\n"
>>>>>> +" ld t0,0(sp)\n"
>>>>>> +" ld ra,8(sp)\n"
>>>>>> +" addi sp,sp,16\n"
>>>>>> +" jr t0\n"
>>>>>> +" .size my_tramp2, .-my_tramp2\n"
>>>>>> +" .popsection\n"
>>>>>> +);
>>>>>> +
>>>>>> +#endif /* CONFIG_RISCV */
>>>>>> +
>>>>>> #ifdef CONFIG_X86_64
>>>>>>
>>>>>> #include <asm/ibt.h>
>>>>>> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c
>>>>>> b/samples/ftrace/ftrace-direct-multi-modify.c
>>>>>> index d52370cad0b6..10884bf418f7 100644
>>>>>> --- a/samples/ftrace/ftrace-direct-multi-modify.c
>>>>>> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
>>>>>> @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
>>>>>> extern void my_tramp1(void *);
>>>>>> extern void my_tramp2(void *);
>>>>>>
>>>>>> +#ifdef CONFIG_RISCV
>>>>>> +
>>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>>>> +" .type my_tramp1, @function\n"
>>>>>> +" .globl my_tramp1\n"
>>>>>> +" my_tramp1:\n"
>>>>>> +" addi sp,sp,-24\n"
>>>>>> +" sd a0,0(sp)\n"
>>>>>> +" sd t0,8(sp)\n"
>>>>>> +" sd ra,16(sp)\n"
>>>>>> +" call my_direct_func1\n"
>>>>>> +" ld a0,0(sp)\n"
>>>>>> +" ld t0,8(sp)\n"
>>>>>> +" ld ra,16(sp)\n"
>>>>>> +" addi sp,sp,24\n"
>>>>>> +" jr t0\n"
>>>>>> +" .size my_tramp1, .-my_tramp1\n"
>>>>>> +
>>>>>> +" .type my_tramp2, @function\n"
>>>>>> +" .globl my_tramp2\n"
>>>>>> +" my_tramp2:\n"
>>>>>> +" addi sp,sp,-24\n"
>>>>>> +" sd a0,0(sp)\n"
>>>>>> +" sd t0,8(sp)\n"
>>>>>> +" sd ra,16(sp)\n"
>>>>>> +" call my_direct_func2\n"
>>>>>> +" ld a0,0(sp)\n"
>>>>>> +" ld t0,8(sp)\n"
>>>>>> +" ld ra,16(sp)\n"
>>>>>> +" addi sp,sp,24\n"
>>>>>> +" jr t0\n"
>>>>>> +" .size my_tramp2, .-my_tramp2\n"
>>>>>> +" .popsection\n"
>>>>>> +);
>>>>>> +
>>>>>> +#endif /* CONFIG_RISCV */
>>>>>> +
>>>>>> #ifdef CONFIG_X86_64
>>>>>>
>>>>>> #include <asm/ibt.h>
>>>>>> diff --git a/samples/ftrace/ftrace-direct-multi.c
>>>>>> b/samples/ftrace/ftrace-direct-multi.c
>>>>>> index ec1088922517..a35bf43bf6d7 100644
>>>>>> --- a/samples/ftrace/ftrace-direct-multi.c
>>>>>> +++ b/samples/ftrace/ftrace-direct-multi.c
>>>>>> @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
>>>>>>
>>>>>> extern void my_tramp(void *);
>>>>>>
>>>>>> +#ifdef CONFIG_RISCV
>>>>>> +
>>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>>>> +" .type my_tramp, @function\n"
>>>>>> +" .globl my_tramp\n"
>>>>>> +" my_tramp:\n"
>>>>>> +" addi sp,sp,-24\n"
>>>>>> +" sd a0,0(sp)\n"
>>>>>> +" sd t0,8(sp)\n"
>>>>>> +" sd ra,16(sp)\n"
>>>>>> +" call my_direct_func\n"
>>>>>> +" ld a0,0(sp)\n"
>>>>>> +" ld t0,8(sp)\n"
>>>>>> +" ld ra,16(sp)\n"
>>>>>> +" addi sp,sp,24\n"
>>>>>> +" jr t0\n"
>>>>>> +" .size my_tramp, .-my_tramp\n"
>>>>>> +" .popsection\n"
>>>>>> +);
>>>>>> +
>>>>>> +#endif /* CONFIG_RISCV */
>>>>>> +
>>>>>> #ifdef CONFIG_X86_64
>>>>>>
>>>>>> #include <asm/ibt.h>
>>>>>> diff --git a/samples/ftrace/ftrace-direct-too.c
>>>>>> b/samples/ftrace/ftrace-direct-too.c
>>>>>> index e13fb59a2b47..3b62e33c2e6d 100644
>>>>>> --- a/samples/ftrace/ftrace-direct-too.c
>>>>>> +++ b/samples/ftrace/ftrace-direct-too.c
>>>>>> @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
>>>>>>
>>>>>> extern void my_tramp(void *);
>>>>>>
>>>>>> +#ifdef CONFIG_RISCV
>>>>>> +
>>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>>>> +" .type my_tramp, @function\n"
>>>>>> +" .globl my_tramp\n"
>>>>>> +" my_tramp:\n"
>>>>>> +" addi sp,sp,-40\n"
>>>>>> +" sd a0,0(sp)\n"
>>>>>> +" sd a1,8(sp)\n"
>>>>>> +" sd a2,16(sp)\n"
>>>>>> +" sd t0,24(sp)\n"
>>>>>> +" sd ra,32(sp)\n"
>>>>>> +" call my_direct_func\n"
>>>>>> +" ld a0,0(sp)\n"
>>>>>> +" ld a1,8(sp)\n"
>>>>>> +" ld a2,16(sp)\n"
>>>>>> +" ld t0,24(sp)\n"
>>>>>> +" ld ra,32(sp)\n"
>>>>>> +" addi sp,sp,40\n"
>>>>>> +" jr t0\n"
>>>>>> +" .size my_tramp, .-my_tramp\n"
>>>>>> +" .popsection\n"
>>>>>> +);
>>>>>> +
>>>>>> +#endif /* CONFIG_RISCV */
>>>>>> +
>>>>>> #ifdef CONFIG_X86_64
>>>>>>
>>>>>> #include <asm/ibt.h>
>>>>>> diff --git a/samples/ftrace/ftrace-direct.c
>>>>>> b/samples/ftrace/ftrace-direct.c
>>>>>> index 1f769d0db20f..2cfe5a7d2d70 100644
>>>>>> --- a/samples/ftrace/ftrace-direct.c
>>>>>> +++ b/samples/ftrace/ftrace-direct.c
>>>>>> @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
>>>>>>
>>>>>> extern void my_tramp(void *);
>>>>>>
>>>>>> +#ifdef CONFIG_RISCV
>>>>>> +
>>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>>>>>> +" .type my_tramp, @function\n"
>>>>>> +" .globl my_tramp\n"
>>>>>> +" my_tramp:\n"
>>>>>> +" addi sp,sp,-24\n"
>>>>>> +" sd a0,0(sp)\n"
>>>>>> +" sd t0,8(sp)\n"
>>>>>> +" sd ra,16(sp)\n"
>>>>>> +" call my_direct_func\n"
>>>>>> +" ld a0,0(sp)\n"
>>>>>> +" ld t0,8(sp)\n"
>>>>>> +" ld ra,16(sp)\n"
>>>>>> +" addi sp,sp,24\n"
>>>>>> +" jr t0\n"
>>>>>> +" .size my_tramp, .-my_tramp\n"
>>>>>> +" .popsection\n"
>>>>>> +);
>>>>>> +
>>>>>> +#endif /* CONFIG_RISCV */
>>>>>> +
>>>>>> #ifdef CONFIG_X86_64
>>>>>>
>>>>>> #include <asm/ibt.h>
>>>>>> --
>>>>>> 2.36.1
>>>>
>>>> --
>>>> Thanks,
>>>> Song
>>>>
>>>
>>> Regards,
>>> Evgenii
>>
>>
>
>
> --
> Thanks,
> Song
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-01-18 15:19 ` Evgenii Shatokhin
@ 2023-01-19 6:05 ` Guo Ren
2023-02-18 21:30 ` Palmer Dabbelt
0 siblings, 1 reply; 44+ messages in thread
From: Guo Ren @ 2023-01-19 6:05 UTC (permalink / raw)
To: Evgenii Shatokhin
Cc: Song Shuai, linux-riscv, linux-kernel, anup, paul.walmsley,
palmer, conor.dooley, heiko, rostedt, mhiramat, jolsa, bp,
jpoimboe, andy.chiu, linux
Thx Evgenii & Song,
I got it; it would be put into v8.
On Wed, Jan 18, 2023 at 11:19 PM Evgenii Shatokhin
<e.shatokhin@yadro.com> wrote:
>
> On 18.01.2023 05:37, Song Shuai wrote:
> > Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月17日周二 16:22写道：
> >>
> >> On 17.01.2023 16:16, Evgenii Shatokhin wrote:
> >>> Hi, Song,
> >>>
> >>> On 17.01.2023 12:32, Song Shuai wrote:
> >>>>
> >>>> Hi, Evgenii:
> >>>>
> >>>> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
> >>>>
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On 12.01.2023 12:06, guoren@kernel.org wrote:
> >>>>>> From: Song Shuai <suagrfillet@gmail.com>
> >>>>>>
> >>>>>> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> >>>>>> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
> >>>>>> the ftrace-direct*.c files in samples/ftrace/.
> >>>>>>
> >>>>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> >>>>>> Tested-by: Guo Ren <guoren@kernel.org>
> >>>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
> >>>>>> ---
> >>>>>> arch/riscv/Kconfig | 2 ++
> >>>>>> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
> >>>>>> samples/ftrace/ftrace-direct-multi-modify.c | 37
> >>>>>> +++++++++++++++++++++
> >>>>>> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
> >>>>>> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
> >>>>>> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
> >>>>>> 6 files changed, 142 insertions(+)
> >>>>>
> >>>>> The samples were built OK now, but ftrace-direct-multi and
> >>>>> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
> >>>>>
> >>>>> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
> >>>>> the messages in the trace:
> >>>>>
> >>>>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> >>>>> # | | | ||||| | |
> >>>>> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
> >>>>> direct func1 ip 0
> >>>>> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
> >>>>> direct func1 ip ff60000001ba9600
> >>>>> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
> >>>>> direct func1 ip ff60000003334d00
> >>>>> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
> >>>>> direct func1 ip 0
> >>>>> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
> >>>>> direct func1 ip 0
> >>>>> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
> >>>>> direct func1 ip 7fffffffffffffff
> >>>>> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
> >>>>> direct func1 ip ff60000001ba9600
> >>>>> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
> >>>>> direct func1 ip ff60000001ba9600
> >>>>> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
> >>>>> direct func1 ip 0
> >>>>> [...]
> >>>>>
> >>>>> If I understand it right, my_direct_func1() should print the address of
> >>>>> some location in the code, probably - at the beginning of the traced
> >>>>> functions.
> >>>>>
> >>>>> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
> >>>>> addresses.
> >>>>>
> >>>> The invalid code address is only printed by accessing the schedule()
> >>>> function's first argument whose address stores in a0 register.
> >>>> While schedule() actually has no parameter declared, so my_direct_func
> >>>> just prints the a0 in the context of the schedule()'s caller and
> >>>> the address maybe varies depending on the caller.
> >>>>
> >>>> I can't really understand why tracing the first argument of the
> >>>> schedule() function, but it seems nonsense at this point.
> >>>
> >>> The question is, what should be passed as the argument(s) of
> >>> my_direct_func() in this particular sample module. The kernel docs and
> >>> commit logs seem to contain no info on that.
> >>>
> >>> With direct functions, I suppose, the trampoline can pass anything it
> >>> wants to my_direct_func(), not just the arguments of the traced function.
> >>>
> >>> I'd check what these sample modules do on x86 and would try to match
> >>> that behaviour on RISC-V.
> >>
> >> I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko
> >> on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should
> >> be passed to my_direct_func().
> >>
> >> ftrace-direct-multi.ko:
> >> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> >> # | | | ||||| | |
> >> insmod-10829 [000] d.h1. 1719.518535: my_direct_func: ip
> >> ffffffff87332f45 // wake_up_process+0x5
> >> rcu_tasks_kthre-11 [000] ..... 1719.518696: my_direct_func: ip
> >> ffffffff8828d935 // schedule+0x5
> >> insmod-10829 [000] ..... 1719.518708: my_direct_func: ip
> >> ffffffff8828d935
> >> systemd-journal-293 [001] ..... 1719.518823: my_direct_func: ip
> >> ffffffff8828d935
> >> systemd-1 [000] ..... 1719.519141: my_direct_func: ip
> >> ffffffff8828d935
> >> <idle>-0 [001] ..s1. 1719.521889: my_direct_func: ip
> >> ffffffff87332f45
> >> <idle>-0 [000] d.s2. 1719.521901: my_direct_func: ip
> >> ffffffff87332f45
> >> rcu_preempt-15 [001] ..... 1719.521917: my_direct_func: ip
> >> ffffffff8828d935
> >> [...]
> >>
> >> The ip values are wake_up_process+0x5 and schedule+0x5, the locations
> >> where the execution of the traced functions resumes after the Ftrace
> >> trampoline has finished.
> >>
> >> The results with ftrace-direct-multi-modify.ko are similar to that.
> >>
> >> The samples look like a demonstration, that one can pass anything
> >> necessary to the handler in case of "direct" functions.
> >>
> >> I suppose, the RISC-V-specific asm code in these two sample modules
> >> could be updated to pass the saved pc value to my_direct_func() in a0.
> >
> > Yes, you're right.
> >
> > I added 'mv a0,t0' in front of `call my_direct_func` to pass the address of
> > traced function with mcount offset.
> >
> > Here is the updated patch for your reference.
> > https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318
>
> Thank you for the quick fix. This one looks good to me.
>
> ftrace-direct-multi*.ko now report the ip values corresponding to
> schedule+0x8 and wake_up_process+0x8, which is what was expected here.
>
> One more thing: please change my "Co-developed-by:" into "Tested-by:" in
> your patch, becase this is what I actually did: tested it and reported
> the results. I cannot take your credit for development of this patch ;-)
>
> Looking forward for v8 of the series.
> >
> >
> >>
> >>>
> >>>>
> >>>> As for this patch, it just impls a simple mcount (direct_caller) to
> >>>> trace kernel functions, and basically saves the necessary ABI,
> >>>> call the tracing function, and restores the ABI, just like other
> >>>> arches do.
> >>>> so It shouldn't be blamed.
> >>>>
> >>>> I started an independent patch to replace schedule with kick_process
> >>>> to make these samples more reasonable. And It has no conflict with the
> >>>> current patch, so we can go on.
> >>>>
> >>>> Link:
> >>>> https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
> >>>>
> >>>>> The same issue is with ftrace-direct-multi-modify.ko.
> >>>>>
> >>>>> Is anything missing here?
> >>>>>
> >>>>>>
> >>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> >>>>>> index 307a9f413edd..e944af44f681 100644
> >>>>>> --- a/arch/riscv/Kconfig
> >>>>>> +++ b/arch/riscv/Kconfig
> >>>>>> @@ -112,6 +112,8 @@ config RISCV
> >>>>>> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
> >>>>>> select HAVE_REGS_AND_STACK_ACCESS_API
> >>>>>> select HAVE_FUNCTION_ARG_ACCESS_API
> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT
> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> >>>>>> select HAVE_STACKPROTECTOR
> >>>>>> select HAVE_SYSCALL_TRACEPOINTS
> >>>>>> select HAVE_RSEQ
> >>>>>> diff --git a/samples/ftrace/ftrace-direct-modify.c
> >>>>>> b/samples/ftrace/ftrace-direct-modify.c
> >>>>>> index de5a0f67f320..be7bf472c3c7 100644
> >>>>>> --- a/samples/ftrace/ftrace-direct-modify.c
> >>>>>> +++ b/samples/ftrace/ftrace-direct-modify.c
> >>>>>> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
> >>>>>>
> >>>>>> static unsigned long my_ip = (unsigned long)schedule;
> >>>>>>
> >>>>>> +#ifdef CONFIG_RISCV
> >>>>>> +
> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>>>> +" .type my_tramp1, @function\n"
> >>>>>> +" .globl my_tramp1\n"
> >>>>>> +" my_tramp1:\n"
> >>>>>> +" addi sp,sp,-16\n"
> >>>>>> +" sd t0,0(sp)\n"
> >>>>>> +" sd ra,8(sp)\n"
> >>>>>> +" call my_direct_func1\n"
> >>>>>> +" ld t0,0(sp)\n"
> >>>>>> +" ld ra,8(sp)\n"
> >>>>>> +" addi sp,sp,16\n"
> >>>>>> +" jr t0\n"
> >>>>>> +" .size my_tramp1, .-my_tramp1\n"
> >>>>>> +
> >>>>>> +" .type my_tramp2, @function\n"
> >>>>>> +" .globl my_tramp2\n"
> >>>>>> +" my_tramp2:\n"
> >>>>>> +" addi sp,sp,-16\n"
> >>>>>> +" sd t0,0(sp)\n"
> >>>>>> +" sd ra,8(sp)\n"
> >>>>>> +" call my_direct_func2\n"
> >>>>>> +" ld t0,0(sp)\n"
> >>>>>> +" ld ra,8(sp)\n"
> >>>>>> +" addi sp,sp,16\n"
> >>>>>> +" jr t0\n"
> >>>>>> +" .size my_tramp2, .-my_tramp2\n"
> >>>>>> +" .popsection\n"
> >>>>>> +);
> >>>>>> +
> >>>>>> +#endif /* CONFIG_RISCV */
> >>>>>> +
> >>>>>> #ifdef CONFIG_X86_64
> >>>>>>
> >>>>>> #include <asm/ibt.h>
> >>>>>> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c
> >>>>>> b/samples/ftrace/ftrace-direct-multi-modify.c
> >>>>>> index d52370cad0b6..10884bf418f7 100644
> >>>>>> --- a/samples/ftrace/ftrace-direct-multi-modify.c
> >>>>>> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> >>>>>> @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
> >>>>>> extern void my_tramp1(void *);
> >>>>>> extern void my_tramp2(void *);
> >>>>>>
> >>>>>> +#ifdef CONFIG_RISCV
> >>>>>> +
> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>>>> +" .type my_tramp1, @function\n"
> >>>>>> +" .globl my_tramp1\n"
> >>>>>> +" my_tramp1:\n"
> >>>>>> +" addi sp,sp,-24\n"
> >>>>>> +" sd a0,0(sp)\n"
> >>>>>> +" sd t0,8(sp)\n"
> >>>>>> +" sd ra,16(sp)\n"
> >>>>>> +" call my_direct_func1\n"
> >>>>>> +" ld a0,0(sp)\n"
> >>>>>> +" ld t0,8(sp)\n"
> >>>>>> +" ld ra,16(sp)\n"
> >>>>>> +" addi sp,sp,24\n"
> >>>>>> +" jr t0\n"
> >>>>>> +" .size my_tramp1, .-my_tramp1\n"
> >>>>>> +
> >>>>>> +" .type my_tramp2, @function\n"
> >>>>>> +" .globl my_tramp2\n"
> >>>>>> +" my_tramp2:\n"
> >>>>>> +" addi sp,sp,-24\n"
> >>>>>> +" sd a0,0(sp)\n"
> >>>>>> +" sd t0,8(sp)\n"
> >>>>>> +" sd ra,16(sp)\n"
> >>>>>> +" call my_direct_func2\n"
> >>>>>> +" ld a0,0(sp)\n"
> >>>>>> +" ld t0,8(sp)\n"
> >>>>>> +" ld ra,16(sp)\n"
> >>>>>> +" addi sp,sp,24\n"
> >>>>>> +" jr t0\n"
> >>>>>> +" .size my_tramp2, .-my_tramp2\n"
> >>>>>> +" .popsection\n"
> >>>>>> +);
> >>>>>> +
> >>>>>> +#endif /* CONFIG_RISCV */
> >>>>>> +
> >>>>>> #ifdef CONFIG_X86_64
> >>>>>>
> >>>>>> #include <asm/ibt.h>
> >>>>>> diff --git a/samples/ftrace/ftrace-direct-multi.c
> >>>>>> b/samples/ftrace/ftrace-direct-multi.c
> >>>>>> index ec1088922517..a35bf43bf6d7 100644
> >>>>>> --- a/samples/ftrace/ftrace-direct-multi.c
> >>>>>> +++ b/samples/ftrace/ftrace-direct-multi.c
> >>>>>> @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
> >>>>>>
> >>>>>> extern void my_tramp(void *);
> >>>>>>
> >>>>>> +#ifdef CONFIG_RISCV
> >>>>>> +
> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>>>> +" .type my_tramp, @function\n"
> >>>>>> +" .globl my_tramp\n"
> >>>>>> +" my_tramp:\n"
> >>>>>> +" addi sp,sp,-24\n"
> >>>>>> +" sd a0,0(sp)\n"
> >>>>>> +" sd t0,8(sp)\n"
> >>>>>> +" sd ra,16(sp)\n"
> >>>>>> +" call my_direct_func\n"
> >>>>>> +" ld a0,0(sp)\n"
> >>>>>> +" ld t0,8(sp)\n"
> >>>>>> +" ld ra,16(sp)\n"
> >>>>>> +" addi sp,sp,24\n"
> >>>>>> +" jr t0\n"
> >>>>>> +" .size my_tramp, .-my_tramp\n"
> >>>>>> +" .popsection\n"
> >>>>>> +);
> >>>>>> +
> >>>>>> +#endif /* CONFIG_RISCV */
> >>>>>> +
> >>>>>> #ifdef CONFIG_X86_64
> >>>>>>
> >>>>>> #include <asm/ibt.h>
> >>>>>> diff --git a/samples/ftrace/ftrace-direct-too.c
> >>>>>> b/samples/ftrace/ftrace-direct-too.c
> >>>>>> index e13fb59a2b47..3b62e33c2e6d 100644
> >>>>>> --- a/samples/ftrace/ftrace-direct-too.c
> >>>>>> +++ b/samples/ftrace/ftrace-direct-too.c
> >>>>>> @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
> >>>>>>
> >>>>>> extern void my_tramp(void *);
> >>>>>>
> >>>>>> +#ifdef CONFIG_RISCV
> >>>>>> +
> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>>>> +" .type my_tramp, @function\n"
> >>>>>> +" .globl my_tramp\n"
> >>>>>> +" my_tramp:\n"
> >>>>>> +" addi sp,sp,-40\n"
> >>>>>> +" sd a0,0(sp)\n"
> >>>>>> +" sd a1,8(sp)\n"
> >>>>>> +" sd a2,16(sp)\n"
> >>>>>> +" sd t0,24(sp)\n"
> >>>>>> +" sd ra,32(sp)\n"
> >>>>>> +" call my_direct_func\n"
> >>>>>> +" ld a0,0(sp)\n"
> >>>>>> +" ld a1,8(sp)\n"
> >>>>>> +" ld a2,16(sp)\n"
> >>>>>> +" ld t0,24(sp)\n"
> >>>>>> +" ld ra,32(sp)\n"
> >>>>>> +" addi sp,sp,40\n"
> >>>>>> +" jr t0\n"
> >>>>>> +" .size my_tramp, .-my_tramp\n"
> >>>>>> +" .popsection\n"
> >>>>>> +);
> >>>>>> +
> >>>>>> +#endif /* CONFIG_RISCV */
> >>>>>> +
> >>>>>> #ifdef CONFIG_X86_64
> >>>>>>
> >>>>>> #include <asm/ibt.h>
> >>>>>> diff --git a/samples/ftrace/ftrace-direct.c
> >>>>>> b/samples/ftrace/ftrace-direct.c
> >>>>>> index 1f769d0db20f..2cfe5a7d2d70 100644
> >>>>>> --- a/samples/ftrace/ftrace-direct.c
> >>>>>> +++ b/samples/ftrace/ftrace-direct.c
> >>>>>> @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
> >>>>>>
> >>>>>> extern void my_tramp(void *);
> >>>>>>
> >>>>>> +#ifdef CONFIG_RISCV
> >>>>>> +
> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >>>>>> +" .type my_tramp, @function\n"
> >>>>>> +" .globl my_tramp\n"
> >>>>>> +" my_tramp:\n"
> >>>>>> +" addi sp,sp,-24\n"
> >>>>>> +" sd a0,0(sp)\n"
> >>>>>> +" sd t0,8(sp)\n"
> >>>>>> +" sd ra,16(sp)\n"
> >>>>>> +" call my_direct_func\n"
> >>>>>> +" ld a0,0(sp)\n"
> >>>>>> +" ld t0,8(sp)\n"
> >>>>>> +" ld ra,16(sp)\n"
> >>>>>> +" addi sp,sp,24\n"
> >>>>>> +" jr t0\n"
> >>>>>> +" .size my_tramp, .-my_tramp\n"
> >>>>>> +" .popsection\n"
> >>>>>> +);
> >>>>>> +
> >>>>>> +#endif /* CONFIG_RISCV */
> >>>>>> +
> >>>>>> #ifdef CONFIG_X86_64
> >>>>>>
> >>>>>> #include <asm/ibt.h>
> >>>>>> --
> >>>>>> 2.36.1
> >>>>
> >>>> --
> >>>> Thanks,
> >>>> Song
> >>>>
> >>>
> >>> Regards,
> >>> Evgenii
> >>
> >>
> >
> >
> > --
> > Thanks,
> > Song
> >
>
>
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-01-19 6:05 ` Guo Ren
@ 2023-02-18 21:30 ` Palmer Dabbelt
2023-02-20 2:46 ` Song Shuai
2023-02-21 4:02 ` Guo Ren
0 siblings, 2 replies; 44+ messages in thread
From: Palmer Dabbelt @ 2023-02-18 21:30 UTC (permalink / raw)
To: guoren
Cc: e.shatokhin, suagrfillet, linux-riscv, linux-kernel, anup,
Paul Walmsley, Conor Dooley, heiko, rostedt, mhiramat, jolsa, bp,
jpoimboe, andy.chiu, linux
On Wed, 18 Jan 2023 22:05:04 PST (-0800), guoren@kernel.org wrote:
> Thx Evgenii & Song,
>
> I got it; it would be put into v8.
Sorry if I missed it, but I don't see a v8 on patchwork. I queued up
the first three patches onto for-next as they seem like pretty
independent fixes/cleanups, I'll hold off on the others until v8.
Thanks!
>
> On Wed, Jan 18, 2023 at 11:19 PM Evgenii Shatokhin
> <e.shatokhin@yadro.com> wrote:
>>
>> On 18.01.2023 05:37, Song Shuai wrote:
>> > Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月17日周二 16:22写道：
>> >>
>> >> On 17.01.2023 16:16, Evgenii Shatokhin wrote:
>> >>> Hi, Song,
>> >>>
>> >>> On 17.01.2023 12:32, Song Shuai wrote:
>> >>>>
>> >>>> Hi, Evgenii:
>> >>>>
>> >>>> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
>> >>>>
>> >>>>>
>> >>>>> Hi,
>> >>>>>
>> >>>>> On 12.01.2023 12:06, guoren@kernel.org wrote:
>> >>>>>> From: Song Shuai <suagrfillet@gmail.com>
>> >>>>>>
>> >>>>>> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
>> >>>>>> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
>> >>>>>> the ftrace-direct*.c files in samples/ftrace/.
>> >>>>>>
>> >>>>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
>> >>>>>> Tested-by: Guo Ren <guoren@kernel.org>
>> >>>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
>> >>>>>> ---
>> >>>>>> arch/riscv/Kconfig | 2 ++
>> >>>>>> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
>> >>>>>> samples/ftrace/ftrace-direct-multi-modify.c | 37
>> >>>>>> +++++++++++++++++++++
>> >>>>>> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
>> >>>>>> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
>> >>>>>> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
>> >>>>>> 6 files changed, 142 insertions(+)
>> >>>>>
>> >>>>> The samples were built OK now, but ftrace-direct-multi and
>> >>>>> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
>> >>>>>
>> >>>>> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
>> >>>>> the messages in the trace:
>> >>>>>
>> >>>>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
>> >>>>> # | | | ||||| | |
>> >>>>> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
>> >>>>> direct func1 ip 0
>> >>>>> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
>> >>>>> direct func1 ip ff60000001ba9600
>> >>>>> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
>> >>>>> direct func1 ip ff60000003334d00
>> >>>>> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
>> >>>>> direct func1 ip 0
>> >>>>> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
>> >>>>> direct func1 ip 0
>> >>>>> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
>> >>>>> direct func1 ip 7fffffffffffffff
>> >>>>> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
>> >>>>> direct func1 ip ff60000001ba9600
>> >>>>> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
>> >>>>> direct func1 ip ff60000001ba9600
>> >>>>> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
>> >>>>> direct func1 ip 0
>> >>>>> [...]
>> >>>>>
>> >>>>> If I understand it right, my_direct_func1() should print the address of
>> >>>>> some location in the code, probably - at the beginning of the traced
>> >>>>> functions.
>> >>>>>
>> >>>>> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
>> >>>>> addresses.
>> >>>>>
>> >>>> The invalid code address is only printed by accessing the schedule()
>> >>>> function's first argument whose address stores in a0 register.
>> >>>> While schedule() actually has no parameter declared, so my_direct_func
>> >>>> just prints the a0 in the context of the schedule()'s caller and
>> >>>> the address maybe varies depending on the caller.
>> >>>>
>> >>>> I can't really understand why tracing the first argument of the
>> >>>> schedule() function, but it seems nonsense at this point.
>> >>>
>> >>> The question is, what should be passed as the argument(s) of
>> >>> my_direct_func() in this particular sample module. The kernel docs and
>> >>> commit logs seem to contain no info on that.
>> >>>
>> >>> With direct functions, I suppose, the trampoline can pass anything it
>> >>> wants to my_direct_func(), not just the arguments of the traced function.
>> >>>
>> >>> I'd check what these sample modules do on x86 and would try to match
>> >>> that behaviour on RISC-V.
>> >>
>> >> I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko
>> >> on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should
>> >> be passed to my_direct_func().
>> >>
>> >> ftrace-direct-multi.ko:
>> >> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
>> >> # | | | ||||| | |
>> >> insmod-10829 [000] d.h1. 1719.518535: my_direct_func: ip
>> >> ffffffff87332f45 // wake_up_process+0x5
>> >> rcu_tasks_kthre-11 [000] ..... 1719.518696: my_direct_func: ip
>> >> ffffffff8828d935 // schedule+0x5
>> >> insmod-10829 [000] ..... 1719.518708: my_direct_func: ip
>> >> ffffffff8828d935
>> >> systemd-journal-293 [001] ..... 1719.518823: my_direct_func: ip
>> >> ffffffff8828d935
>> >> systemd-1 [000] ..... 1719.519141: my_direct_func: ip
>> >> ffffffff8828d935
>> >> <idle>-0 [001] ..s1. 1719.521889: my_direct_func: ip
>> >> ffffffff87332f45
>> >> <idle>-0 [000] d.s2. 1719.521901: my_direct_func: ip
>> >> ffffffff87332f45
>> >> rcu_preempt-15 [001] ..... 1719.521917: my_direct_func: ip
>> >> ffffffff8828d935
>> >> [...]
>> >>
>> >> The ip values are wake_up_process+0x5 and schedule+0x5, the locations
>> >> where the execution of the traced functions resumes after the Ftrace
>> >> trampoline has finished.
>> >>
>> >> The results with ftrace-direct-multi-modify.ko are similar to that.
>> >>
>> >> The samples look like a demonstration, that one can pass anything
>> >> necessary to the handler in case of "direct" functions.
>> >>
>> >> I suppose, the RISC-V-specific asm code in these two sample modules
>> >> could be updated to pass the saved pc value to my_direct_func() in a0.
>> >
>> > Yes, you're right.
>> >
>> > I added 'mv a0,t0' in front of `call my_direct_func` to pass the address of
>> > traced function with mcount offset.
>> >
>> > Here is the updated patch for your reference.
>> > https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318
>>
>> Thank you for the quick fix. This one looks good to me.
>>
>> ftrace-direct-multi*.ko now report the ip values corresponding to
>> schedule+0x8 and wake_up_process+0x8, which is what was expected here.
>>
>> One more thing: please change my "Co-developed-by:" into "Tested-by:" in
>> your patch, becase this is what I actually did: tested it and reported
>> the results. I cannot take your credit for development of this patch ;-)
>>
>> Looking forward for v8 of the series.
>> >
>> >
>> >>
>> >>>
>> >>>>
>> >>>> As for this patch, it just impls a simple mcount (direct_caller) to
>> >>>> trace kernel functions, and basically saves the necessary ABI,
>> >>>> call the tracing function, and restores the ABI, just like other
>> >>>> arches do.
>> >>>> so It shouldn't be blamed.
>> >>>>
>> >>>> I started an independent patch to replace schedule with kick_process
>> >>>> to make these samples more reasonable. And It has no conflict with the
>> >>>> current patch, so we can go on.
>> >>>>
>> >>>> Link:
>> >>>> https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
>> >>>>
>> >>>>> The same issue is with ftrace-direct-multi-modify.ko.
>> >>>>>
>> >>>>> Is anything missing here?
>> >>>>>
>> >>>>>>
>> >>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> >>>>>> index 307a9f413edd..e944af44f681 100644
>> >>>>>> --- a/arch/riscv/Kconfig
>> >>>>>> +++ b/arch/riscv/Kconfig
>> >>>>>> @@ -112,6 +112,8 @@ config RISCV
>> >>>>>> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
>> >>>>>> select HAVE_REGS_AND_STACK_ACCESS_API
>> >>>>>> select HAVE_FUNCTION_ARG_ACCESS_API
>> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT
>> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
>> >>>>>> select HAVE_STACKPROTECTOR
>> >>>>>> select HAVE_SYSCALL_TRACEPOINTS
>> >>>>>> select HAVE_RSEQ
>> >>>>>> diff --git a/samples/ftrace/ftrace-direct-modify.c
>> >>>>>> b/samples/ftrace/ftrace-direct-modify.c
>> >>>>>> index de5a0f67f320..be7bf472c3c7 100644
>> >>>>>> --- a/samples/ftrace/ftrace-direct-modify.c
>> >>>>>> +++ b/samples/ftrace/ftrace-direct-modify.c
>> >>>>>> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
>> >>>>>>
>> >>>>>> static unsigned long my_ip = (unsigned long)schedule;
>> >>>>>>
>> >>>>>> +#ifdef CONFIG_RISCV
>> >>>>>> +
>> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>> >>>>>> +" .type my_tramp1, @function\n"
>> >>>>>> +" .globl my_tramp1\n"
>> >>>>>> +" my_tramp1:\n"
>> >>>>>> +" addi sp,sp,-16\n"
>> >>>>>> +" sd t0,0(sp)\n"
>> >>>>>> +" sd ra,8(sp)\n"
>> >>>>>> +" call my_direct_func1\n"
>> >>>>>> +" ld t0,0(sp)\n"
>> >>>>>> +" ld ra,8(sp)\n"
>> >>>>>> +" addi sp,sp,16\n"
>> >>>>>> +" jr t0\n"
>> >>>>>> +" .size my_tramp1, .-my_tramp1\n"
>> >>>>>> +
>> >>>>>> +" .type my_tramp2, @function\n"
>> >>>>>> +" .globl my_tramp2\n"
>> >>>>>> +" my_tramp2:\n"
>> >>>>>> +" addi sp,sp,-16\n"
>> >>>>>> +" sd t0,0(sp)\n"
>> >>>>>> +" sd ra,8(sp)\n"
>> >>>>>> +" call my_direct_func2\n"
>> >>>>>> +" ld t0,0(sp)\n"
>> >>>>>> +" ld ra,8(sp)\n"
>> >>>>>> +" addi sp,sp,16\n"
>> >>>>>> +" jr t0\n"
>> >>>>>> +" .size my_tramp2, .-my_tramp2\n"
>> >>>>>> +" .popsection\n"
>> >>>>>> +);
>> >>>>>> +
>> >>>>>> +#endif /* CONFIG_RISCV */
>> >>>>>> +
>> >>>>>> #ifdef CONFIG_X86_64
>> >>>>>>
>> >>>>>> #include <asm/ibt.h>
>> >>>>>> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c
>> >>>>>> b/samples/ftrace/ftrace-direct-multi-modify.c
>> >>>>>> index d52370cad0b6..10884bf418f7 100644
>> >>>>>> --- a/samples/ftrace/ftrace-direct-multi-modify.c
>> >>>>>> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
>> >>>>>> @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
>> >>>>>> extern void my_tramp1(void *);
>> >>>>>> extern void my_tramp2(void *);
>> >>>>>>
>> >>>>>> +#ifdef CONFIG_RISCV
>> >>>>>> +
>> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>> >>>>>> +" .type my_tramp1, @function\n"
>> >>>>>> +" .globl my_tramp1\n"
>> >>>>>> +" my_tramp1:\n"
>> >>>>>> +" addi sp,sp,-24\n"
>> >>>>>> +" sd a0,0(sp)\n"
>> >>>>>> +" sd t0,8(sp)\n"
>> >>>>>> +" sd ra,16(sp)\n"
>> >>>>>> +" call my_direct_func1\n"
>> >>>>>> +" ld a0,0(sp)\n"
>> >>>>>> +" ld t0,8(sp)\n"
>> >>>>>> +" ld ra,16(sp)\n"
>> >>>>>> +" addi sp,sp,24\n"
>> >>>>>> +" jr t0\n"
>> >>>>>> +" .size my_tramp1, .-my_tramp1\n"
>> >>>>>> +
>> >>>>>> +" .type my_tramp2, @function\n"
>> >>>>>> +" .globl my_tramp2\n"
>> >>>>>> +" my_tramp2:\n"
>> >>>>>> +" addi sp,sp,-24\n"
>> >>>>>> +" sd a0,0(sp)\n"
>> >>>>>> +" sd t0,8(sp)\n"
>> >>>>>> +" sd ra,16(sp)\n"
>> >>>>>> +" call my_direct_func2\n"
>> >>>>>> +" ld a0,0(sp)\n"
>> >>>>>> +" ld t0,8(sp)\n"
>> >>>>>> +" ld ra,16(sp)\n"
>> >>>>>> +" addi sp,sp,24\n"
>> >>>>>> +" jr t0\n"
>> >>>>>> +" .size my_tramp2, .-my_tramp2\n"
>> >>>>>> +" .popsection\n"
>> >>>>>> +);
>> >>>>>> +
>> >>>>>> +#endif /* CONFIG_RISCV */
>> >>>>>> +
>> >>>>>> #ifdef CONFIG_X86_64
>> >>>>>>
>> >>>>>> #include <asm/ibt.h>
>> >>>>>> diff --git a/samples/ftrace/ftrace-direct-multi.c
>> >>>>>> b/samples/ftrace/ftrace-direct-multi.c
>> >>>>>> index ec1088922517..a35bf43bf6d7 100644
>> >>>>>> --- a/samples/ftrace/ftrace-direct-multi.c
>> >>>>>> +++ b/samples/ftrace/ftrace-direct-multi.c
>> >>>>>> @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
>> >>>>>>
>> >>>>>> extern void my_tramp(void *);
>> >>>>>>
>> >>>>>> +#ifdef CONFIG_RISCV
>> >>>>>> +
>> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>> >>>>>> +" .type my_tramp, @function\n"
>> >>>>>> +" .globl my_tramp\n"
>> >>>>>> +" my_tramp:\n"
>> >>>>>> +" addi sp,sp,-24\n"
>> >>>>>> +" sd a0,0(sp)\n"
>> >>>>>> +" sd t0,8(sp)\n"
>> >>>>>> +" sd ra,16(sp)\n"
>> >>>>>> +" call my_direct_func\n"
>> >>>>>> +" ld a0,0(sp)\n"
>> >>>>>> +" ld t0,8(sp)\n"
>> >>>>>> +" ld ra,16(sp)\n"
>> >>>>>> +" addi sp,sp,24\n"
>> >>>>>> +" jr t0\n"
>> >>>>>> +" .size my_tramp, .-my_tramp\n"
>> >>>>>> +" .popsection\n"
>> >>>>>> +);
>> >>>>>> +
>> >>>>>> +#endif /* CONFIG_RISCV */
>> >>>>>> +
>> >>>>>> #ifdef CONFIG_X86_64
>> >>>>>>
>> >>>>>> #include <asm/ibt.h>
>> >>>>>> diff --git a/samples/ftrace/ftrace-direct-too.c
>> >>>>>> b/samples/ftrace/ftrace-direct-too.c
>> >>>>>> index e13fb59a2b47..3b62e33c2e6d 100644
>> >>>>>> --- a/samples/ftrace/ftrace-direct-too.c
>> >>>>>> +++ b/samples/ftrace/ftrace-direct-too.c
>> >>>>>> @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
>> >>>>>>
>> >>>>>> extern void my_tramp(void *);
>> >>>>>>
>> >>>>>> +#ifdef CONFIG_RISCV
>> >>>>>> +
>> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>> >>>>>> +" .type my_tramp, @function\n"
>> >>>>>> +" .globl my_tramp\n"
>> >>>>>> +" my_tramp:\n"
>> >>>>>> +" addi sp,sp,-40\n"
>> >>>>>> +" sd a0,0(sp)\n"
>> >>>>>> +" sd a1,8(sp)\n"
>> >>>>>> +" sd a2,16(sp)\n"
>> >>>>>> +" sd t0,24(sp)\n"
>> >>>>>> +" sd ra,32(sp)\n"
>> >>>>>> +" call my_direct_func\n"
>> >>>>>> +" ld a0,0(sp)\n"
>> >>>>>> +" ld a1,8(sp)\n"
>> >>>>>> +" ld a2,16(sp)\n"
>> >>>>>> +" ld t0,24(sp)\n"
>> >>>>>> +" ld ra,32(sp)\n"
>> >>>>>> +" addi sp,sp,40\n"
>> >>>>>> +" jr t0\n"
>> >>>>>> +" .size my_tramp, .-my_tramp\n"
>> >>>>>> +" .popsection\n"
>> >>>>>> +);
>> >>>>>> +
>> >>>>>> +#endif /* CONFIG_RISCV */
>> >>>>>> +
>> >>>>>> #ifdef CONFIG_X86_64
>> >>>>>>
>> >>>>>> #include <asm/ibt.h>
>> >>>>>> diff --git a/samples/ftrace/ftrace-direct.c
>> >>>>>> b/samples/ftrace/ftrace-direct.c
>> >>>>>> index 1f769d0db20f..2cfe5a7d2d70 100644
>> >>>>>> --- a/samples/ftrace/ftrace-direct.c
>> >>>>>> +++ b/samples/ftrace/ftrace-direct.c
>> >>>>>> @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
>> >>>>>>
>> >>>>>> extern void my_tramp(void *);
>> >>>>>>
>> >>>>>> +#ifdef CONFIG_RISCV
>> >>>>>> +
>> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
>> >>>>>> +" .type my_tramp, @function\n"
>> >>>>>> +" .globl my_tramp\n"
>> >>>>>> +" my_tramp:\n"
>> >>>>>> +" addi sp,sp,-24\n"
>> >>>>>> +" sd a0,0(sp)\n"
>> >>>>>> +" sd t0,8(sp)\n"
>> >>>>>> +" sd ra,16(sp)\n"
>> >>>>>> +" call my_direct_func\n"
>> >>>>>> +" ld a0,0(sp)\n"
>> >>>>>> +" ld t0,8(sp)\n"
>> >>>>>> +" ld ra,16(sp)\n"
>> >>>>>> +" addi sp,sp,24\n"
>> >>>>>> +" jr t0\n"
>> >>>>>> +" .size my_tramp, .-my_tramp\n"
>> >>>>>> +" .popsection\n"
>> >>>>>> +);
>> >>>>>> +
>> >>>>>> +#endif /* CONFIG_RISCV */
>> >>>>>> +
>> >>>>>> #ifdef CONFIG_X86_64
>> >>>>>>
>> >>>>>> #include <asm/ibt.h>
>> >>>>>> --
>> >>>>>> 2.36.1
>> >>>>
>> >>>> --
>> >>>> Thanks,
>> >>>> Song
>> >>>>
>> >>>
>> >>> Regards,
>> >>> Evgenii
>> >>
>> >>
>> >
>> >
>> > --
>> > Thanks,
>> > Song
>> >
>>
>>
>
>
> --
> Best Regards
> Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-02-18 21:30 ` Palmer Dabbelt
@ 2023-02-20 2:46 ` Song Shuai
2023-02-21 3:56 ` Guo Ren
2023-02-21 4:02 ` Guo Ren
1 sibling, 1 reply; 44+ messages in thread
From: Song Shuai @ 2023-02-20 2:46 UTC (permalink / raw)
To: guoren
Cc: Palmer Dabbelt, e.shatokhin, linux-riscv, linux-kernel, anup,
Paul Walmsley, Conor Dooley, heiko, rostedt, mhiramat, jolsa, bp,
jpoimboe, andy.chiu, linux
Hi,Guo:
Palmer Dabbelt <palmer@dabbelt.com> 于2023年2月18日周六 21:30写道：
>
> On Wed, 18 Jan 2023 22:05:04 PST (-0800), guoren@kernel.org wrote:
> > Thx Evgenii & Song,
> >
> > I got it; it would be put into v8.
>
There were 2 problems for this patch in its V6 and V7 version:
1. build error resulted by including of nospec-branch.h file in V6
- this common fixup patch had been sent to the linux-trace mail
list and would show up in Linux v6.3
- here is the link:
https://lore.kernel.org/lkml/20230208015633.791198913@goodmis.org/
2. invalid code address reported by Evgenii in V7
- for your convenience, the link to this updated patch is
https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318
Is it time to launch the V8 with this updated patch, or waiting for
the common fixup patch to be merged?
> Sorry if I missed it, but I don't see a v8 on patchwork. I queued up
> the first three patches onto for-next as they seem like pretty
> independent fixes/cleanups, I'll hold off on the others until v8.
>
> Thanks!
>
> >
> > On Wed, Jan 18, 2023 at 11:19 PM Evgenii Shatokhin
> > <e.shatokhin@yadro.com> wrote:
> >>
> >> On 18.01.2023 05:37, Song Shuai wrote:
> >> > Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月17日周二 16:22写道：
> >> >>
> >> >> On 17.01.2023 16:16, Evgenii Shatokhin wrote:
> >> >>> Hi, Song,
> >> >>>
> >> >>> On 17.01.2023 12:32, Song Shuai wrote:
> >> >>>>
> >> >>>> Hi, Evgenii:
> >> >>>>
> >> >>>> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
> >> >>>>
> >> >>>>>
> >> >>>>> Hi,
> >> >>>>>
> >> >>>>> On 12.01.2023 12:06, guoren@kernel.org wrote:
> >> >>>>>> From: Song Shuai <suagrfillet@gmail.com>
> >> >>>>>>
> >> >>>>>> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> >> >>>>>> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
> >> >>>>>> the ftrace-direct*.c files in samples/ftrace/.
> >> >>>>>>
> >> >>>>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> >> >>>>>> Tested-by: Guo Ren <guoren@kernel.org>
> >> >>>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
> >> >>>>>> ---
> >> >>>>>> arch/riscv/Kconfig | 2 ++
> >> >>>>>> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
> >> >>>>>> samples/ftrace/ftrace-direct-multi-modify.c | 37
> >> >>>>>> +++++++++++++++++++++
> >> >>>>>> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
> >> >>>>>> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
> >> >>>>>> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
> >> >>>>>> 6 files changed, 142 insertions(+)
> >> >>>>>
> >> >>>>> The samples were built OK now, but ftrace-direct-multi and
> >> >>>>> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
> >> >>>>>
> >> >>>>> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
> >> >>>>> the messages in the trace:
> >> >>>>>
> >> >>>>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> >> >>>>> # | | | ||||| | |
> >> >>>>> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
> >> >>>>> direct func1 ip 0
> >> >>>>> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
> >> >>>>> direct func1 ip ff60000001ba9600
> >> >>>>> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
> >> >>>>> direct func1 ip ff60000003334d00
> >> >>>>> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
> >> >>>>> direct func1 ip 0
> >> >>>>> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
> >> >>>>> direct func1 ip 0
> >> >>>>> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
> >> >>>>> direct func1 ip 7fffffffffffffff
> >> >>>>> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
> >> >>>>> direct func1 ip ff60000001ba9600
> >> >>>>> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
> >> >>>>> direct func1 ip ff60000001ba9600
> >> >>>>> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
> >> >>>>> direct func1 ip 0
> >> >>>>> [...]
> >> >>>>>
> >> >>>>> If I understand it right, my_direct_func1() should print the address of
> >> >>>>> some location in the code, probably - at the beginning of the traced
> >> >>>>> functions.
> >> >>>>>
> >> >>>>> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
> >> >>>>> addresses.
> >> >>>>>
> >> >>>> The invalid code address is only printed by accessing the schedule()
> >> >>>> function's first argument whose address stores in a0 register.
> >> >>>> While schedule() actually has no parameter declared, so my_direct_func
> >> >>>> just prints the a0 in the context of the schedule()'s caller and
> >> >>>> the address maybe varies depending on the caller.
> >> >>>>
> >> >>>> I can't really understand why tracing the first argument of the
> >> >>>> schedule() function, but it seems nonsense at this point.
> >> >>>
> >> >>> The question is, what should be passed as the argument(s) of
> >> >>> my_direct_func() in this particular sample module. The kernel docs and
> >> >>> commit logs seem to contain no info on that.
> >> >>>
> >> >>> With direct functions, I suppose, the trampoline can pass anything it
> >> >>> wants to my_direct_func(), not just the arguments of the traced function.
> >> >>>
> >> >>> I'd check what these sample modules do on x86 and would try to match
> >> >>> that behaviour on RISC-V.
> >> >>
> >> >> I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko
> >> >> on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should
> >> >> be passed to my_direct_func().
> >> >>
> >> >> ftrace-direct-multi.ko:
> >> >> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> >> >> # | | | ||||| | |
> >> >> insmod-10829 [000] d.h1. 1719.518535: my_direct_func: ip
> >> >> ffffffff87332f45 // wake_up_process+0x5
> >> >> rcu_tasks_kthre-11 [000] ..... 1719.518696: my_direct_func: ip
> >> >> ffffffff8828d935 // schedule+0x5
> >> >> insmod-10829 [000] ..... 1719.518708: my_direct_func: ip
> >> >> ffffffff8828d935
> >> >> systemd-journal-293 [001] ..... 1719.518823: my_direct_func: ip
> >> >> ffffffff8828d935
> >> >> systemd-1 [000] ..... 1719.519141: my_direct_func: ip
> >> >> ffffffff8828d935
> >> >> <idle>-0 [001] ..s1. 1719.521889: my_direct_func: ip
> >> >> ffffffff87332f45
> >> >> <idle>-0 [000] d.s2. 1719.521901: my_direct_func: ip
> >> >> ffffffff87332f45
> >> >> rcu_preempt-15 [001] ..... 1719.521917: my_direct_func: ip
> >> >> ffffffff8828d935
> >> >> [...]
> >> >>
> >> >> The ip values are wake_up_process+0x5 and schedule+0x5, the locations
> >> >> where the execution of the traced functions resumes after the Ftrace
> >> >> trampoline has finished.
> >> >>
> >> >> The results with ftrace-direct-multi-modify.ko are similar to that.
> >> >>
> >> >> The samples look like a demonstration, that one can pass anything
> >> >> necessary to the handler in case of "direct" functions.
> >> >>
> >> >> I suppose, the RISC-V-specific asm code in these two sample modules
> >> >> could be updated to pass the saved pc value to my_direct_func() in a0.
> >> >
> >> > Yes, you're right.
> >> >
> >> > I added 'mv a0,t0' in front of `call my_direct_func` to pass the address of
> >> > traced function with mcount offset.
> >> >
> >> > Here is the updated patch for your reference.
> >> > https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318
> >>
> >> Thank you for the quick fix. This one looks good to me.
> >>
> >> ftrace-direct-multi*.ko now report the ip values corresponding to
> >> schedule+0x8 and wake_up_process+0x8, which is what was expected here.
> >>
> >> One more thing: please change my "Co-developed-by:" into "Tested-by:" in
> >> your patch, becase this is what I actually did: tested it and reported
> >> the results. I cannot take your credit for development of this patch ;-)
> >>
> >> Looking forward for v8 of the series.
> >> >
> >> >
> >> >>
> >> >>>
> >> >>>>
> >> >>>> As for this patch, it just impls a simple mcount (direct_caller) to
> >> >>>> trace kernel functions, and basically saves the necessary ABI,
> >> >>>> call the tracing function, and restores the ABI, just like other
> >> >>>> arches do.
> >> >>>> so It shouldn't be blamed.
> >> >>>>
> >> >>>> I started an independent patch to replace schedule with kick_process
> >> >>>> to make these samples more reasonable. And It has no conflict with the
> >> >>>> current patch, so we can go on.
> >> >>>>
> >> >>>> Link:
> >> >>>> https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
> >> >>>>
> >> >>>>> The same issue is with ftrace-direct-multi-modify.ko.
> >> >>>>>
> >> >>>>> Is anything missing here?
> >> >>>>>
> >> >>>>>>
> >> >>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> >> >>>>>> index 307a9f413edd..e944af44f681 100644
> >> >>>>>> --- a/arch/riscv/Kconfig
> >> >>>>>> +++ b/arch/riscv/Kconfig
> >> >>>>>> @@ -112,6 +112,8 @@ config RISCV
> >> >>>>>> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
> >> >>>>>> select HAVE_REGS_AND_STACK_ACCESS_API
> >> >>>>>> select HAVE_FUNCTION_ARG_ACCESS_API
> >> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT
> >> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> >> >>>>>> select HAVE_STACKPROTECTOR
> >> >>>>>> select HAVE_SYSCALL_TRACEPOINTS
> >> >>>>>> select HAVE_RSEQ
> >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-modify.c
> >> >>>>>> b/samples/ftrace/ftrace-direct-modify.c
> >> >>>>>> index de5a0f67f320..be7bf472c3c7 100644
> >> >>>>>> --- a/samples/ftrace/ftrace-direct-modify.c
> >> >>>>>> +++ b/samples/ftrace/ftrace-direct-modify.c
> >> >>>>>> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
> >> >>>>>>
> >> >>>>>> static unsigned long my_ip = (unsigned long)schedule;
> >> >>>>>>
> >> >>>>>> +#ifdef CONFIG_RISCV
> >> >>>>>> +
> >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >> >>>>>> +" .type my_tramp1, @function\n"
> >> >>>>>> +" .globl my_tramp1\n"
> >> >>>>>> +" my_tramp1:\n"
> >> >>>>>> +" addi sp,sp,-16\n"
> >> >>>>>> +" sd t0,0(sp)\n"
> >> >>>>>> +" sd ra,8(sp)\n"
> >> >>>>>> +" call my_direct_func1\n"
> >> >>>>>> +" ld t0,0(sp)\n"
> >> >>>>>> +" ld ra,8(sp)\n"
> >> >>>>>> +" addi sp,sp,16\n"
> >> >>>>>> +" jr t0\n"
> >> >>>>>> +" .size my_tramp1, .-my_tramp1\n"
> >> >>>>>> +
> >> >>>>>> +" .type my_tramp2, @function\n"
> >> >>>>>> +" .globl my_tramp2\n"
> >> >>>>>> +" my_tramp2:\n"
> >> >>>>>> +" addi sp,sp,-16\n"
> >> >>>>>> +" sd t0,0(sp)\n"
> >> >>>>>> +" sd ra,8(sp)\n"
> >> >>>>>> +" call my_direct_func2\n"
> >> >>>>>> +" ld t0,0(sp)\n"
> >> >>>>>> +" ld ra,8(sp)\n"
> >> >>>>>> +" addi sp,sp,16\n"
> >> >>>>>> +" jr t0\n"
> >> >>>>>> +" .size my_tramp2, .-my_tramp2\n"
> >> >>>>>> +" .popsection\n"
> >> >>>>>> +);
> >> >>>>>> +
> >> >>>>>> +#endif /* CONFIG_RISCV */
> >> >>>>>> +
> >> >>>>>> #ifdef CONFIG_X86_64
> >> >>>>>>
> >> >>>>>> #include <asm/ibt.h>
> >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c
> >> >>>>>> b/samples/ftrace/ftrace-direct-multi-modify.c
> >> >>>>>> index d52370cad0b6..10884bf418f7 100644
> >> >>>>>> --- a/samples/ftrace/ftrace-direct-multi-modify.c
> >> >>>>>> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> >> >>>>>> @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
> >> >>>>>> extern void my_tramp1(void *);
> >> >>>>>> extern void my_tramp2(void *);
> >> >>>>>>
> >> >>>>>> +#ifdef CONFIG_RISCV
> >> >>>>>> +
> >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >> >>>>>> +" .type my_tramp1, @function\n"
> >> >>>>>> +" .globl my_tramp1\n"
> >> >>>>>> +" my_tramp1:\n"
> >> >>>>>> +" addi sp,sp,-24\n"
> >> >>>>>> +" sd a0,0(sp)\n"
> >> >>>>>> +" sd t0,8(sp)\n"
> >> >>>>>> +" sd ra,16(sp)\n"
> >> >>>>>> +" call my_direct_func1\n"
> >> >>>>>> +" ld a0,0(sp)\n"
> >> >>>>>> +" ld t0,8(sp)\n"
> >> >>>>>> +" ld ra,16(sp)\n"
> >> >>>>>> +" addi sp,sp,24\n"
> >> >>>>>> +" jr t0\n"
> >> >>>>>> +" .size my_tramp1, .-my_tramp1\n"
> >> >>>>>> +
> >> >>>>>> +" .type my_tramp2, @function\n"
> >> >>>>>> +" .globl my_tramp2\n"
> >> >>>>>> +" my_tramp2:\n"
> >> >>>>>> +" addi sp,sp,-24\n"
> >> >>>>>> +" sd a0,0(sp)\n"
> >> >>>>>> +" sd t0,8(sp)\n"
> >> >>>>>> +" sd ra,16(sp)\n"
> >> >>>>>> +" call my_direct_func2\n"
> >> >>>>>> +" ld a0,0(sp)\n"
> >> >>>>>> +" ld t0,8(sp)\n"
> >> >>>>>> +" ld ra,16(sp)\n"
> >> >>>>>> +" addi sp,sp,24\n"
> >> >>>>>> +" jr t0\n"
> >> >>>>>> +" .size my_tramp2, .-my_tramp2\n"
> >> >>>>>> +" .popsection\n"
> >> >>>>>> +);
> >> >>>>>> +
> >> >>>>>> +#endif /* CONFIG_RISCV */
> >> >>>>>> +
> >> >>>>>> #ifdef CONFIG_X86_64
> >> >>>>>>
> >> >>>>>> #include <asm/ibt.h>
> >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-multi.c
> >> >>>>>> b/samples/ftrace/ftrace-direct-multi.c
> >> >>>>>> index ec1088922517..a35bf43bf6d7 100644
> >> >>>>>> --- a/samples/ftrace/ftrace-direct-multi.c
> >> >>>>>> +++ b/samples/ftrace/ftrace-direct-multi.c
> >> >>>>>> @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
> >> >>>>>>
> >> >>>>>> extern void my_tramp(void *);
> >> >>>>>>
> >> >>>>>> +#ifdef CONFIG_RISCV
> >> >>>>>> +
> >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >> >>>>>> +" .type my_tramp, @function\n"
> >> >>>>>> +" .globl my_tramp\n"
> >> >>>>>> +" my_tramp:\n"
> >> >>>>>> +" addi sp,sp,-24\n"
> >> >>>>>> +" sd a0,0(sp)\n"
> >> >>>>>> +" sd t0,8(sp)\n"
> >> >>>>>> +" sd ra,16(sp)\n"
> >> >>>>>> +" call my_direct_func\n"
> >> >>>>>> +" ld a0,0(sp)\n"
> >> >>>>>> +" ld t0,8(sp)\n"
> >> >>>>>> +" ld ra,16(sp)\n"
> >> >>>>>> +" addi sp,sp,24\n"
> >> >>>>>> +" jr t0\n"
> >> >>>>>> +" .size my_tramp, .-my_tramp\n"
> >> >>>>>> +" .popsection\n"
> >> >>>>>> +);
> >> >>>>>> +
> >> >>>>>> +#endif /* CONFIG_RISCV */
> >> >>>>>> +
> >> >>>>>> #ifdef CONFIG_X86_64
> >> >>>>>>
> >> >>>>>> #include <asm/ibt.h>
> >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-too.c
> >> >>>>>> b/samples/ftrace/ftrace-direct-too.c
> >> >>>>>> index e13fb59a2b47..3b62e33c2e6d 100644
> >> >>>>>> --- a/samples/ftrace/ftrace-direct-too.c
> >> >>>>>> +++ b/samples/ftrace/ftrace-direct-too.c
> >> >>>>>> @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
> >> >>>>>>
> >> >>>>>> extern void my_tramp(void *);
> >> >>>>>>
> >> >>>>>> +#ifdef CONFIG_RISCV
> >> >>>>>> +
> >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >> >>>>>> +" .type my_tramp, @function\n"
> >> >>>>>> +" .globl my_tramp\n"
> >> >>>>>> +" my_tramp:\n"
> >> >>>>>> +" addi sp,sp,-40\n"
> >> >>>>>> +" sd a0,0(sp)\n"
> >> >>>>>> +" sd a1,8(sp)\n"
> >> >>>>>> +" sd a2,16(sp)\n"
> >> >>>>>> +" sd t0,24(sp)\n"
> >> >>>>>> +" sd ra,32(sp)\n"
> >> >>>>>> +" call my_direct_func\n"
> >> >>>>>> +" ld a0,0(sp)\n"
> >> >>>>>> +" ld a1,8(sp)\n"
> >> >>>>>> +" ld a2,16(sp)\n"
> >> >>>>>> +" ld t0,24(sp)\n"
> >> >>>>>> +" ld ra,32(sp)\n"
> >> >>>>>> +" addi sp,sp,40\n"
> >> >>>>>> +" jr t0\n"
> >> >>>>>> +" .size my_tramp, .-my_tramp\n"
> >> >>>>>> +" .popsection\n"
> >> >>>>>> +);
> >> >>>>>> +
> >> >>>>>> +#endif /* CONFIG_RISCV */
> >> >>>>>> +
> >> >>>>>> #ifdef CONFIG_X86_64
> >> >>>>>>
> >> >>>>>> #include <asm/ibt.h>
> >> >>>>>> diff --git a/samples/ftrace/ftrace-direct.c
> >> >>>>>> b/samples/ftrace/ftrace-direct.c
> >> >>>>>> index 1f769d0db20f..2cfe5a7d2d70 100644
> >> >>>>>> --- a/samples/ftrace/ftrace-direct.c
> >> >>>>>> +++ b/samples/ftrace/ftrace-direct.c
> >> >>>>>> @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
> >> >>>>>>
> >> >>>>>> extern void my_tramp(void *);
> >> >>>>>>
> >> >>>>>> +#ifdef CONFIG_RISCV
> >> >>>>>> +
> >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >> >>>>>> +" .type my_tramp, @function\n"
> >> >>>>>> +" .globl my_tramp\n"
> >> >>>>>> +" my_tramp:\n"
> >> >>>>>> +" addi sp,sp,-24\n"
> >> >>>>>> +" sd a0,0(sp)\n"
> >> >>>>>> +" sd t0,8(sp)\n"
> >> >>>>>> +" sd ra,16(sp)\n"
> >> >>>>>> +" call my_direct_func\n"
> >> >>>>>> +" ld a0,0(sp)\n"
> >> >>>>>> +" ld t0,8(sp)\n"
> >> >>>>>> +" ld ra,16(sp)\n"
> >> >>>>>> +" addi sp,sp,24\n"
> >> >>>>>> +" jr t0\n"
> >> >>>>>> +" .size my_tramp, .-my_tramp\n"
> >> >>>>>> +" .popsection\n"
> >> >>>>>> +);
> >> >>>>>> +
> >> >>>>>> +#endif /* CONFIG_RISCV */
> >> >>>>>> +
> >> >>>>>> #ifdef CONFIG_X86_64
> >> >>>>>>
> >> >>>>>> #include <asm/ibt.h>
> >> >>>>>> --
> >> >>>>>> 2.36.1
> >> >>>>
> >> >>>> --
> >> >>>> Thanks,
> >> >>>> Song
> >> >>>>
> >> >>>
> >> >>> Regards,
> >> >>> Evgenii
> >> >>
> >> >>
> >> >
> >> >
> >> > --
> >> > Thanks,
> >> > Song
> >> >
> >>
> >>
> >
> >
> > --
> > Best Regards
> > Guo Ren
--
Thanks,
Song
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-02-20 2:46 ` Song Shuai
@ 2023-02-21 3:56 ` Guo Ren
0 siblings, 0 replies; 44+ messages in thread
From: Guo Ren @ 2023-02-21 3:56 UTC (permalink / raw)
To: Song Shuai
Cc: Palmer Dabbelt, e.shatokhin, linux-riscv, linux-kernel, anup,
Paul Walmsley, Conor Dooley, heiko, rostedt, mhiramat, jolsa, bp,
jpoimboe, andy.chiu, linux
On Mon, Feb 20, 2023 at 10:46 AM Song Shuai <suagrfillet@gmail.com> wrote:
>
> Hi,Guo:
>
> Palmer Dabbelt <palmer@dabbelt.com> 于2023年2月18日周六 21:30写道：
> >
> > On Wed, 18 Jan 2023 22:05:04 PST (-0800), guoren@kernel.org wrote:
> > > Thx Evgenii & Song,
> > >
> > > I got it; it would be put into v8.
> >
> There were 2 problems for this patch in its V6 and V7 version:
>
> 1. build error resulted by including of nospec-branch.h file in V6
> - this common fixup patch had been sent to the linux-trace mail
> list and would show up in Linux v6.3
> - here is the link:
> https://lore.kernel.org/lkml/20230208015633.791198913@goodmis.org/
>
> 2. invalid code address reported by Evgenii in V7
> - for your convenience, the link to this updated patch is
> https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318
>
> Is it time to launch the V8 with this updated patch, or waiting for
> the common fixup patch to be merged?
I don't know.
I've made v8 based on the newest remotes/palmer/for-next with your
newest branch:
https://github.com/guoren83/linux/tree/ftrace_fixup_v8
I fixed some conflicts with the palmer/for-next, You could send them any time.
>
> > Sorry if I missed it, but I don't see a v8 on patchwork. I queued up
> > the first three patches onto for-next as they seem like pretty
> > independent fixes/cleanups, I'll hold off on the others until v8.
> >
> > Thanks!
> >
> > >
> > > On Wed, Jan 18, 2023 at 11:19 PM Evgenii Shatokhin
> > > <e.shatokhin@yadro.com> wrote:
> > >>
> > >> On 18.01.2023 05:37, Song Shuai wrote:
> > >> > Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月17日周二 16:22写道：
> > >> >>
> > >> >> On 17.01.2023 16:16, Evgenii Shatokhin wrote:
> > >> >>> Hi, Song,
> > >> >>>
> > >> >>> On 17.01.2023 12:32, Song Shuai wrote:
> > >> >>>>
> > >> >>>> Hi, Evgenii:
> > >> >>>>
> > >> >>>> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
> > >> >>>>
> > >> >>>>>
> > >> >>>>> Hi,
> > >> >>>>>
> > >> >>>>> On 12.01.2023 12:06, guoren@kernel.org wrote:
> > >> >>>>>> From: Song Shuai <suagrfillet@gmail.com>
> > >> >>>>>>
> > >> >>>>>> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> > >> >>>>>> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
> > >> >>>>>> the ftrace-direct*.c files in samples/ftrace/.
> > >> >>>>>>
> > >> >>>>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> > >> >>>>>> Tested-by: Guo Ren <guoren@kernel.org>
> > >> >>>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
> > >> >>>>>> ---
> > >> >>>>>> arch/riscv/Kconfig | 2 ++
> > >> >>>>>> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
> > >> >>>>>> samples/ftrace/ftrace-direct-multi-modify.c | 37
> > >> >>>>>> +++++++++++++++++++++
> > >> >>>>>> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
> > >> >>>>>> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
> > >> >>>>>> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
> > >> >>>>>> 6 files changed, 142 insertions(+)
> > >> >>>>>
> > >> >>>>> The samples were built OK now, but ftrace-direct-multi and
> > >> >>>>> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
> > >> >>>>>
> > >> >>>>> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
> > >> >>>>> the messages in the trace:
> > >> >>>>>
> > >> >>>>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> > >> >>>>> # | | | ||||| | |
> > >> >>>>> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
> > >> >>>>> direct func1 ip 0
> > >> >>>>> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
> > >> >>>>> direct func1 ip ff60000001ba9600
> > >> >>>>> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
> > >> >>>>> direct func1 ip ff60000003334d00
> > >> >>>>> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
> > >> >>>>> direct func1 ip 0
> > >> >>>>> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
> > >> >>>>> direct func1 ip 0
> > >> >>>>> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
> > >> >>>>> direct func1 ip 7fffffffffffffff
> > >> >>>>> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
> > >> >>>>> direct func1 ip ff60000001ba9600
> > >> >>>>> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
> > >> >>>>> direct func1 ip ff60000001ba9600
> > >> >>>>> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
> > >> >>>>> direct func1 ip 0
> > >> >>>>> [...]
> > >> >>>>>
> > >> >>>>> If I understand it right, my_direct_func1() should print the address of
> > >> >>>>> some location in the code, probably - at the beginning of the traced
> > >> >>>>> functions.
> > >> >>>>>
> > >> >>>>> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
> > >> >>>>> addresses.
> > >> >>>>>
> > >> >>>> The invalid code address is only printed by accessing the schedule()
> > >> >>>> function's first argument whose address stores in a0 register.
> > >> >>>> While schedule() actually has no parameter declared, so my_direct_func
> > >> >>>> just prints the a0 in the context of the schedule()'s caller and
> > >> >>>> the address maybe varies depending on the caller.
> > >> >>>>
> > >> >>>> I can't really understand why tracing the first argument of the
> > >> >>>> schedule() function, but it seems nonsense at this point.
> > >> >>>
> > >> >>> The question is, what should be passed as the argument(s) of
> > >> >>> my_direct_func() in this particular sample module. The kernel docs and
> > >> >>> commit logs seem to contain no info on that.
> > >> >>>
> > >> >>> With direct functions, I suppose, the trampoline can pass anything it
> > >> >>> wants to my_direct_func(), not just the arguments of the traced function.
> > >> >>>
> > >> >>> I'd check what these sample modules do on x86 and would try to match
> > >> >>> that behaviour on RISC-V.
> > >> >>
> > >> >> I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko
> > >> >> on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should
> > >> >> be passed to my_direct_func().
> > >> >>
> > >> >> ftrace-direct-multi.ko:
> > >> >> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> > >> >> # | | | ||||| | |
> > >> >> insmod-10829 [000] d.h1. 1719.518535: my_direct_func: ip
> > >> >> ffffffff87332f45 // wake_up_process+0x5
> > >> >> rcu_tasks_kthre-11 [000] ..... 1719.518696: my_direct_func: ip
> > >> >> ffffffff8828d935 // schedule+0x5
> > >> >> insmod-10829 [000] ..... 1719.518708: my_direct_func: ip
> > >> >> ffffffff8828d935
> > >> >> systemd-journal-293 [001] ..... 1719.518823: my_direct_func: ip
> > >> >> ffffffff8828d935
> > >> >> systemd-1 [000] ..... 1719.519141: my_direct_func: ip
> > >> >> ffffffff8828d935
> > >> >> <idle>-0 [001] ..s1. 1719.521889: my_direct_func: ip
> > >> >> ffffffff87332f45
> > >> >> <idle>-0 [000] d.s2. 1719.521901: my_direct_func: ip
> > >> >> ffffffff87332f45
> > >> >> rcu_preempt-15 [001] ..... 1719.521917: my_direct_func: ip
> > >> >> ffffffff8828d935
> > >> >> [...]
> > >> >>
> > >> >> The ip values are wake_up_process+0x5 and schedule+0x5, the locations
> > >> >> where the execution of the traced functions resumes after the Ftrace
> > >> >> trampoline has finished.
> > >> >>
> > >> >> The results with ftrace-direct-multi-modify.ko are similar to that.
> > >> >>
> > >> >> The samples look like a demonstration, that one can pass anything
> > >> >> necessary to the handler in case of "direct" functions.
> > >> >>
> > >> >> I suppose, the RISC-V-specific asm code in these two sample modules
> > >> >> could be updated to pass the saved pc value to my_direct_func() in a0.
> > >> >
> > >> > Yes, you're right.
> > >> >
> > >> > I added 'mv a0,t0' in front of `call my_direct_func` to pass the address of
> > >> > traced function with mcount offset.
> > >> >
> > >> > Here is the updated patch for your reference.
> > >> > https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318
> > >>
> > >> Thank you for the quick fix. This one looks good to me.
> > >>
> > >> ftrace-direct-multi*.ko now report the ip values corresponding to
> > >> schedule+0x8 and wake_up_process+0x8, which is what was expected here.
> > >>
> > >> One more thing: please change my "Co-developed-by:" into "Tested-by:" in
> > >> your patch, becase this is what I actually did: tested it and reported
> > >> the results. I cannot take your credit for development of this patch ;-)
> > >>
> > >> Looking forward for v8 of the series.
> > >> >
> > >> >
> > >> >>
> > >> >>>
> > >> >>>>
> > >> >>>> As for this patch, it just impls a simple mcount (direct_caller) to
> > >> >>>> trace kernel functions, and basically saves the necessary ABI,
> > >> >>>> call the tracing function, and restores the ABI, just like other
> > >> >>>> arches do.
> > >> >>>> so It shouldn't be blamed.
> > >> >>>>
> > >> >>>> I started an independent patch to replace schedule with kick_process
> > >> >>>> to make these samples more reasonable. And It has no conflict with the
> > >> >>>> current patch, so we can go on.
> > >> >>>>
> > >> >>>> Link:
> > >> >>>> https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
> > >> >>>>
> > >> >>>>> The same issue is with ftrace-direct-multi-modify.ko.
> > >> >>>>>
> > >> >>>>> Is anything missing here?
> > >> >>>>>
> > >> >>>>>>
> > >> >>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > >> >>>>>> index 307a9f413edd..e944af44f681 100644
> > >> >>>>>> --- a/arch/riscv/Kconfig
> > >> >>>>>> +++ b/arch/riscv/Kconfig
> > >> >>>>>> @@ -112,6 +112,8 @@ config RISCV
> > >> >>>>>> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
> > >> >>>>>> select HAVE_REGS_AND_STACK_ACCESS_API
> > >> >>>>>> select HAVE_FUNCTION_ARG_ACCESS_API
> > >> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT
> > >> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> > >> >>>>>> select HAVE_STACKPROTECTOR
> > >> >>>>>> select HAVE_SYSCALL_TRACEPOINTS
> > >> >>>>>> select HAVE_RSEQ
> > >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-modify.c
> > >> >>>>>> b/samples/ftrace/ftrace-direct-modify.c
> > >> >>>>>> index de5a0f67f320..be7bf472c3c7 100644
> > >> >>>>>> --- a/samples/ftrace/ftrace-direct-modify.c
> > >> >>>>>> +++ b/samples/ftrace/ftrace-direct-modify.c
> > >> >>>>>> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
> > >> >>>>>>
> > >> >>>>>> static unsigned long my_ip = (unsigned long)schedule;
> > >> >>>>>>
> > >> >>>>>> +#ifdef CONFIG_RISCV
> > >> >>>>>> +
> > >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> > >> >>>>>> +" .type my_tramp1, @function\n"
> > >> >>>>>> +" .globl my_tramp1\n"
> > >> >>>>>> +" my_tramp1:\n"
> > >> >>>>>> +" addi sp,sp,-16\n"
> > >> >>>>>> +" sd t0,0(sp)\n"
> > >> >>>>>> +" sd ra,8(sp)\n"
> > >> >>>>>> +" call my_direct_func1\n"
> > >> >>>>>> +" ld t0,0(sp)\n"
> > >> >>>>>> +" ld ra,8(sp)\n"
> > >> >>>>>> +" addi sp,sp,16\n"
> > >> >>>>>> +" jr t0\n"
> > >> >>>>>> +" .size my_tramp1, .-my_tramp1\n"
> > >> >>>>>> +
> > >> >>>>>> +" .type my_tramp2, @function\n"
> > >> >>>>>> +" .globl my_tramp2\n"
> > >> >>>>>> +" my_tramp2:\n"
> > >> >>>>>> +" addi sp,sp,-16\n"
> > >> >>>>>> +" sd t0,0(sp)\n"
> > >> >>>>>> +" sd ra,8(sp)\n"
> > >> >>>>>> +" call my_direct_func2\n"
> > >> >>>>>> +" ld t0,0(sp)\n"
> > >> >>>>>> +" ld ra,8(sp)\n"
> > >> >>>>>> +" addi sp,sp,16\n"
> > >> >>>>>> +" jr t0\n"
> > >> >>>>>> +" .size my_tramp2, .-my_tramp2\n"
> > >> >>>>>> +" .popsection\n"
> > >> >>>>>> +);
> > >> >>>>>> +
> > >> >>>>>> +#endif /* CONFIG_RISCV */
> > >> >>>>>> +
> > >> >>>>>> #ifdef CONFIG_X86_64
> > >> >>>>>>
> > >> >>>>>> #include <asm/ibt.h>
> > >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c
> > >> >>>>>> b/samples/ftrace/ftrace-direct-multi-modify.c
> > >> >>>>>> index d52370cad0b6..10884bf418f7 100644
> > >> >>>>>> --- a/samples/ftrace/ftrace-direct-multi-modify.c
> > >> >>>>>> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> > >> >>>>>> @@ -21,6 +21,43 @@ void my_direct_func2(unsigned long ip)
> > >> >>>>>> extern void my_tramp1(void *);
> > >> >>>>>> extern void my_tramp2(void *);
> > >> >>>>>>
> > >> >>>>>> +#ifdef CONFIG_RISCV
> > >> >>>>>> +
> > >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> > >> >>>>>> +" .type my_tramp1, @function\n"
> > >> >>>>>> +" .globl my_tramp1\n"
> > >> >>>>>> +" my_tramp1:\n"
> > >> >>>>>> +" addi sp,sp,-24\n"
> > >> >>>>>> +" sd a0,0(sp)\n"
> > >> >>>>>> +" sd t0,8(sp)\n"
> > >> >>>>>> +" sd ra,16(sp)\n"
> > >> >>>>>> +" call my_direct_func1\n"
> > >> >>>>>> +" ld a0,0(sp)\n"
> > >> >>>>>> +" ld t0,8(sp)\n"
> > >> >>>>>> +" ld ra,16(sp)\n"
> > >> >>>>>> +" addi sp,sp,24\n"
> > >> >>>>>> +" jr t0\n"
> > >> >>>>>> +" .size my_tramp1, .-my_tramp1\n"
> > >> >>>>>> +
> > >> >>>>>> +" .type my_tramp2, @function\n"
> > >> >>>>>> +" .globl my_tramp2\n"
> > >> >>>>>> +" my_tramp2:\n"
> > >> >>>>>> +" addi sp,sp,-24\n"
> > >> >>>>>> +" sd a0,0(sp)\n"
> > >> >>>>>> +" sd t0,8(sp)\n"
> > >> >>>>>> +" sd ra,16(sp)\n"
> > >> >>>>>> +" call my_direct_func2\n"
> > >> >>>>>> +" ld a0,0(sp)\n"
> > >> >>>>>> +" ld t0,8(sp)\n"
> > >> >>>>>> +" ld ra,16(sp)\n"
> > >> >>>>>> +" addi sp,sp,24\n"
> > >> >>>>>> +" jr t0\n"
> > >> >>>>>> +" .size my_tramp2, .-my_tramp2\n"
> > >> >>>>>> +" .popsection\n"
> > >> >>>>>> +);
> > >> >>>>>> +
> > >> >>>>>> +#endif /* CONFIG_RISCV */
> > >> >>>>>> +
> > >> >>>>>> #ifdef CONFIG_X86_64
> > >> >>>>>>
> > >> >>>>>> #include <asm/ibt.h>
> > >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-multi.c
> > >> >>>>>> b/samples/ftrace/ftrace-direct-multi.c
> > >> >>>>>> index ec1088922517..a35bf43bf6d7 100644
> > >> >>>>>> --- a/samples/ftrace/ftrace-direct-multi.c
> > >> >>>>>> +++ b/samples/ftrace/ftrace-direct-multi.c
> > >> >>>>>> @@ -16,6 +16,28 @@ void my_direct_func(unsigned long ip)
> > >> >>>>>>
> > >> >>>>>> extern void my_tramp(void *);
> > >> >>>>>>
> > >> >>>>>> +#ifdef CONFIG_RISCV
> > >> >>>>>> +
> > >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> > >> >>>>>> +" .type my_tramp, @function\n"
> > >> >>>>>> +" .globl my_tramp\n"
> > >> >>>>>> +" my_tramp:\n"
> > >> >>>>>> +" addi sp,sp,-24\n"
> > >> >>>>>> +" sd a0,0(sp)\n"
> > >> >>>>>> +" sd t0,8(sp)\n"
> > >> >>>>>> +" sd ra,16(sp)\n"
> > >> >>>>>> +" call my_direct_func\n"
> > >> >>>>>> +" ld a0,0(sp)\n"
> > >> >>>>>> +" ld t0,8(sp)\n"
> > >> >>>>>> +" ld ra,16(sp)\n"
> > >> >>>>>> +" addi sp,sp,24\n"
> > >> >>>>>> +" jr t0\n"
> > >> >>>>>> +" .size my_tramp, .-my_tramp\n"
> > >> >>>>>> +" .popsection\n"
> > >> >>>>>> +);
> > >> >>>>>> +
> > >> >>>>>> +#endif /* CONFIG_RISCV */
> > >> >>>>>> +
> > >> >>>>>> #ifdef CONFIG_X86_64
> > >> >>>>>>
> > >> >>>>>> #include <asm/ibt.h>
> > >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-too.c
> > >> >>>>>> b/samples/ftrace/ftrace-direct-too.c
> > >> >>>>>> index e13fb59a2b47..3b62e33c2e6d 100644
> > >> >>>>>> --- a/samples/ftrace/ftrace-direct-too.c
> > >> >>>>>> +++ b/samples/ftrace/ftrace-direct-too.c
> > >> >>>>>> @@ -18,6 +18,32 @@ void my_direct_func(struct vm_area_struct *vma,
> > >> >>>>>>
> > >> >>>>>> extern void my_tramp(void *);
> > >> >>>>>>
> > >> >>>>>> +#ifdef CONFIG_RISCV
> > >> >>>>>> +
> > >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> > >> >>>>>> +" .type my_tramp, @function\n"
> > >> >>>>>> +" .globl my_tramp\n"
> > >> >>>>>> +" my_tramp:\n"
> > >> >>>>>> +" addi sp,sp,-40\n"
> > >> >>>>>> +" sd a0,0(sp)\n"
> > >> >>>>>> +" sd a1,8(sp)\n"
> > >> >>>>>> +" sd a2,16(sp)\n"
> > >> >>>>>> +" sd t0,24(sp)\n"
> > >> >>>>>> +" sd ra,32(sp)\n"
> > >> >>>>>> +" call my_direct_func\n"
> > >> >>>>>> +" ld a0,0(sp)\n"
> > >> >>>>>> +" ld a1,8(sp)\n"
> > >> >>>>>> +" ld a2,16(sp)\n"
> > >> >>>>>> +" ld t0,24(sp)\n"
> > >> >>>>>> +" ld ra,32(sp)\n"
> > >> >>>>>> +" addi sp,sp,40\n"
> > >> >>>>>> +" jr t0\n"
> > >> >>>>>> +" .size my_tramp, .-my_tramp\n"
> > >> >>>>>> +" .popsection\n"
> > >> >>>>>> +);
> > >> >>>>>> +
> > >> >>>>>> +#endif /* CONFIG_RISCV */
> > >> >>>>>> +
> > >> >>>>>> #ifdef CONFIG_X86_64
> > >> >>>>>>
> > >> >>>>>> #include <asm/ibt.h>
> > >> >>>>>> diff --git a/samples/ftrace/ftrace-direct.c
> > >> >>>>>> b/samples/ftrace/ftrace-direct.c
> > >> >>>>>> index 1f769d0db20f..2cfe5a7d2d70 100644
> > >> >>>>>> --- a/samples/ftrace/ftrace-direct.c
> > >> >>>>>> +++ b/samples/ftrace/ftrace-direct.c
> > >> >>>>>> @@ -15,6 +15,28 @@ void my_direct_func(struct task_struct *p)
> > >> >>>>>>
> > >> >>>>>> extern void my_tramp(void *);
> > >> >>>>>>
> > >> >>>>>> +#ifdef CONFIG_RISCV
> > >> >>>>>> +
> > >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> > >> >>>>>> +" .type my_tramp, @function\n"
> > >> >>>>>> +" .globl my_tramp\n"
> > >> >>>>>> +" my_tramp:\n"
> > >> >>>>>> +" addi sp,sp,-24\n"
> > >> >>>>>> +" sd a0,0(sp)\n"
> > >> >>>>>> +" sd t0,8(sp)\n"
> > >> >>>>>> +" sd ra,16(sp)\n"
> > >> >>>>>> +" call my_direct_func\n"
> > >> >>>>>> +" ld a0,0(sp)\n"
> > >> >>>>>> +" ld t0,8(sp)\n"
> > >> >>>>>> +" ld ra,16(sp)\n"
> > >> >>>>>> +" addi sp,sp,24\n"
> > >> >>>>>> +" jr t0\n"
> > >> >>>>>> +" .size my_tramp, .-my_tramp\n"
> > >> >>>>>> +" .popsection\n"
> > >> >>>>>> +);
> > >> >>>>>> +
> > >> >>>>>> +#endif /* CONFIG_RISCV */
> > >> >>>>>> +
> > >> >>>>>> #ifdef CONFIG_X86_64
> > >> >>>>>>
> > >> >>>>>> #include <asm/ibt.h>
> > >> >>>>>> --
> > >> >>>>>> 2.36.1
> > >> >>>>
> > >> >>>> --
> > >> >>>> Thanks,
> > >> >>>> Song
> > >> >>>>
> > >> >>>
> > >> >>> Regards,
> > >> >>> Evgenii
> > >> >>
> > >> >>
> > >> >
> > >> >
> > >> > --
> > >> > Thanks,
> > >> > Song
> > >> >
> > >>
> > >>
> > >
> > >
> > > --
> > > Best Regards
> > > Guo Ren
>
>
>
> --
> Thanks,
> Song
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH -next V7 6/7] samples: ftrace: Add riscv support for SAMPLE_FTRACE_DIRECT[_MULTI]
2023-02-18 21:30 ` Palmer Dabbelt
2023-02-20 2:46 ` Song Shuai
@ 2023-02-21 4:02 ` Guo Ren
1 sibling, 0 replies; 44+ messages in thread
From: Guo Ren @ 2023-02-21 4:02 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: e.shatokhin, suagrfillet, linux-riscv, linux-kernel, anup,
Paul Walmsley, Conor Dooley, heiko, rostedt, mhiramat, jolsa, bp,
jpoimboe, andy.chiu, linux
On Sun, Feb 19, 2023 at 5:30 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 18 Jan 2023 22:05:04 PST (-0800), guoren@kernel.org wrote:
> > Thx Evgenii & Song,
> >
> > I got it; it would be put into v8.
>
> Sorry if I missed it, but I don't see a v8 on patchwork. I queued up
> the first three patches onto for-next as they seem like pretty
> independent fixes/cleanups, I'll hold off on the others until v8.
Thx for approving. I would keep the patch series more independent next time.
>
> Thanks!
>
> >
> > On Wed, Jan 18, 2023 at 11:19 PM Evgenii Shatokhin
> > <e.shatokhin@yadro.com> wrote:
> >>
> >> On 18.01.2023 05:37, Song Shuai wrote:
> >> > Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月17日周二 16:22写道：
> >> >>
> >> >> On 17.01.2023 16:16, Evgenii Shatokhin wrote:
> >> >>> Hi, Song,
> >> >>>
> >> >>> On 17.01.2023 12:32, Song Shuai wrote:
> >> >>>>
> >> >>>> Hi, Evgenii:
> >> >>>>
> >> >>>> Evgenii Shatokhin <e.shatokhin@yadro.com> 于2023年1月16日周一 14:30写道：
> >> >>>>
> >> >>>>>
> >> >>>>> Hi,
> >> >>>>>
> >> >>>>> On 12.01.2023 12:06, guoren@kernel.org wrote:
> >> >>>>>> From: Song Shuai <suagrfillet@gmail.com>
> >> >>>>>>
> >> >>>>>> select HAVE_SAMPLE_FTRACE_DIRECT and HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> >> >>>>>> for ARCH_RV64I in arch/riscv/Kconfig. And add riscv asm code for
> >> >>>>>> the ftrace-direct*.c files in samples/ftrace/.
> >> >>>>>>
> >> >>>>>> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> >> >>>>>> Tested-by: Guo Ren <guoren@kernel.org>
> >> >>>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
> >> >>>>>> ---
> >> >>>>>> arch/riscv/Kconfig | 2 ++
> >> >>>>>> samples/ftrace/ftrace-direct-modify.c | 33 ++++++++++++++++++
> >> >>>>>> samples/ftrace/ftrace-direct-multi-modify.c | 37
> >> >>>>>> +++++++++++++++++++++
> >> >>>>>> samples/ftrace/ftrace-direct-multi.c | 22 ++++++++++++
> >> >>>>>> samples/ftrace/ftrace-direct-too.c | 26 +++++++++++++++
> >> >>>>>> samples/ftrace/ftrace-direct.c | 22 ++++++++++++
> >> >>>>>> 6 files changed, 142 insertions(+)
> >> >>>>>
> >> >>>>> The samples were built OK now, but ftrace-direct-multi and
> >> >>>>> ftrace-direct-multi-modify report incorrect values of ip/pc it seems.
> >> >>>>>
> >> >>>>> I ran 'insmod ftrace-direct-multi.ko', waited a little and then checked
> >> >>>>> the messages in the trace:
> >> >>>>>
> >> >>>>> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> >> >>>>> # | | | ||||| | |
> >> >>>>> migration/1-19 [001] ..... 3858.532131: my_direct_func1: my
> >> >>>>> direct func1 ip 0
> >> >>>>> migration/0-15 [000] d.s2. 3858.532136: my_direct_func1: my
> >> >>>>> direct func1 ip ff60000001ba9600
> >> >>>>> migration/0-15 [000] d..2. 3858.532204: my_direct_func1: my
> >> >>>>> direct func1 ip ff60000003334d00
> >> >>>>> migration/0-15 [000] ..... 3858.532232: my_direct_func1: my
> >> >>>>> direct func1 ip 0
> >> >>>>> rcu_sched-14 [001] ..... 3858.532257: my_direct_func1: my
> >> >>>>> direct func1 ip 0
> >> >>>>> insmod-415 [000] ..... 3858.532270: my_direct_func1: my
> >> >>>>> direct func1 ip 7fffffffffffffff
> >> >>>>> <idle>-0 [001] ..s1. 3858.539051: my_direct_func1: my
> >> >>>>> direct func1 ip ff60000001ba9600
> >> >>>>> <idle>-0 [001] dns2. 3858.539124: my_direct_func1: my
> >> >>>>> direct func1 ip ff60000001ba9600
> >> >>>>> rcu_sched-14 [001] ..... 3858.539208: my_direct_func1: my
> >> >>>>> direct func1 ip 0
> >> >>>>> [...]
> >> >>>>>
> >> >>>>> If I understand it right, my_direct_func1() should print the address of
> >> >>>>> some location in the code, probably - at the beginning of the traced
> >> >>>>> functions.
> >> >>>>>
> >> >>>>> The printed values (0x0, 0x7fffffffffffffff, ...) are not valid code
> >> >>>>> addresses.
> >> >>>>>
> >> >>>> The invalid code address is only printed by accessing the schedule()
> >> >>>> function's first argument whose address stores in a0 register.
> >> >>>> While schedule() actually has no parameter declared, so my_direct_func
> >> >>>> just prints the a0 in the context of the schedule()'s caller and
> >> >>>> the address maybe varies depending on the caller.
> >> >>>>
> >> >>>> I can't really understand why tracing the first argument of the
> >> >>>> schedule() function, but it seems nonsense at this point.
> >> >>>
> >> >>> The question is, what should be passed as the argument(s) of
> >> >>> my_direct_func() in this particular sample module. The kernel docs and
> >> >>> commit logs seem to contain no info on that.
> >> >>>
> >> >>> With direct functions, I suppose, the trampoline can pass anything it
> >> >>> wants to my_direct_func(), not just the arguments of the traced function.
> >> >>>
> >> >>> I'd check what these sample modules do on x86 and would try to match
> >> >>> that behaviour on RISC-V.
> >> >>
> >> >> I have checked ftrace-direct-multi.ko and ftrace-direct-multi-modify.ko
> >> >> on 6.2-rc4 built for x86-64 - yes, ip/pc in the traced function should
> >> >> be passed to my_direct_func().
> >> >>
> >> >> ftrace-direct-multi.ko:
> >> >> # TASK-PID CPU# ||||| TIMESTAMP FUNCTION
> >> >> # | | | ||||| | |
> >> >> insmod-10829 [000] d.h1. 1719.518535: my_direct_func: ip
> >> >> ffffffff87332f45 // wake_up_process+0x5
> >> >> rcu_tasks_kthre-11 [000] ..... 1719.518696: my_direct_func: ip
> >> >> ffffffff8828d935 // schedule+0x5
> >> >> insmod-10829 [000] ..... 1719.518708: my_direct_func: ip
> >> >> ffffffff8828d935
> >> >> systemd-journal-293 [001] ..... 1719.518823: my_direct_func: ip
> >> >> ffffffff8828d935
> >> >> systemd-1 [000] ..... 1719.519141: my_direct_func: ip
> >> >> ffffffff8828d935
> >> >> <idle>-0 [001] ..s1. 1719.521889: my_direct_func: ip
> >> >> ffffffff87332f45
> >> >> <idle>-0 [000] d.s2. 1719.521901: my_direct_func: ip
> >> >> ffffffff87332f45
> >> >> rcu_preempt-15 [001] ..... 1719.521917: my_direct_func: ip
> >> >> ffffffff8828d935
> >> >> [...]
> >> >>
> >> >> The ip values are wake_up_process+0x5 and schedule+0x5, the locations
> >> >> where the execution of the traced functions resumes after the Ftrace
> >> >> trampoline has finished.
> >> >>
> >> >> The results with ftrace-direct-multi-modify.ko are similar to that.
> >> >>
> >> >> The samples look like a demonstration, that one can pass anything
> >> >> necessary to the handler in case of "direct" functions.
> >> >>
> >> >> I suppose, the RISC-V-specific asm code in these two sample modules
> >> >> could be updated to pass the saved pc value to my_direct_func() in a0.
> >> >
> >> > Yes, you're right.
> >> >
> >> > I added 'mv a0,t0' in front of `call my_direct_func` to pass the address of
> >> > traced function with mcount offset.
> >> >
> >> > Here is the updated patch for your reference.
> >> > https://github.com/sugarfillet/linux/commit/95b174fb104dd970b982ee6fa19879393e229318
> >>
> >> Thank you for the quick fix. This one looks good to me.
> >>
> >> ftrace-direct-multi*.ko now report the ip values corresponding to
> >> schedule+0x8 and wake_up_process+0x8, which is what was expected here.
> >>
> >> One more thing: please change my "Co-developed-by:" into "Tested-by:" in
> >> your patch, becase this is what I actually did: tested it and reported
> >> the results. I cannot take your credit for development of this patch ;-)
> >>
> >> Looking forward for v8 of the series.
> >> >
> >> >
> >> >>
> >> >>>
> >> >>>>
> >> >>>> As for this patch, it just impls a simple mcount (direct_caller) to
> >> >>>> trace kernel functions, and basically saves the necessary ABI,
> >> >>>> call the tracing function, and restores the ABI, just like other
> >> >>>> arches do.
> >> >>>> so It shouldn't be blamed.
> >> >>>>
> >> >>>> I started an independent patch to replace schedule with kick_process
> >> >>>> to make these samples more reasonable. And It has no conflict with the
> >> >>>> current patch, so we can go on.
> >> >>>>
> >> >>>> Link:
> >> >>>> https://lore.kernel.org/linux-kernel/20230117091101.3669996-1-suagrfillet@gmail.com/T/#u
> >> >>>>
> >> >>>>> The same issue is with ftrace-direct-multi-modify.ko.
> >> >>>>>
> >> >>>>> Is anything missing here?
> >> >>>>>
> >> >>>>>>
> >> >>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> >> >>>>>> index 307a9f413edd..e944af44f681 100644
> >> >>>>>> --- a/arch/riscv/Kconfig
> >> >>>>>> +++ b/arch/riscv/Kconfig
> >> >>>>>> @@ -112,6 +112,8 @@ config RISCV
> >> >>>>>> select HAVE_POSIX_CPU_TIMERS_TASK_WORK
> >> >>>>>> select HAVE_REGS_AND_STACK_ACCESS_API
> >> >>>>>> select HAVE_FUNCTION_ARG_ACCESS_API
> >> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT
> >> >>>>>> + select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
> >> >>>>>> select HAVE_STACKPROTECTOR
> >> >>>>>> select HAVE_SYSCALL_TRACEPOINTS
> >> >>>>>> select HAVE_RSEQ
> >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-modify.c
> >> >>>>>> b/samples/ftrace/ftrace-direct-modify.c
> >> >>>>>> index de5a0f67f320..be7bf472c3c7 100644
> >> >>>>>> --- a/samples/ftrace/ftrace-direct-modify.c
> >> >>>>>> +++ b/samples/ftrace/ftrace-direct-modify.c
> >> >>>>>> @@ -23,6 +23,39 @@ extern void my_tramp2(void *);
> >> >>>>>>
> >> >>>>>> static unsigned long my_ip = (unsigned long)schedule;
> >> >>>>>>
> >> >>>>>> +#ifdef CONFIG_RISCV
> >> >>>>>> +
> >> >>>>>> +asm (" .pushsection .text, \"ax\", @progbits\n"
> >> >>>>>> +" .type my_tramp1, @function\n"
> >> >>>>>> +" .globl my_tramp1\n"
> >> >>>>>> +" my_tramp1:\n"
> >> >>>>>> +" addi sp,sp,-16\n"
> >> >>>>>> +" sd t0,0(sp)\n"
> >> >>>>>> +" sd ra,8(sp)\n"
> >> >>>>>> +" call my_direct_func1\n"
> >> >>>>>> +" ld t0,0(sp)\n"
> >> >>>>>> +" ld ra,8(sp)\n"
> >> >>>>>> +" addi sp,sp,16\n"
> >> >>>>>> +" jr t0\n"
> >> >>>>>> +" .size my_tramp1, .-my_tramp1\n"
> >> >>>>>> +
> >> >>>>>> +" .type my_tramp2, @function\n"
> >> >>>>>> +" .globl my_tramp2\n"
> >> >>>>>> +" my_tramp2:\n"
> >> >>>>>> +" addi sp,sp,-16\n"
> >> >>>>>> +" sd t0,0(sp)\n"
> >> >>>>>> +" sd ra,8(sp)\n"
> >> >>>>>> +" call my_direct_func2\n"
> >> >>>>>> +" ld t0,0(sp)\n"
> >> >>>>>> +" ld ra,8(sp)\n"
> >> >>>>>> +" addi sp,sp,16\n"
> >> >>>>>> +" jr t0\n"
> >> >>>>>> +" .size my_tramp2, .-my_tramp2\n"
> >> >>>>>> +" .popsection\n"
> >> >>>>>> +);
> >> >>>>>> +
> >> >>>>>> +#endif /* CONFIG_RISCV */
> >> >>>>>> +
> >> >>>>>> #ifdef CONFIG_X86_64
> >> >>>>>>
> >> >>>>>> #include <asm/ibt.h>
> >> >>>>>> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c
> >> >>>>>> b/samples/ftrace/ftrace-direct-multi-modify.c
> >> >>>>>> index d52370cad0b6..10884bf418f7 100644
> >> >>>>>>