Re: [PATCH bpf-next v1] ARC: Add eBPF JIT support

[Björn Töpel]

Shahab Vahedi  writes:

> Hi Björn,
>
> Thank you very much for your inputs. Please find my remarks below.
>
> Björn Töpel  writes:
>
>> Shahab Vahedi  writes:
>> 
>> What's the easiest way to test test this w/o ARC HW? Is there a qemu
>> port avaiable?
>
> Yes, there is a (downstream) port available on GitHub [1]. If one is
> interested, there are also guides about building QEMU for ARC targets [2]
> and how to run eBPF tests for ARC Linux [3].
>
> [1] ARC QEMU port
> https://github.com/foss-for-synopsys-dwc-arc-processors/qemu
>
> [2] Building ARC QEMU
> https://foss-for-synopsys-dwc-arc-processors.github.io/experimental-documentation/2023.09/simulators/qemu/
>
> [3] Runing eBPF tests for ARC Linux
> https://foss-for-synopsys-dwc-arc-processors.github.io/experimental-documentation/2023.09/linux/ebpf/build/

Cool, TY.

>> I don't know much about ARC -- Is v2 compatible with v3?
>
> No, they're not. For what it's worth, ARCv3 comes in {32,64}-bit
> flavours which are not compatible with each other either.
>
>> I'm curious about the missing support; tailcall/atomic/division/extable
>> support. Would it require a lot of work to add that support in the
>> initial change set?
>
> If you're asking whether it is possible that I add those features now,
> my answer unfortunately would be "no". However, the way that things
> are implemented, it will be a straightforward addition.

Ok! Did you try building the kselftest/bpf suite? Would be interesting
to see the pass/fail rate of test_progs.

>> There are a lot of checkpatch/kernel style issues. Run, e.g.,
>> "checkpatch --strict -g HEAD" and you'll get a bunch of issues. Most of
>> them are just basic style issues. Please try to fix most of them for the
>> next rev.
>
> I did run the "checkpatch" before submitting. I've fixed all the "errors"
> and most of the "warnings". But now that you brought it up, I will try to
> fix as many "warnings"/"checks" as make sense.

Ok. I noticed a lot non-kernel style in your patch (checking against
NULL e.g.)

>> You should add yourself to the MAINTAINERS file.
>
> I will. Thanks!
>
>> Please try to avoid static inline in the C-files. The compiler usually
>> knows better.
>
> I will replace them with "static" then.
>
>> > +/* Sane initial values for the globals */
>> > +bool emit = true;
>> > +bool zext_thyself = true;
>> 
>> Hmm, this is racy. Can we move this into the jit context? Also, is
>> zext_thyself even used?
>
> I will get rid of those. For the record, "zext_thyself" is used by
> calling "zext()" after handling "BPF_ALU" operations.

Ah, indeed!

>> > +#define CHECK_RET(cmd)\
>> > +  do {\
>> > +  ret = (cmd);\
>> > +  if (ret < 0)\
>> > +  return ret; \
>> > +  } while (0)
>> > +
>> 
>> Nit/personal taste, but I prefer not having these kind of macros. I
>> think it makes it harder to read the code.
>
> At some point, I found myself distracted from seeing the bigger picture
> while the code was interspersed by the menial "return checking"s. If
> you don't mind, I'd rather keep it as is, unless you feel strong about
> it or Vineet also agrees with you.
>
>> Care to elaborate a bit more on ARC_BPF_JIT_DEBUG. This smells of
>> duplicated funtionality with bpf_jit_dump(), and the BUG()s are scary.
>
> ARC_BPF_JIT_DEBUG is supposed to be enabled for development purposes.
> It enables:
>
> 1. A set of assert-like condition checking which makes the code
> slow and can lead to ungraceful terminations.
>
> 2. Use of a custom version of hex dumps. The most important difference
> with bpf_jit_dump() is that bpf_jit_dump() cannot be used for dumping
> the input BPF byte stream. Rest, I can live with. An example follows:
>
> Using only "bpf_jit_dump" (ARC_BPF_JIT_DEBUG is not defined)
>
>   flen=2 proglen=20 pass=1 image=2e8c6fb9 from=hello pid=127
>   JIT code: : 8a 20 00 10 8a 21 00 10 0a 20 00 02 0a 21 40 02
>   JIT code: 0010: e0 20 c0 07
>
> vs.
>
> Using the custom version (ARC_BPF_JIT_DEBUG is defined)
>   -[  VM   ]-
>   0xb7, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>   0x95, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>   -[ JIT:1 ]-
>   0x8a, 0x20, 0x00, 0x10, 0x8a, 0x21, 0x00, 0x10
>   0x0a, 0x20, 0x00, 0x02, 0x0a, 0x21, 0x40, 0x02
>   0xe0, 0x20, 0xc0, 0x07
>
>> > +static int jit_ctx_init(struct jit_context *ctx, struct bpf_prog *prog)
>> > +{
>> > +   ...
>> 
>> I'd just make sure that ctx is zeroed, and init the non-zero members here.
>
> Very good point! I will implement it that way.
>
> If you have read this far, I'd like to thank you again for spending time
> on reviewing this patch. It is much appreciated.

Looking forward for the next revision!


Björn

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH bpf-next v1] ARC: Add eBPF JIT support

