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

[Shahab Vahedi <list+bpf@vahedi.org>]

Hi Björn,

Thank you very much for your inputs. Please find my remarks below.

Björn Töpel <bjorn@kernel.org> writes:

> Shahab Vahedi <list+bpf@vahedi.org> 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/

> 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.

> 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.

> 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.

> > +#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: 00000000: 8a 20 00 10 8a 21 00 10 0a 20 00 02 0a 21 40 02
  JIT code: 00000010: 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.


Cheers,
Shahab


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