From: Puranjay Mohan <puranjay12@gmail.com>
To: Song Liu <song@kernel.org>
Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
martin.lau@linux.dev, catalin.marinas@arm.com,
mark.rutland@arm.com, bpf@vger.kernel.org, kpsingh@kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 3/3] bpf, arm64: use bpf_jit_binary_pack_alloc
Date: Thu, 8 Jun 2023 18:51:23 +0200 [thread overview]
Message-ID: <CANk7y0ihF2RvwGXK0p0gOaGrMoPxfb6fcfn3-J40zn8LS8vROQ@mail.gmail.com> (raw)
In-Reply-To: <CAPhsuW7soOQasGw5fHB2qTeJnqR4ZrGBodyO87k=vg=TYqCsWA@mail.gmail.com>
Hi Song,
On Thu, Jun 8, 2023 at 6:28 PM Song Liu <song@kernel.org> wrote:
>
> On Wed, Jun 7, 2023 at 2:18 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >
> [...]
> > +
> > static inline int epilogue_offset(const struct jit_ctx *ctx)
> > {
> > int to = ctx->epilogue_offset;
> > @@ -701,7 +716,8 @@ static int add_exception_handler(const struct bpf_insn *insn,
> > struct jit_ctx *ctx,
> > int dst_reg)
> > {
> > - off_t offset;
> > + off_t ins_offset;
> > + off_t fixup_offset;
>
> Please add some comments for these two offsets.
Here I am using two variables because I need to change from the RO
buffer for calculating offsets
to the RW buffer for writing.
Earlier, a single variable could work because it was being reused for
calculating the second offset
after writing the first one. Here, I can't re-calculate using the same
variable because I have to change
to the RW buffer, and using the same variable would need changing back
to the RO buffer.
So, I am calculating both offsets first, changing to RW buffer and
writing both offsets.
But I will add comments explaining what these offsets are being used for.
>
> > unsigned long pc;
> > struct exception_table_entry *ex;
> >
> > @@ -717,12 +733,11 @@ static int add_exception_handler(const struct bpf_insn *insn,
> > return -EINVAL;
> >
> > ex = &ctx->prog->aux->extable[ctx->exentry_idx];
> > - pc = (unsigned long)&ctx->image[ctx->idx - 1];
> > + pc = (unsigned long)&ctx->ro_image[ctx->idx - 1];
> >
> > - offset = pc - (long)&ex->insn;
> > - if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
> > + ins_offset = pc - (long)&ex->insn;
> > + if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
> > return -ERANGE;
> > - ex->insn = offset;
> >
> > /*
> > * Since the extable follows the program, the fixup offset is always
> > @@ -732,11 +747,20 @@ static int add_exception_handler(const struct bpf_insn *insn,
> > * modifying the upper bits because the table is already sorted, and
> > * isn't part of the main exception table.
> > */
> > - offset = (long)&ex->fixup - (pc + AARCH64_INSN_SIZE);
> > - if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
> > + fixup_offset = (long)&ex->fixup - (pc + AARCH64_INSN_SIZE);
> > + if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
> > return -ERANGE;
> >
> > - ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
> > + /*
> > + * The offsets above have been calculated using the RO buffer but we
> > + * need to use the R/W buffer for writes.
> > + * switch ex to rw buffer for writing.
> > + */
> > + ex = (void *)ctx->image + ((void *)ex - (void *)ctx->ro_image);
> > +
> > + ex->insn = ins_offset;
> > +
> > + ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
> > FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
> >
> > ex->type = EX_TYPE_BPF;
> [...]
> > /* And we're done. */
> > if (bpf_jit_enable > 1)
> > bpf_jit_dump(prog->len, prog_size, 2, ctx.image);
> >
> > - bpf_flush_icache(header, ctx.image + ctx.idx);
> > + bpf_flush_icache(ro_header, ctx.ro_image + ctx.idx);
> >
> > if (!prog->is_func || extra_pass) {
> > if (extra_pass && ctx.idx != jit_data->ctx.idx) {
> > pr_err_once("multi-func JIT bug %d != %d\n",
> > ctx.idx, jit_data->ctx.idx);
> > - bpf_jit_binary_free(header);
> > prog->bpf_func = NULL;
> > prog->jited = 0;
> > prog->jited_len = 0;
> > + goto out_free_hdr;
> > + }
> > + if (WARN_ON(bpf_jit_binary_pack_finalize(prog, ro_header,
> > + header))) {
> > + ro_header = NULL;
>
> I think we need
> prog = orig_prog;
> here.
I agree, this is a mistake from my side.
I will add this in the next version.
Thanks,
Puranjay Mohan
next prev parent reply other threads:[~2023-06-08 16:51 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-07 9:18 [PATCH bpf-next v2 0/3] bpf, arm64: use BPF prog pack allocator in BPF JIT Puranjay Mohan
2023-06-07 9:18 ` [PATCH bpf-next v2 1/3] bpf: make bpf_prog_pack allocator portable Puranjay Mohan
2023-06-07 9:18 ` [PATCH bpf-next v2 2/3] arm64: patching: Add aarch64_insn_copy() Puranjay Mohan
2023-06-07 9:18 ` [PATCH bpf-next v2 3/3] bpf, arm64: use bpf_jit_binary_pack_alloc Puranjay Mohan
2023-06-08 16:28 ` Song Liu
2023-06-08 16:51 ` Puranjay Mohan [this message]
2023-06-08 21:13 ` Song Liu
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=CANk7y0ihF2RvwGXK0p0gOaGrMoPxfb6fcfn3-J40zn8LS8vROQ@mail.gmail.com \
--to=puranjay12@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=catalin.marinas@arm.com \
--cc=daniel@iogearbox.net \
--cc=kpsingh@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=martin.lau@linux.dev \
--cc=song@kernel.org \
/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).