BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
@ 2021-03-01 19:04 Jiri Olsa
  2021-03-01 22:58 ` Yonghong Song
  2021-03-02  0:34 ` Andrii Nakryiko
  0 siblings, 2 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-03-01 19:04 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen,
	Yauheni Kaliuta

When testing uprobes we the test gets GEP (Global Entry Point)
address from kallsyms, but then the function is called locally
so the uprobe is not triggered.

Fixing this by adjusting the address to LEP (Local Entry Point)
for powerpc arch.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
index a0ee87c8e1ea..c3cfb48d3ed0 100644
--- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
+++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
@@ -2,6 +2,22 @@
 #include <test_progs.h>
 #include "test_attach_probe.skel.h"
 
+#if defined(__powerpc64__)
+/*
+ * We get the GEP (Global Entry Point) address from kallsyms,
+ * but then the function is called locally, so we need to adjust
+ * the address to get LEP (Local Entry Point).
+ */
+#define LEP_OFFSET 8
+
+static ssize_t get_offset(ssize_t offset)
+{
+	return offset + LEP_OFFSET;
+}
+#else
+#define get_offset(offset) (offset)
+#endif
+
 ssize_t get_base_addr() {
 	size_t start, offset;
 	char buf[256];
@@ -36,7 +52,7 @@ void test_attach_probe(void)
 	if (CHECK(base_addr < 0, "get_base_addr",
 		  "failed to find base addr: %zd", base_addr))
 		return;
-	uprobe_offset = (size_t)&get_base_addr - base_addr;
+	uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
 
 	skel = test_attach_probe__open_and_load();
 	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
-- 
2.29.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-01 19:04 [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes Jiri Olsa
@ 2021-03-01 22:58 ` Yonghong Song
  2021-03-02 10:35   ` Jiri Olsa
  2021-03-02  0:34 ` Andrii Nakryiko
  1 sibling, 1 reply; 15+ messages in thread
From: Yonghong Song @ 2021-03-01 22:58 UTC (permalink / raw)
  To: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, John Fastabend,
	KP Singh, Toke Høiland-Jørgensen, Yauheni Kaliuta



On 3/1/21 11:04 AM, Jiri Olsa wrote:
> When testing uprobes we the test gets GEP (Global Entry Point)
> address from kallsyms, but then the function is called locally
> so the uprobe is not triggered.
> 
> Fixing this by adjusting the address to LEP (Local Entry Point)
> for powerpc arch.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> index a0ee87c8e1ea..c3cfb48d3ed0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -2,6 +2,22 @@
>   #include <test_progs.h>
>   #include "test_attach_probe.skel.h"
>   
> +#if defined(__powerpc64__)
> +/*
> + * We get the GEP (Global Entry Point) address from kallsyms,
> + * but then the function is called locally, so we need to adjust
> + * the address to get LEP (Local Entry Point).

Any documentation in the kernel about this behavior? This will
help to validate the change without trying with powerpc64 qemu...

> + */
> +#define LEP_OFFSET 8
> +
> +static ssize_t get_offset(ssize_t offset)
> +{
> +	return offset + LEP_OFFSET;
> +}
> +#else
> +#define get_offset(offset) (offset)
> +#endif
> +
>   ssize_t get_base_addr() {
>   	size_t start, offset;
>   	char buf[256];
> @@ -36,7 +52,7 @@ void test_attach_probe(void)
>   	if (CHECK(base_addr < 0, "get_base_addr",
>   		  "failed to find base addr: %zd", base_addr))
>   		return;
> -	uprobe_offset = (size_t)&get_base_addr - base_addr;
> +	uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
>   
>   	skel = test_attach_probe__open_and_load();
>   	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-01 19:04 [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes Jiri Olsa
  2021-03-01 22:58 ` Yonghong Song
@ 2021-03-02  0:34 ` Andrii Nakryiko
  2021-03-02 11:14   ` Jiri Olsa
  1 sibling, 1 reply; 15+ messages in thread
From: Andrii Nakryiko @ 2021-03-02  0:34 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
	bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Toke Høiland-Jørgensen, Yauheni Kaliuta

On Mon, Mar 1, 2021 at 11:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> When testing uprobes we the test gets GEP (Global Entry Point)
> address from kallsyms, but then the function is called locally
> so the uprobe is not triggered.
>
> Fixing this by adjusting the address to LEP (Local Entry Point)
> for powerpc arch.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> index a0ee87c8e1ea..c3cfb48d3ed0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -2,6 +2,22 @@
>  #include <test_progs.h>
>  #include "test_attach_probe.skel.h"
>
> +#if defined(__powerpc64__)
> +/*
> + * We get the GEP (Global Entry Point) address from kallsyms,
> + * but then the function is called locally, so we need to adjust
> + * the address to get LEP (Local Entry Point).
> + */
> +#define LEP_OFFSET 8
> +
> +static ssize_t get_offset(ssize_t offset)

if we mark this function __weak global, would it work as is? Would it
get an address of a global entry point? I know nothing about this GEP
vs LEP stuff, interesting :)

> +{
> +       return offset + LEP_OFFSET;
> +}
> +#else
> +#define get_offset(offset) (offset)
> +#endif
> +
>  ssize_t get_base_addr() {
>         size_t start, offset;
>         char buf[256];
> @@ -36,7 +52,7 @@ void test_attach_probe(void)
>         if (CHECK(base_addr < 0, "get_base_addr",
>                   "failed to find base addr: %zd", base_addr))
>                 return;
> -       uprobe_offset = (size_t)&get_base_addr - base_addr;
> +       uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
>
>         skel = test_attach_probe__open_and_load();
>         if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> --
> 2.29.2
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-01 22:58 ` Yonghong Song
@ 2021-03-02 10:35   ` Jiri Olsa
  2021-03-03  6:40     ` Naveen N. Rao
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2021-03-02 10:35 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	netdev, bpf, Martin KaFai Lau, Song Liu, John Fastabend,
	KP Singh, Toke Høiland-Jørgensen, Yauheni Kaliuta,
	Naveen N. Rao, Michael Ellerman, Srikar Dronamraju,
	Paul Mackerras

On Mon, Mar 01, 2021 at 02:58:53PM -0800, Yonghong Song wrote:
> 
> 
> On 3/1/21 11:04 AM, Jiri Olsa wrote:
> > When testing uprobes we the test gets GEP (Global Entry Point)
> > address from kallsyms, but then the function is called locally
> > so the uprobe is not triggered.
> > 
> > Fixing this by adjusting the address to LEP (Local Entry Point)
> > for powerpc arch.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> >   1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > @@ -2,6 +2,22 @@
> >   #include <test_progs.h>
> >   #include "test_attach_probe.skel.h"
> > +#if defined(__powerpc64__)
> > +/*
> > + * We get the GEP (Global Entry Point) address from kallsyms,
> > + * but then the function is called locally, so we need to adjust
> > + * the address to get LEP (Local Entry Point).
> 
> Any documentation in the kernel about this behavior? This will
> help to validate the change without trying with powerpc64 qemu...

we got similar fix in perf:

7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms lookup

CC-ing few other folks from ppc land for more info

jirka

> 
> > + */
> > +#define LEP_OFFSET 8
> > +
> > +static ssize_t get_offset(ssize_t offset)
> > +{
> > +	return offset + LEP_OFFSET;
> > +}
> > +#else
> > +#define get_offset(offset) (offset)
> > +#endif
> > +
> >   ssize_t get_base_addr() {
> >   	size_t start, offset;
> >   	char buf[256];
> > @@ -36,7 +52,7 @@ void test_attach_probe(void)
> >   	if (CHECK(base_addr < 0, "get_base_addr",
> >   		  "failed to find base addr: %zd", base_addr))
> >   		return;
> > -	uprobe_offset = (size_t)&get_base_addr - base_addr;
> > +	uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
> >   	skel = test_attach_probe__open_and_load();
> >   	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> > 
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-02  0:34 ` Andrii Nakryiko
@ 2021-03-02 11:14   ` Jiri Olsa
  2021-03-02 17:19     ` Yonghong Song
  2021-03-02 18:31     ` Andrii Nakryiko
  0 siblings, 2 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-03-02 11:14 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Networking, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen,
	Yauheni Kaliuta

On Mon, Mar 01, 2021 at 04:34:24PM -0800, Andrii Nakryiko wrote:
> On Mon, Mar 1, 2021 at 11:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > When testing uprobes we the test gets GEP (Global Entry Point)
> > address from kallsyms, but then the function is called locally
> > so the uprobe is not triggered.
> >
> > Fixing this by adjusting the address to LEP (Local Entry Point)
> > for powerpc arch.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > @@ -2,6 +2,22 @@
> >  #include <test_progs.h>
> >  #include "test_attach_probe.skel.h"
> >
> > +#if defined(__powerpc64__)
> > +/*
> > + * We get the GEP (Global Entry Point) address from kallsyms,
> > + * but then the function is called locally, so we need to adjust
> > + * the address to get LEP (Local Entry Point).
> > + */
> > +#define LEP_OFFSET 8
> > +
> > +static ssize_t get_offset(ssize_t offset)
> 
> if we mark this function __weak global, would it work as is? Would it
> get an address of a global entry point? I know nothing about this GEP
> vs LEP stuff, interesting :)

you mean get_base_addr? it's already global

all the calls to get_base_addr within the object are made
to get_base_addr+0x8

00000000100350c0 <test_attach_probe>:
    ...
    100350e0:   59 fd ff 4b     bl      10034e38 <get_base_addr+0x8>
    ...
    100358a8:   91 f5 ff 4b     bl      10034e38 <get_base_addr+0x8>


I'm following perf fix we had for similar issue:
  7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms lookup

I'll get more info on that

jirka

> 
> > +{
> > +       return offset + LEP_OFFSET;
> > +}
> > +#else
> > +#define get_offset(offset) (offset)
> > +#endif
> > +
> >  ssize_t get_base_addr() {
> >         size_t start, offset;
> >         char buf[256];
> > @@ -36,7 +52,7 @@ void test_attach_probe(void)
> >         if (CHECK(base_addr < 0, "get_base_addr",
> >                   "failed to find base addr: %zd", base_addr))
> >                 return;
> > -       uprobe_offset = (size_t)&get_base_addr - base_addr;
> > +       uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
> >
> >         skel = test_attach_probe__open_and_load();
> >         if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> > --
> > 2.29.2
> >
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-02 11:14   ` Jiri Olsa
@ 2021-03-02 17:19     ` Yonghong Song
  2021-03-03  6:42       ` Naveen N. Rao
  2021-03-02 18:31     ` Andrii Nakryiko
  1 sibling, 1 reply; 15+ messages in thread
From: Yonghong Song @ 2021-03-02 17:19 UTC (permalink / raw)
  To: Jiri Olsa, Andrii Nakryiko
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Networking, bpf, Martin KaFai Lau, Song Liu, John Fastabend,
	KP Singh, Toke Høiland-Jørgensen, Yauheni Kaliuta



On 3/2/21 3:14 AM, Jiri Olsa wrote:
> On Mon, Mar 01, 2021 at 04:34:24PM -0800, Andrii Nakryiko wrote:
>> On Mon, Mar 1, 2021 at 11:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
>>>
>>> When testing uprobes we the test gets GEP (Global Entry Point)
>>> address from kallsyms, but then the function is called locally
>>> so the uprobe is not triggered.
>>>
>>> Fixing this by adjusting the address to LEP (Local Entry Point)
>>> for powerpc arch.
>>>
>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>> ---
>>>   .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
>>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
>>> index a0ee87c8e1ea..c3cfb48d3ed0 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
>>> @@ -2,6 +2,22 @@
>>>   #include <test_progs.h>
>>>   #include "test_attach_probe.skel.h"
>>>
>>> +#if defined(__powerpc64__)
>>> +/*
>>> + * We get the GEP (Global Entry Point) address from kallsyms,
>>> + * but then the function is called locally, so we need to adjust
>>> + * the address to get LEP (Local Entry Point).
>>> + */
>>> +#define LEP_OFFSET 8
>>> +
>>> +static ssize_t get_offset(ssize_t offset)
>>
>> if we mark this function __weak global, would it work as is? Would it
>> get an address of a global entry point? I know nothing about this GEP
>> vs LEP stuff, interesting :)
> 
> you mean get_base_addr? it's already global
> 
> all the calls to get_base_addr within the object are made
> to get_base_addr+0x8
> 
> 00000000100350c0 <test_attach_probe>:
>      ...
>      100350e0:   59 fd ff 4b     bl      10034e38 <get_base_addr+0x8>
>      ...
>      100358a8:   91 f5 ff 4b     bl      10034e38 <get_base_addr+0x8>
> 
> 
> I'm following perf fix we had for similar issue:
>    7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms lookup
> 
> I'll get more info on that

Thanks. The patch
    7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using 
kallsyms lookup
talked about offset + 8 for kernel symbols.
I guess uprobe symbol might be in the same situation if using the
same compilation mechanism as kernel. But it would be good
to get confirmation from ppc people.

> 
> jirka
> 
>>
>>> +{
>>> +       return offset + LEP_OFFSET;
>>> +}
>>> +#else
>>> +#define get_offset(offset) (offset)
>>> +#endif
>>> +
>>>   ssize_t get_base_addr() {
>>>          size_t start, offset;
>>>          char buf[256];
>>> @@ -36,7 +52,7 @@ void test_attach_probe(void)
>>>          if (CHECK(base_addr < 0, "get_base_addr",
>>>                    "failed to find base addr: %zd", base_addr))
>>>                  return;
>>> -       uprobe_offset = (size_t)&get_base_addr - base_addr;
>>> +       uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
>>>
>>>          skel = test_attach_probe__open_and_load();
>>>          if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
>>> --
>>> 2.29.2
>>>
>>
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-02 11:14   ` Jiri Olsa
  2021-03-02 17:19     ` Yonghong Song
@ 2021-03-02 18:31     ` Andrii Nakryiko
  2021-03-02 18:58       ` Jiri Olsa
  1 sibling, 1 reply; 15+ messages in thread
From: Andrii Nakryiko @ 2021-03-02 18:31 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Networking, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen,
	Yauheni Kaliuta

On Tue, Mar 2, 2021 at 3:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Mar 01, 2021 at 04:34:24PM -0800, Andrii Nakryiko wrote:
> > On Mon, Mar 1, 2021 at 11:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > When testing uprobes we the test gets GEP (Global Entry Point)
> > > address from kallsyms, but then the function is called locally
> > > so the uprobe is not triggered.
> > >
> > > Fixing this by adjusting the address to LEP (Local Entry Point)
> > > for powerpc arch.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > @@ -2,6 +2,22 @@
> > >  #include <test_progs.h>
> > >  #include "test_attach_probe.skel.h"
> > >
> > > +#if defined(__powerpc64__)
> > > +/*
> > > + * We get the GEP (Global Entry Point) address from kallsyms,
> > > + * but then the function is called locally, so we need to adjust
> > > + * the address to get LEP (Local Entry Point).
> > > + */
> > > +#define LEP_OFFSET 8
> > > +
> > > +static ssize_t get_offset(ssize_t offset)
> >
> > if we mark this function __weak global, would it work as is? Would it
> > get an address of a global entry point? I know nothing about this GEP
> > vs LEP stuff, interesting :)
>
> you mean get_base_addr? it's already global
>
> all the calls to get_base_addr within the object are made
> to get_base_addr+0x8
>
> 00000000100350c0 <test_attach_probe>:
>     ...
>     100350e0:   59 fd ff 4b     bl      10034e38 <get_base_addr+0x8>
>     ...
>     100358a8:   91 f5 ff 4b     bl      10034e38 <get_base_addr+0x8>
>
>
> I'm following perf fix we had for similar issue:
>   7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms lookup
>
> I'll get more info on that

My thinking was that if you mark the function as __weak, then the
compiler is not allowed to assume that the actual implementation of
that function will come from the same object (because it might be
replaced by the linker later), so it has to be pessimistic and use
global entry, no? Totally theoritizing here, of course.

>
> jirka
>
> >
> > > +{
> > > +       return offset + LEP_OFFSET;
> > > +}
> > > +#else
> > > +#define get_offset(offset) (offset)
> > > +#endif
> > > +
> > >  ssize_t get_base_addr() {
> > >         size_t start, offset;
> > >         char buf[256];
> > > @@ -36,7 +52,7 @@ void test_attach_probe(void)
> > >         if (CHECK(base_addr < 0, "get_base_addr",
> > >                   "failed to find base addr: %zd", base_addr))
> > >                 return;
> > > -       uprobe_offset = (size_t)&get_base_addr - base_addr;
> > > +       uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
> > >
> > >         skel = test_attach_probe__open_and_load();
> > >         if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> > > --
> > > 2.29.2
> > >
> >
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-02 18:31     ` Andrii Nakryiko
@ 2021-03-02 18:58       ` Jiri Olsa
  0 siblings, 0 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-03-02 18:58 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Networking, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen,
	Yauheni Kaliuta

On Tue, Mar 02, 2021 at 10:31:41AM -0800, Andrii Nakryiko wrote:
> On Tue, Mar 2, 2021 at 3:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Mon, Mar 01, 2021 at 04:34:24PM -0800, Andrii Nakryiko wrote:
> > > On Mon, Mar 1, 2021 at 11:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > When testing uprobes we the test gets GEP (Global Entry Point)
> > > > address from kallsyms, but then the function is called locally
> > > > so the uprobe is not triggered.
> > > >
> > > > Fixing this by adjusting the address to LEP (Local Entry Point)
> > > > for powerpc arch.
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> > > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > > @@ -2,6 +2,22 @@
> > > >  #include <test_progs.h>
> > > >  #include "test_attach_probe.skel.h"
> > > >
> > > > +#if defined(__powerpc64__)
> > > > +/*
> > > > + * We get the GEP (Global Entry Point) address from kallsyms,
> > > > + * but then the function is called locally, so we need to adjust
> > > > + * the address to get LEP (Local Entry Point).
> > > > + */
> > > > +#define LEP_OFFSET 8
> > > > +
> > > > +static ssize_t get_offset(ssize_t offset)
> > >
> > > if we mark this function __weak global, would it work as is? Would it
> > > get an address of a global entry point? I know nothing about this GEP
> > > vs LEP stuff, interesting :)
> >
> > you mean get_base_addr? it's already global
> >
> > all the calls to get_base_addr within the object are made
> > to get_base_addr+0x8
> >
> > 00000000100350c0 <test_attach_probe>:
> >     ...
> >     100350e0:   59 fd ff 4b     bl      10034e38 <get_base_addr+0x8>
> >     ...
> >     100358a8:   91 f5 ff 4b     bl      10034e38 <get_base_addr+0x8>
> >
> >
> > I'm following perf fix we had for similar issue:
> >   7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms lookup
> >
> > I'll get more info on that
> 
> My thinking was that if you mark the function as __weak, then the
> compiler is not allowed to assume that the actual implementation of
> that function will come from the same object (because it might be
> replaced by the linker later), so it has to be pessimistic and use
> global entry, no? Totally theoritizing here, of course.

ah ok.. good idea, but it's still jumping to +8 in my test

    # nm test_progs | grep get_base_addr
    0000000010034e30 W get_base_addr

    100350e0:   59 fd ff 4b     bl      10034e38 <get_base_addr+0x8>

looks like it's linker, because compiler leaves just jump to next instruction

jirka


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-02 10:35   ` Jiri Olsa
@ 2021-03-03  6:40     ` Naveen N. Rao
  2021-03-03 22:49       ` Jiri Olsa
  2021-03-04  0:46       ` Michael Ellerman
  0 siblings, 2 replies; 15+ messages in thread
From: Naveen N. Rao @ 2021-03-03  6:40 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Yonghong Song, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, netdev, bpf, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen,
	Yauheni Kaliuta, Michael Ellerman, Srikar Dronamraju,
	Paul Mackerras

On 2021/03/02 11:35AM, Jiri Olsa wrote:
> On Mon, Mar 01, 2021 at 02:58:53PM -0800, Yonghong Song wrote:
> > 
> > 
> > On 3/1/21 11:04 AM, Jiri Olsa wrote:
> > > When testing uprobes we the test gets GEP (Global Entry Point)
> > > address from kallsyms, but then the function is called locally
> > > so the uprobe is not triggered.
> > > 
> > > Fixing this by adjusting the address to LEP (Local Entry Point)
> > > for powerpc arch.
> > > 
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >   .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> > >   1 file changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > @@ -2,6 +2,22 @@
> > >   #include <test_progs.h>
> > >   #include "test_attach_probe.skel.h"
> > > +#if defined(__powerpc64__)

This needs to be specific to ELF v2 ABI, so you'll need to check 
_CALL_ELF. See commit d5c2e2c17ae1d6 ("perf probe ppc64le: Prefer symbol 
table lookup over DWARF") for an example.

> > > +/*
> > > + * We get the GEP (Global Entry Point) address from kallsyms,
> > > + * but then the function is called locally, so we need to adjust
> > > + * the address to get LEP (Local Entry Point).
> > 
> > Any documentation in the kernel about this behavior? This will
> > help to validate the change without trying with powerpc64 qemu...

I don't think we have documented this in the kernel anywhere, but this 
is specific to the ELF v2 ABI and is described there:
- 2.3.2.1.  Function Prologue: 
  http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655240___RefHeading___Toc377640597.html
- 3.4.1.  Symbol Values:
   http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655241_95185.html

> 
> we got similar fix in perf:
> 
> 7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms lookup
> 
> CC-ing few other folks from ppc land for more info

Thanks.

> > 
> > > + */
> > > +#define LEP_OFFSET 8
> > > +
> > > +static ssize_t get_offset(ssize_t offset)
> > > +{
> > > +	return offset + LEP_OFFSET;
> > > +}
> > > +#else
> > > +#define get_offset(offset) (offset)
> > > +#endif
> > > +
> > >   ssize_t get_base_addr() {
> > >   	size_t start, offset;
> > >   	char buf[256];
> > > @@ -36,7 +52,7 @@ void test_attach_probe(void)
> > >   	if (CHECK(base_addr < 0, "get_base_addr",
> > >   		  "failed to find base addr: %zd", base_addr))
> > >   		return;
> > > -	uprobe_offset = (size_t)&get_base_addr - base_addr;
> > > +	uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
> > >   	skel = test_attach_probe__open_and_load();
> > >   	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> > > 

As documented in the links above, the right way to identify local entry 
point (LEP) is by looking at the symbol table. Falling back to using a 
hardcoded offset of 8 is a reasonable workaround if we don't have access 
to the symbol table.

- Naveen


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-02 17:19     ` Yonghong Song
@ 2021-03-03  6:42       ` Naveen N. Rao
  0 siblings, 0 replies; 15+ messages in thread
From: Naveen N. Rao @ 2021-03-03  6:42 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Jiri Olsa, Andrii Nakryiko, Jiri Olsa, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Networking, bpf,
	Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
	Toke Høiland-Jørgensen, Yauheni Kaliuta

On 2021/03/02 09:19AM, Yonghong Song wrote:
> 
> 
> On 3/2/21 3:14 AM, Jiri Olsa wrote:
> > On Mon, Mar 01, 2021 at 04:34:24PM -0800, Andrii Nakryiko wrote:
> > > On Mon, Mar 1, 2021 at 11:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > 
> > > > When testing uprobes we the test gets GEP (Global Entry Point)
> > > > address from kallsyms, but then the function is called locally
> > > > so the uprobe is not triggered.
> > > > 
> > > > Fixing this by adjusting the address to LEP (Local Entry Point)
> > > > for powerpc arch.
> > > > 
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >   .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> > > >   1 file changed, 17 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > > @@ -2,6 +2,22 @@
> > > >   #include <test_progs.h>
> > > >   #include "test_attach_probe.skel.h"
> > > > 
> > > > +#if defined(__powerpc64__)
> > > > +/*
> > > > + * We get the GEP (Global Entry Point) address from kallsyms,
> > > > + * but then the function is called locally, so we need to adjust
> > > > + * the address to get LEP (Local Entry Point).
> > > > + */
> > > > +#define LEP_OFFSET 8
> > > > +
> > > > +static ssize_t get_offset(ssize_t offset)
> > > 
> > > if we mark this function __weak global, would it work as is? Would it
> > > get an address of a global entry point? I know nothing about this GEP
> > > vs LEP stuff, interesting :)
> > 
> > you mean get_base_addr? it's already global
> > 
> > all the calls to get_base_addr within the object are made
> > to get_base_addr+0x8
> > 
> > 00000000100350c0 <test_attach_probe>:
> >      ...
> >      100350e0:   59 fd ff 4b     bl      10034e38 <get_base_addr+0x8>
> >      ...
> >      100358a8:   91 f5 ff 4b     bl      10034e38 <get_base_addr+0x8>
> > 
> > 
> > I'm following perf fix we had for similar issue:
> >    7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms lookup
> > 
> > I'll get more info on that
> 
> Thanks. The patch
>    7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms
> lookup
> talked about offset + 8 for kernel symbols.
> I guess uprobe symbol might be in the same situation if using the
> same compilation mechanism as kernel. But it would be good
> to get confirmation from ppc people.

Yes, this is part of the ELF V2 ABI, so it applies to both kernel and 
userspace.

- Naveen


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-03  6:40     ` Naveen N. Rao
@ 2021-03-03 22:49       ` Jiri Olsa
  2021-03-04  0:46       ` Michael Ellerman
  1 sibling, 0 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-03-03 22:49 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Yonghong Song, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, netdev, bpf, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen,
	Yauheni Kaliuta, Michael Ellerman, Srikar Dronamraju,
	Paul Mackerras

On Wed, Mar 03, 2021 at 12:10:43PM +0530, Naveen N. Rao wrote:
> On 2021/03/02 11:35AM, Jiri Olsa wrote:
> > On Mon, Mar 01, 2021 at 02:58:53PM -0800, Yonghong Song wrote:
> > > 
> > > 
> > > On 3/1/21 11:04 AM, Jiri Olsa wrote:
> > > > When testing uprobes we the test gets GEP (Global Entry Point)
> > > > address from kallsyms, but then the function is called locally
> > > > so the uprobe is not triggered.
> > > > 
> > > > Fixing this by adjusting the address to LEP (Local Entry Point)
> > > > for powerpc arch.
> > > > 
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >   .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> > > >   1 file changed, 17 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > > @@ -2,6 +2,22 @@
> > > >   #include <test_progs.h>
> > > >   #include "test_attach_probe.skel.h"
> > > > +#if defined(__powerpc64__)
> 
> This needs to be specific to ELF v2 ABI, so you'll need to check 
> _CALL_ELF. See commit d5c2e2c17ae1d6 ("perf probe ppc64le: Prefer symbol 
> table lookup over DWARF") for an example.

i see, there's the outer #if _CALL_ELF I missed

> 
> > > > +/*
> > > > + * We get the GEP (Global Entry Point) address from kallsyms,
> > > > + * but then the function is called locally, so we need to adjust
> > > > + * the address to get LEP (Local Entry Point).
> > > 
> > > Any documentation in the kernel about this behavior? This will
> > > help to validate the change without trying with powerpc64 qemu...
> 
> I don't think we have documented this in the kernel anywhere, but this 
> is specific to the ELF v2 ABI and is described there:
> - 2.3.2.1.  Function Prologue: 
>   http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655240___RefHeading___Toc377640597.html
> - 3.4.1.  Symbol Values:
>    http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655241_95185.html
> 
> > 
> > we got similar fix in perf:
> > 
> > 7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms lookup
> > 
> > CC-ing few other folks from ppc land for more info
> 
> Thanks.
> 
> > > 
> > > > + */
> > > > +#define LEP_OFFSET 8
> > > > +
> > > > +static ssize_t get_offset(ssize_t offset)
> > > > +{
> > > > +	return offset + LEP_OFFSET;
> > > > +}
> > > > +#else
> > > > +#define get_offset(offset) (offset)
> > > > +#endif
> > > > +
> > > >   ssize_t get_base_addr() {
> > > >   	size_t start, offset;
> > > >   	char buf[256];
> > > > @@ -36,7 +52,7 @@ void test_attach_probe(void)
> > > >   	if (CHECK(base_addr < 0, "get_base_addr",
> > > >   		  "failed to find base addr: %zd", base_addr))
> > > >   		return;
> > > > -	uprobe_offset = (size_t)&get_base_addr - base_addr;
> > > > +	uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
> > > >   	skel = test_attach_probe__open_and_load();
> > > >   	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> > > > 
> 
> As documented in the links above, the right way to identify local entry 
> point (LEP) is by looking at the symbol table. Falling back to using a 
> hardcoded offset of 8 is a reasonable workaround if we don't have access 
> to the symbol table.

thanks for the all the info, I'll send v2 with _CALL_ELF check

jirka


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-03  6:40     ` Naveen N. Rao
  2021-03-03 22:49       ` Jiri Olsa
@ 2021-03-04  0:46       ` Michael Ellerman
  2021-03-04 15:55         ` Jiri Olsa
  1 sibling, 1 reply; 15+ messages in thread
From: Michael Ellerman @ 2021-03-04  0:46 UTC (permalink / raw)
  To: Naveen N. Rao, Jiri Olsa
  Cc: Yonghong Song, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, netdev, bpf, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen,
	Yauheni Kaliuta, Srikar Dronamraju, Paul Mackerras

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> On 2021/03/02 11:35AM, Jiri Olsa wrote:
>> On Mon, Mar 01, 2021 at 02:58:53PM -0800, Yonghong Song wrote:
>> > 
>> > 
>> > On 3/1/21 11:04 AM, Jiri Olsa wrote:
>> > > When testing uprobes we the test gets GEP (Global Entry Point)
>> > > address from kallsyms, but then the function is called locally
>> > > so the uprobe is not triggered.
>> > > 
>> > > Fixing this by adjusting the address to LEP (Local Entry Point)
>> > > for powerpc arch.
>> > > 
>> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> > > ---
>> > >   .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
>> > >   1 file changed, 17 insertions(+), 1 deletion(-)
>> > > 
>> > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
>> > > index a0ee87c8e1ea..c3cfb48d3ed0 100644
>> > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
>> > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
>> > > @@ -2,6 +2,22 @@
>> > >   #include <test_progs.h>
>> > >   #include "test_attach_probe.skel.h"
>> > > +#if defined(__powerpc64__)
>
> This needs to be specific to ELF v2 ABI, so you'll need to check 
> _CALL_ELF. See commit d5c2e2c17ae1d6 ("perf probe ppc64le: Prefer symbol 
> table lookup over DWARF") for an example.
>
>> > > +/*
>> > > + * We get the GEP (Global Entry Point) address from kallsyms,
>> > > + * but then the function is called locally, so we need to adjust
>> > > + * the address to get LEP (Local Entry Point).
>> > 
>> > Any documentation in the kernel about this behavior? This will
>> > help to validate the change without trying with powerpc64 qemu...
>
> I don't think we have documented this in the kernel anywhere, but this 
> is specific to the ELF v2 ABI and is described there:
> - 2.3.2.1.  Function Prologue: 
>   http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655240___RefHeading___Toc377640597.html
> - 3.4.1.  Symbol Values:
>    http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655241_95185.html

There's a comment in ppc_function_entry(), but I don't think we have any
actual "documentation".

static inline unsigned long ppc_function_entry(void *func)
{
#ifdef PPC64_ELF_ABI_v2
	u32 *insn = func;

	/*
	 * A PPC64 ABIv2 function may have a local and a global entry
	 * point. We need to use the local entry point when patching
	 * functions, so identify and step over the global entry point
	 * sequence.
	 *
	 * The global entry point sequence is always of the form:
	 *
	 * addis r2,r12,XXXX
	 * addi  r2,r2,XXXX
	 *
	 * A linker optimisation may convert the addis to lis:
	 *
	 * lis   r2,XXXX
	 * addi  r2,r2,XXXX
	 */
	if ((((*insn & OP_RT_RA_MASK) == ADDIS_R2_R12) ||
	     ((*insn & OP_RT_RA_MASK) == LIS_R2)) &&
	    ((*(insn+1) & OP_RT_RA_MASK) == ADDI_R2_R2))
		return (unsigned long)(insn + 2);
	else
		return (unsigned long)func;


cheers

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-04 15:55         ` Jiri Olsa
@ 2021-03-04  1:34           ` Naveen N. Rao
  2021-03-05 13:38             ` Jiri Olsa
  0 siblings, 1 reply; 15+ messages in thread
From: Naveen N. Rao @ 2021-03-04  1:34 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Michael Ellerman, Yonghong Song, Jiri Olsa, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, netdev, bpf, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh,
	Toke Høiland-Jørgensen, Yauheni Kaliuta,
	Srikar Dronamraju, Paul Mackerras

On 2021/03/04 04:55PM, Jiri Olsa wrote:
> On Thu, Mar 04, 2021 at 11:46:27AM +1100, Michael Ellerman wrote:
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> > > On 2021/03/02 11:35AM, Jiri Olsa wrote:
> > >> On Mon, Mar 01, 2021 at 02:58:53PM -0800, Yonghong Song wrote:
> > >> > 
> > >> > 
> > >> > On 3/1/21 11:04 AM, Jiri Olsa wrote:
> > >> > > When testing uprobes we the test gets GEP (Global Entry Point)
> > >> > > address from kallsyms, but then the function is called locally
> > >> > > so the uprobe is not triggered.
> > >> > > 
> > >> > > Fixing this by adjusting the address to LEP (Local Entry Point)
> > >> > > for powerpc arch.
> > >> > > 
> > >> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > >> > > ---
> > >> > >   .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> > >> > >   1 file changed, 17 insertions(+), 1 deletion(-)
> > >> > > 
> > >> > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > >> > > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> > >> > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > >> > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > >> > > @@ -2,6 +2,22 @@
> > >> > >   #include <test_progs.h>
> > >> > >   #include "test_attach_probe.skel.h"
> > >> > > +#if defined(__powerpc64__)
> > >
> > > This needs to be specific to ELF v2 ABI, so you'll need to check 
> > > _CALL_ELF. See commit d5c2e2c17ae1d6 ("perf probe ppc64le: Prefer symbol 
> > > table lookup over DWARF") for an example.
> > >
> > >> > > +/*
> > >> > > + * We get the GEP (Global Entry Point) address from kallsyms,
> > >> > > + * but then the function is called locally, so we need to adjust
> > >> > > + * the address to get LEP (Local Entry Point).
> > >> > 
> > >> > Any documentation in the kernel about this behavior? This will
> > >> > help to validate the change without trying with powerpc64 qemu...
> > >
> > > I don't think we have documented this in the kernel anywhere, but this 
> > > is specific to the ELF v2 ABI and is described there:
> > > - 2.3.2.1.  Function Prologue: 
> > >   http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655240___RefHeading___Toc377640597.html
> > > - 3.4.1.  Symbol Values:
> > >    http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655241_95185.html
> > 
> > There's a comment in ppc_function_entry(), but I don't think we have any
> > actual "documentation".
> > 
> > static inline unsigned long ppc_function_entry(void *func)
> > {
> > #ifdef PPC64_ELF_ABI_v2
> > 	u32 *insn = func;
> > 
> > 	/*
> > 	 * A PPC64 ABIv2 function may have a local and a global entry
> > 	 * point. We need to use the local entry point when patching
> > 	 * functions, so identify and step over the global entry point
> > 	 * sequence.
> 
> hm, so I need to do the instructions check below as well

It's a good check, but probably not necessary. In most functions, we 
expect to be able to probe two instructions later without much of a 
change to affect function tracing for userspace. For this reason, we 
just probe at an offset of 8 as a reasonable fallback.

It is definetely good if we can come up with a better approach though.

> 
> > 	 *
> > 	 * The global entry point sequence is always of the form:
> > 	 *
> > 	 * addis r2,r12,XXXX
> > 	 * addi  r2,r2,XXXX
> > 	 *
> > 	 * A linker optimisation may convert the addis to lis:
> > 	 *
> > 	 * lis   r2,XXXX
> > 	 * addi  r2,r2,XXXX
> > 	 */
> > 	if ((((*insn & OP_RT_RA_MASK) == ADDIS_R2_R12) ||
> > 	     ((*insn & OP_RT_RA_MASK) == LIS_R2)) &&
> > 	    ((*(insn+1) & OP_RT_RA_MASK) == ADDI_R2_R2))
> 
> is this check/instructions specific to kernel code?
> 
> In the test prog I see following instructions:
> 
> Dump of assembler code for function get_base_addr:
>    0x0000000010034cb0 <+0>:     lis     r2,4256
>    0x0000000010034cb4 <+4>:     addi    r2,r2,31488
>    ...
> 
> but first instruction does not match the check in kernel code above:
> 
> 	1.insn value:	0x3c4010a0
> 	2.insn value:	0x38427b00
> 
> the used defines are:
> 	#define OP_RT_RA_MASK   0xffff0000UL
> 	#define LIS_R2          0x3c020000UL
> 	#define ADDIS_R2_R12    0x3c4c0000UL
> 	#define ADDI_R2_R2      0x38420000UL

Good catch! That's wrong, and I suspect we haven't noticed since kernel 
almost always ends up using the addis variant. I will send a fix for 
this.

> 
> 
> maybe we could skip the check, and run the test twice: first on
> kallsym address and if the uprobe is not hit we will run it again
> on address + 8

Sure, like I mentioned, I'm fine with any approach. Offset'ing into the 
function by 8 is easy and generally works. Re-trying is fine too. The 
proper approach will requires us to consult the symbol table and check 
st_other field [see commit 0b3c2264ae30ed ("perf symbols: Fix kallsyms 
perf test on ppc64le")]

Thanks,
- Naveen

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-04  0:46       ` Michael Ellerman
@ 2021-03-04 15:55         ` Jiri Olsa
  2021-03-04  1:34           ` Naveen N. Rao
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2021-03-04 15:55 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Naveen N. Rao, Yonghong Song, Jiri Olsa, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, netdev, bpf, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh,
	Toke Høiland-Jørgensen, Yauheni Kaliuta,
	Srikar Dronamraju, Paul Mackerras

On Thu, Mar 04, 2021 at 11:46:27AM +1100, Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> > On 2021/03/02 11:35AM, Jiri Olsa wrote:
> >> On Mon, Mar 01, 2021 at 02:58:53PM -0800, Yonghong Song wrote:
> >> > 
> >> > 
> >> > On 3/1/21 11:04 AM, Jiri Olsa wrote:
> >> > > When testing uprobes we the test gets GEP (Global Entry Point)
> >> > > address from kallsyms, but then the function is called locally
> >> > > so the uprobe is not triggered.
> >> > > 
> >> > > Fixing this by adjusting the address to LEP (Local Entry Point)
> >> > > for powerpc arch.
> >> > > 
> >> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >> > > ---
> >> > >   .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> >> > >   1 file changed, 17 insertions(+), 1 deletion(-)
> >> > > 
> >> > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> >> > > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> >> > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> >> > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> >> > > @@ -2,6 +2,22 @@
> >> > >   #include <test_progs.h>
> >> > >   #include "test_attach_probe.skel.h"
> >> > > +#if defined(__powerpc64__)
> >
> > This needs to be specific to ELF v2 ABI, so you'll need to check 
> > _CALL_ELF. See commit d5c2e2c17ae1d6 ("perf probe ppc64le: Prefer symbol 
> > table lookup over DWARF") for an example.
> >
> >> > > +/*
> >> > > + * We get the GEP (Global Entry Point) address from kallsyms,
> >> > > + * but then the function is called locally, so we need to adjust
> >> > > + * the address to get LEP (Local Entry Point).
> >> > 
> >> > Any documentation in the kernel about this behavior? This will
> >> > help to validate the change without trying with powerpc64 qemu...
> >
> > I don't think we have documented this in the kernel anywhere, but this 
> > is specific to the ELF v2 ABI and is described there:
> > - 2.3.2.1.  Function Prologue: 
> >   http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655240___RefHeading___Toc377640597.html
> > - 3.4.1.  Symbol Values:
> >    http://cdn.openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655241_95185.html
> 
> There's a comment in ppc_function_entry(), but I don't think we have any
> actual "documentation".
> 
> static inline unsigned long ppc_function_entry(void *func)
> {
> #ifdef PPC64_ELF_ABI_v2
> 	u32 *insn = func;
> 
> 	/*
> 	 * A PPC64 ABIv2 function may have a local and a global entry
> 	 * point. We need to use the local entry point when patching
> 	 * functions, so identify and step over the global entry point
> 	 * sequence.

hm, so I need to do the instructions check below as well

> 	 *
> 	 * The global entry point sequence is always of the form:
> 	 *
> 	 * addis r2,r12,XXXX
> 	 * addi  r2,r2,XXXX
> 	 *
> 	 * A linker optimisation may convert the addis to lis:
> 	 *
> 	 * lis   r2,XXXX
> 	 * addi  r2,r2,XXXX
> 	 */
> 	if ((((*insn & OP_RT_RA_MASK) == ADDIS_R2_R12) ||
> 	     ((*insn & OP_RT_RA_MASK) == LIS_R2)) &&
> 	    ((*(insn+1) & OP_RT_RA_MASK) == ADDI_R2_R2))

is this check/instructions specific to kernel code?

In the test prog I see following instructions:

Dump of assembler code for function get_base_addr:
   0x0000000010034cb0 <+0>:     lis     r2,4256
   0x0000000010034cb4 <+4>:     addi    r2,r2,31488
   ...

but first instruction does not match the check in kernel code above:

	1.insn value:	0x3c4010a0
	2.insn value:	0x38427b00

the used defines are:
	#define OP_RT_RA_MASK   0xffff0000UL
	#define LIS_R2          0x3c020000UL
	#define ADDIS_R2_R12    0x3c4c0000UL
	#define ADDI_R2_R2      0x38420000UL


maybe we could skip the check, and run the test twice: first on
kallsym address and if the uprobe is not hit we will run it again
on address + 8

thanks,
jirka

> 		return (unsigned long)(insn + 2);
> 	else
> 		return (unsigned long)func;
> 
> 
> cheers
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
  2021-03-04  1:34           ` Naveen N. Rao
@ 2021-03-05 13:38             ` Jiri Olsa
  0 siblings, 0 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-03-05 13:38 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Michael Ellerman, Yonghong Song, Jiri Olsa, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, netdev, bpf, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh,
	Toke Høiland-Jørgensen, Yauheni Kaliuta,
	Srikar Dronamraju, Paul Mackerras

On Thu, Mar 04, 2021 at 07:04:59AM +0530, Naveen N. Rao wrote:

SNIP

> > > static inline unsigned long ppc_function_entry(void *func)
> > > {
> > > #ifdef PPC64_ELF_ABI_v2
> > > 	u32 *insn = func;
> > > 
> > > 	/*
> > > 	 * A PPC64 ABIv2 function may have a local and a global entry
> > > 	 * point. We need to use the local entry point when patching
> > > 	 * functions, so identify and step over the global entry point
> > > 	 * sequence.
> > 
> > hm, so I need to do the instructions check below as well
> 
> It's a good check, but probably not necessary. In most functions, we 
> expect to be able to probe two instructions later without much of a 
> change to affect function tracing for userspace. For this reason, we 
> just probe at an offset of 8 as a reasonable fallback.
> 
> It is definetely good if we can come up with a better approach though.
> 
> > 
> > > 	 *
> > > 	 * The global entry point sequence is always of the form:
> > > 	 *
> > > 	 * addis r2,r12,XXXX
> > > 	 * addi  r2,r2,XXXX
> > > 	 *
> > > 	 * A linker optimisation may convert the addis to lis:
> > > 	 *
> > > 	 * lis   r2,XXXX
> > > 	 * addi  r2,r2,XXXX
> > > 	 */
> > > 	if ((((*insn & OP_RT_RA_MASK) == ADDIS_R2_R12) ||
> > > 	     ((*insn & OP_RT_RA_MASK) == LIS_R2)) &&
> > > 	    ((*(insn+1) & OP_RT_RA_MASK) == ADDI_R2_R2))
> > 
> > is this check/instructions specific to kernel code?
> > 
> > In the test prog I see following instructions:
> > 
> > Dump of assembler code for function get_base_addr:
> >    0x0000000010034cb0 <+0>:     lis     r2,4256
> >    0x0000000010034cb4 <+4>:     addi    r2,r2,31488
> >    ...
> > 
> > but first instruction does not match the check in kernel code above:
> > 
> > 	1.insn value:	0x3c4010a0
> > 	2.insn value:	0x38427b00
> > 
> > the used defines are:
> > 	#define OP_RT_RA_MASK   0xffff0000UL
> > 	#define LIS_R2          0x3c020000UL
> > 	#define ADDIS_R2_R12    0x3c4c0000UL
> > 	#define ADDI_R2_R2      0x38420000UL
> 
> Good catch! That's wrong, and I suspect we haven't noticed since kernel 
> almost always ends up using the addis variant. I will send a fix for 
> this.

the new macro value from your fix works for the test,
so I'll use it in v2, so we don't just blindly go to
+8 offset.. I'll send it out shortly

> 
> > 
> > 
> > maybe we could skip the check, and run the test twice: first on
> > kallsym address and if the uprobe is not hit we will run it again
> > on address + 8
> 
> Sure, like I mentioned, I'm fine with any approach. Offset'ing into the 
> function by 8 is easy and generally works. Re-trying is fine too. The 
> proper approach will requires us to consult the symbol table and check 
> st_other field [see commit 0b3c2264ae30ed ("perf symbols: Fix kallsyms 
> perf test on ppc64le")]

I think we don't want to complicate this test with symbol table
check. I'll propose the fix with the extra instructions check
for now and we can add symbol table check in future if it's not
enough

thanks for all the info,
jirka

> 
> Thanks,
> - Naveen
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-01 19:04 [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes Jiri Olsa
2021-03-01 22:58 ` Yonghong Song
2021-03-02 10:35   ` Jiri Olsa
2021-03-03  6:40     ` Naveen N. Rao
2021-03-03 22:49       ` Jiri Olsa
2021-03-04  0:46       ` Michael Ellerman
2021-03-04 15:55         ` Jiri Olsa
2021-03-04  1:34           ` Naveen N. Rao
2021-03-05 13:38             ` Jiri Olsa
2021-03-02  0:34 ` Andrii Nakryiko
2021-03-02 11:14   ` Jiri Olsa
2021-03-02 17:19     ` Yonghong Song
2021-03-03  6:42       ` Naveen N. Rao
2021-03-02 18:31     ` Andrii Nakryiko
2021-03-02 18:58       ` Jiri Olsa

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git