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