BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Jiri Olsa <jolsa@redhat.com>
To: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Cc: "Michael Ellerman" <mpe@ellerman.id.au>,
	"Yonghong Song" <yhs@fb.com>, "Jiri Olsa" <jolsa@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Andrii Nakryiko" <andriin@fb.com>,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"KP Singh" <kpsingh@chromium.org>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Yauheni Kaliuta" <ykaliuta@redhat.com>,
	"Srikar Dronamraju" <srikar@linux.vnet.ibm.com>,
	"Paul Mackerras" <paulus@samba.org>
Subject: Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes
Date: Fri, 5 Mar 2021 14:38:58 +0100
Message-ID: <YEI0cptuDzUUOaLr@krava> (raw)
In-Reply-To: <20210304013459.GG1913@DESKTOP-TDPLP67.localdomain>

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
> 


  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-01 19:04 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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YEI0cptuDzUUOaLr@krava \
    --to=jolsa@redhat.com \
    --cc=andriin@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kafai@fb.com \
    --cc=kpsingh@chromium.org \
    --cc=mpe@ellerman.id.au \
    --cc=naveen.n.rao@linux.vnet.ibm.com \
    --cc=netdev@vger.kernel.org \
    --cc=paulus@samba.org \
    --cc=songliubraving@fb.com \
    --cc=srikar@linux.vnet.ibm.com \
    --cc=toke@redhat.com \
    --cc=yhs@fb.com \
    --cc=ykaliuta@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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