Shahab Vahedi <list+bpf@xxxxxxxxxx> writes: > Hi Björn, > > Thank you very much for your inputs. Please find my remarks below. > > Björn Töpel <bjorn@xxxxxxxxxx> writes: > >> Shahab Vahedi <list+bpf@xxxxxxxxxx> 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: 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. Looking forward for the next revision! Björn
