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

[Björn Töpel]

Hey!

Chen Pei <cp0613@xxxxxxxxxxxxxxxxx> writes:

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

Maybe just (d):
  | #endif /* gotox: x86, arm64, powerpc, riscv64 */

but (c) or (a) is fine as well, IMO.

Feel free to add for the series for your v2:

Reviewed-by: Björn Töpel <bjorn@xxxxxxxxxx>
Acked-by: Björn Töpel <bjorn@xxxxxxxxxx>