[Björn Töpel]

Shahab,

Shahab Vahedi  writes:

> From: Shahab Vahedi 
>
> This will add eBPF JIT support to the 32-bit ARCv2 processors. The
> implementation is qualified by running the BPF tests on a Synopsys HSDK
> board with "ARC HS38 v2.1c at 500 MHz" as the 4-core CPU.

Cool!

I did quick review, mosty focusing on style, and not function. Some
general input/Qs:

What's the easiest way to test test this w/o ARC HW? Is there a qemu
port avaiable?

I don't know much about ARC -- Is v2 compatible with v3?

I'm curious about the missing support; tailcall/atomic/division/extable
support. Would it require a lot of work to add that support in the
inital change set?

There are a lot of checkpatch/kernel style issues. Run, e.g.,
"checkpatch --strict -g HEAD" and you'll get a bunch of issues. Most of
them are just basic style issues. Please try to fix most of them for the
next rev.

You should add yourself to the MAINTAINERS file.

Please try to avoid static inline in the C-files. The compiler usually
knows better.


[...]

> diff --git a/arch/arc/net/bpf_jit_core.c b/arch/arc/net/bpf_jit_core.c
> new file mode 100644
> index ..730a715d324e
> --- /dev/null
> +++ b/arch/arc/net/bpf_jit_core.c
> @@ -0,0 +1,1425 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * The back-end-agnostic part of Just-In-Time compiler for eBPF bytecode.
> + *
> + * Copyright (c) 2024 Synopsys Inc.
> + * Author: Shahab Vahedi 
> + */
> +#include 
> +#include "bpf_jit.h"
> +
> +/* Sane initial values for the globals */
> +bool emit = true;
> +bool zext_thyself = true;

Hmm, this is racy. Can we move this into the jit context? Also, is
zext_thyself even used?

> +
> +/*
> + * Check for the return value. A pattern used oftenly in this file.
> + * There must be a "ret" variable of type "int" in the scope.
> + */
> +#define CHECK_RET(cmd)   \
> + do {\
> + ret = (cmd);\
> + if (ret < 0)\
> + return ret; \
> + } while (0)
> +

Nit/personal taste, but I prefer not having these kind of macros. I
think it makes it harder to read the code.

> +#ifdef ARC_BPF_JIT_DEBUG
> +/* Dumps bytes in /var/log/messages at KERN_INFO level (4). */
> +static void dump_bytes(const u8 *buf, u32 len, const char *header)
> +{
> + u8 line[64];
> + size_t i, j;
> +
> + pr_info("-[ %s ]-\n", header);
> +
> + for (i = 0, j = 0; i < len; i++) {
> + /* Last input byte? */
> + if (i == len-1) {
> + j += scnprintf(line+j, 64-j, "0x%02x", buf[i]);
> + pr_info("%s\n", line);
> + break;
> + }
> + /* End of line? */
> + else if (i % 8 == 7) {
> + j += scnprintf(line+j, 64-j, "0x%02x", buf[i]);
> + pr_info("%s\n", line);
> + j = 0;
> + } else {
> + j += scnprintf(line+j, 64-j, "0x%02x, ", buf[i]);
> + }
> + }
> +}
> +#endif /* ARC_BPF_JIT_DEBUG */
> +
> +/* JIT context ***/
> +
> +/*
> + * buf:  Translated instructions end up here.
> + * len:  The length of whole block in bytes.
> + * index:The offset at which the _next_ instruction may be put.
> + */
> +struct jit_buffer {
> + u8  *buf;
> + u32 len;
> + u32 index;
> +};
> +
> +/*
> + * This is a subset of "struct jit_context" that its information is deemed
> + * necessary for the next extra pass to come.
> + *
> + * bpf_header:   Needed to finally lock the region.
> + * bpf2insn: Used to find the translation for instructions of interest.
> + *
> + * Things like "jit.buf" and "jit.len" can be retrieved respectively from
> + * "prog->bpf_func" and "prog->jited_len".
> + */
> +struct arc_jit_data {
> + struct bpf_binary_header *bpf_header;
> + u32  *bpf2insn;
> +};
> +
> +/*
> + * The JIT pertinent context that is used by different functions.
> + *
> + * prog: The current eBPF program being handled.
> + * orig_prog:The original eBPF program before any possible 
> change.
> + * jit:  The JIT buffer and its length.
> + * bpf_header:   The JITed program header. "jit.buf" points 
> inside it.
> + * bpf2insn: Maps BPF insn indices to their counterparts in jit.buf.
> + * bpf2insn_valid:   Indicates if "bpf2ins" is populated with the mappings.
> + * jit_data: A piece of memory to transfer data to the next pass.
> + * arc_regs_clobbered:   Each bit status determines if that arc reg is 
> clobbered.
> + * save_blink:   Whether ARC's "blink" register needs to be 
> saved.
> + * frame_size:   Derived from "prog->aux->stack_depth".
> + * epilogue_offset:  Used by early "return"s in the code to jump here.
> + *