All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Nicholas Piggin <npiggin@gmail.com>
Cc: "Andrii Nakryiko" <andrii@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Hao Luo" <haoluo@google.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Jiri Olsa" <jolsa@kernel.org>, "KP Singh" <kpsingh@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"Martin KaFai Lau" <martin.lau@linux.dev>,
	"Stanislav Fomichev" <sdf@google.com>,
	"Song Liu" <song@kernel.org>,
	"Yonghong\
	 Song" <yhs@fb.com>
Subject: Re: [PATCH] powerpc/bpf: Only update ldimm64 during extra pass when it is an address
Date: Thu, 24 Nov 2022 19:19:29 +0530	[thread overview]
Message-ID: <1669297066.kxu8xl391n.naveen@linux.ibm.com> (raw)
In-Reply-To: <9f17237f-94da-f58f-4f4b-0068851b4123@csgroup.eu>

Christophe Leroy wrote:
> 
> 
> Le 24/11/2022 à 11:13, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> ldimm64 is not only used for loading function addresses, and
>> 
>> That's probably true today, but I worry that that can change upstream 
>> and we may not notice at all.
> 
> Not sure what you mean.
> 
> Today POWERPC considers that ldimm64 is _always_ loading a function 
> address whereas upstream BPF considers that ldimm64 is a function only 
> when it is flagged BPF_PSEUDO_FUNC.

Not sure why you think we consider ldimm64 to always be loading a 
function address. Perhaps it is due to the poorly chosen variable name 
func_addr in bpf_jit_fixup_addresses(), or due to the fact that we 
always update the JIT code for ldimm64. In any case, we simply overwrite 
imm64 load instructions to ensure we are using the updated address.

> 
> In what direction could that change in the future ?
> 
> For me if they change that it becomes an API change.

More of an extension, which is exactly what we had when BPF_PSEUDO_FUNC 
was introduced. Took us nearly a year before we noticed.

Because we do not do a full JIT during the extra pass today like other 
architectures, we are the exception - there is always the risk of bpf 
core changes breaking our JIT. So, I still think it is better if we do a 
full JIT during extra pass.


- Naveen


WARNING: multiple messages have this Message-ID (diff)
From: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Nicholas Piggin <npiggin@gmail.com>
Cc: "Hao Luo" <haoluo@google.com>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Martin KaFai Lau" <martin.lau@linux.dev>,
	"Song Liu" <song@kernel.org>,
	"Stanislav Fomichev" <sdf@google.com>,
	"Jiri Olsa" <jolsa@kernel.org>, "KP Singh" <kpsingh@kernel.org>,
	"Yonghong\
	 Song" <yhs@fb.com>, "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH] powerpc/bpf: Only update ldimm64 during extra pass when it is an address
Date: Thu, 24 Nov 2022 19:19:29 +0530	[thread overview]
Message-ID: <1669297066.kxu8xl391n.naveen@linux.ibm.com> (raw)
In-Reply-To: <9f17237f-94da-f58f-4f4b-0068851b4123@csgroup.eu>

Christophe Leroy wrote:
> 
> 
> Le 24/11/2022 à 11:13, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> ldimm64 is not only used for loading function addresses, and
>> 
>> That's probably true today, but I worry that that can change upstream 
>> and we may not notice at all.
> 
> Not sure what you mean.
> 
> Today POWERPC considers that ldimm64 is _always_ loading a function 
> address whereas upstream BPF considers that ldimm64 is a function only 
> when it is flagged BPF_PSEUDO_FUNC.

Not sure why you think we consider ldimm64 to always be loading a 
function address. Perhaps it is due to the poorly chosen variable name 
func_addr in bpf_jit_fixup_addresses(), or due to the fact that we 
always update the JIT code for ldimm64. In any case, we simply overwrite 
imm64 load instructions to ensure we are using the updated address.

> 
> In what direction could that change in the future ?
> 
> For me if they change that it becomes an API change.

More of an extension, which is exactly what we had when BPF_PSEUDO_FUNC 
was introduced. Took us nearly a year before we noticed.

Because we do not do a full JIT during the extra pass today like other 
architectures, we are the exception - there is always the risk of bpf 
core changes breaking our JIT. So, I still think it is better if we do a 
full JIT during extra pass.


- Naveen


  reply	other threads:[~2022-11-24 13:50 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-24  8:39 [PATCH] powerpc/bpf: Only update ldimm64 during extra pass when it is an address Christophe Leroy
2022-11-24  8:39 ` Christophe Leroy
2022-11-24 10:13 ` Naveen N. Rao
2022-11-24 10:13   ` Naveen N. Rao
2022-11-24 12:24   ` Christophe Leroy
2022-11-24 12:24     ` Christophe Leroy
2022-11-24 13:49     ` Naveen N. Rao [this message]
2022-11-24 13:49       ` Naveen N. Rao
2022-11-24 19:46       ` Christophe Leroy
2022-11-24 19:46         ` Christophe Leroy
2022-11-25  5:38         ` Naveen N. Rao
2022-11-25  5:38           ` Naveen N. Rao
2022-11-25  5:59           ` Christophe Leroy
2022-11-25  5:59             ` Christophe Leroy

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=1669297066.kxu8xl391n.naveen@linux.ibm.com \
    --to=naveen.n.rao@linux.ibm.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=christophe.leroy@csgroup.eu \
    --cc=daniel@iogearbox.net \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=martin.lau@linux.dev \
    --cc=mpe@ellerman.id.au \
    --cc=npiggin@gmail.com \
    --cc=sdf@google.com \
    --cc=song@kernel.org \
    --cc=yhs@fb.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.