Re: [PATCH bpf-next 0/2] bpf, riscv: Add support for indirect jumps

[Chen Pei]

Hi all,

Thanks for the reviews. Patch 1 is a plain wording fix and will go into
v2. For patch 2 I need your input on one thing before I respin: which
form of the #endif comment you would prefer.

Patch 1, on the comment above bpf_prog_update_insn_ptrs():

> This isn't a bug, but could the comment say "shift ctx->offset[] by one"
> rather than "shift the linfo array by one"? The jited_linfo was already
> filled in by the bpf_prog_fill_jited_linfo() call above, and it is the
> JIT's own offset table that is being rewritten here.

Agreed. Fixed in v2.

Patch 2, on the #endif marker in verifier_gotox.c:

> [Severity: Low]
> This isn't a bug, but does this newly introduced multi-line comment
> follow the BPF subsystem style guide? The subsystem guidelines
> explicitly require that multi-line comments have the opening '/*' on
> its own line, rather than beginning text on the same line as the
> opening marker.

I would rather collapse it to a single line, but there is more than one
way to spell it:

  a) 89 columns, clean under scripts/checkpatch.pl --strict:

#endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 || __TARGET_ARCH_powerpc || riscv64 */

  b) 101 columns, keeps every macro name verbatim, but checkpatch then
     reports "WARNING: line length of 101 exceeds 100 columns":

#endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 || __TARGET_ARCH_powerpc || __TARGET_ARCH_riscv */

  c) or a short marker that does not repeat the condition at all:

#endif /* gotox-capable arch */

I lean towards (a): tools/testing/selftests/bpf/progs/ otherwise only
uses single-line #endif markers, and "riscv64" matches the guard, which
is __TARGET_ARCH_riscv && __riscv_xlen == 64.

I will send v2 with the patch 1 fix and whichever form you pick, together
with any other feedback, so please let me know if you would like anything
else changed. If nobody has a preference, I will go with (a) in a few
days.

Thanks,
Pei