From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Andrii Nakryiko <andriin@fb.com>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org, ast@fb.com,
daniel@iogearbox.net, andrii.nakryiko@gmail.com,
kernel-team@fb.com, Luka Perkov <luka.perkov@sartura.hr>,
Tony Ambardar <tony.ambardar@gmail.com>
Subject: Re: [PATCH bpf-next 1/3] libbpf: support safe subset of load/store instruction resizing with CO-RE
Date: Tue, 6 Oct 2020 11:08:39 -0700 [thread overview]
Message-ID: <20201006180839.lvoigzpmr32rrroj@ast-mbp> (raw)
In-Reply-To: <20201002010633.3706122-2-andriin@fb.com>
On Thu, Oct 01, 2020 at 06:06:31PM -0700, Andrii Nakryiko wrote:
> Add support for patching instructions of the following form:
> - rX = *(T *)(rY + <off>);
> - *(T *)(rX + <off>) = rY;
> - *(T *)(rX + <off>) = <imm>, where T is one of {u8, u16, u32, u64}.
llvm doesn't generate ST instruction. It never did.
STX is generated, but can it actually be used with relocations?
Looking at the test in patch 3... it's testing LDX only.
ST/STX suppose to work by analogy, but would be good to have a test.
At least of STX.
> +static int insn_mem_sz_to_bytes(struct bpf_insn *insn)
> +{
> + switch (BPF_SIZE(insn->code)) {
> + case BPF_DW: return 8;
> + case BPF_W: return 4;
> + case BPF_H: return 2;
> + case BPF_B: return 1;
> + default: return -1;
> + }
> +}
> +
> +static int insn_bytes_to_mem_sz(__u32 sz)
> +{
> + switch (sz) {
> + case 8: return BPF_DW;
> + case 4: return BPF_W;
> + case 2: return BPF_H;
> + case 1: return BPF_B;
> + default: return -1;
> + }
> +}
filter.h has these two helpers. They're named bytes_to_bpf_size() and bpf_size_to_bytes().
I guess we cannot really share kernel and libbpf implementation, but
could you please name them the same way so it's easier to follow
for folks who read both kernel and libbpf code?
> + if (res->new_sz != res->orig_sz) {
> + mem_sz = insn_mem_sz_to_bytes(insn);
> + if (mem_sz != res->orig_sz) {
> + pr_warn("prog '%s': relo #%d: insn #%d (LDX/ST/STX) unexpected mem size: got %u, exp %u\n",
> + prog->name, relo_idx, insn_idx, mem_sz, res->orig_sz);
> + return -EINVAL;
> + }
> +
> + mem_sz = insn_bytes_to_mem_sz(res->new_sz);
> + if (mem_sz < 0) {
Please use new variable here appropriately named.
Few lines above mem_sz is in bytes while here it's encoding opcode.
next prev parent reply other threads:[~2020-10-06 18:08 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-02 1:06 [PATCH bpf-next 0/3] libbpf: auto-resize relocatable LOAD/STORE instructions Andrii Nakryiko
2020-10-02 1:06 ` [PATCH bpf-next 1/3] libbpf: support safe subset of load/store instruction resizing with CO-RE Andrii Nakryiko
2020-10-06 18:08 ` Alexei Starovoitov [this message]
2020-10-06 18:17 ` Andrii Nakryiko
2020-10-02 1:06 ` [PATCH bpf-next 2/3] libbpf: allow specifying both ELF and raw BTF for CO-RE BTF override Andrii Nakryiko
2020-10-02 1:06 ` [PATCH bpf-next 3/3] selftests/bpf: validate libbpf's auto-sizing of LD/ST/STX instructions Andrii Nakryiko
2020-10-07 17:56 ` [PATCH bpf-next 0/3] libbpf: auto-resize relocatable LOAD/STORE instructions Luka Perkov
2020-10-07 18:01 ` Andrii Nakryiko
2020-10-08 10:34 ` Luka Perkov
2020-10-08 17:59 ` Andrii Nakryiko
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=20201006180839.lvoigzpmr32rrroj@ast-mbp \
--to=alexei.starovoitov@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=luka.perkov@sartura.hr \
--cc=netdev@vger.kernel.org \
--cc=tony.ambardar@gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).