Re: [PATCH v10 6/8] selftest/bpf: Add tailcall verifier selftest for powerpc64

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The patch introduces a multi-line comment that violates the BPF subsystem comment style.
- [Low] Redundant and misleading repetition of `__jited("...")` annotations.
--

--- Patch [6]: [PATCH v10 6/8] selftest/bpf: Add tailcall verifier selftest for powerpc64 ---
commit 93fe35d1a465f1c1dd05d30ed36bf46603bc5f71
Author: Abhishek Dubey <adubey@linux.ibm.com>

selftest/bpf: Add tailcall verifier selftest for powerpc64

This commit adds a new BPF tailcall verifier JIT selftest for the
powerpc64 architecture. It verifies the emitted JIT instructions for
tail calls, handling differences between PCREL and non-PCREL kernels.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
> index 48fa34d2959f4..7655742f627bc 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
[ ... ]
> @@ -91,6 +91,80 @@ __jited("	popq	%rax")
>  __jited("	jmp	{{.*}}")		/* jump to tail call tgt   */
>  __jited("L0:	leave")
>  __jited("	{{(retq|jmp	0x)}}")		/* return or jump to rethunk */
> +__arch_powerpc64
> +/* program entry for main(), regular function prologue */
> +__jited("	nop")
> +__jited("...")                          /* ld 2, 16(13) absent with CONFIG_PPC_KERNEL_PCREL */
> +__jited("	li 9, 0")
> +__jited("	std 9, -8(1)")
> +__jited("	mflr 0")
> +__jited("	std 0, 16(1)")
> +__jited("	stdu 1, {{.*}}(1)")
> +/* load address and call sub() via count register */
> +/* Address materialization differs between PCREL and non-PCREL kernels.
> + * Skip the address generation sequence and verify only that the call
> + * target is loaded into CTR before branching.
> + */

[Severity: Low]
This isn't a bug, but does this comment block follow the standard BPF
subsystem comment style? 

The subsystem guidelines require multi-line comments to have the opening
characters on a dedicated line, like this:

/*
 * Address materialization differs between PCREL and non-PCREL kernels.
 * Skip the address generation sequence...
 */

> +__jited("...")
> +__jited("...")
> +__jited("...")
> +__jited("...")
> +__jited("...")

[Severity: Low]
Are these consecutive string annotations redundant?

When test_loader.c parses the "..." string via push_disasm_msg(), it merely
sets the on_next_line flag to false. Repeating it multiple times does not
force the verifier to skip exactly five instructions; it just sets the same
boolean flag repeatedly. 

Would a single annotation be sufficient to achieve the desired skip?

> +__jited("	mtctr 12")
> +__jited("	bctrl")
> +__jited("	mr	8, 3")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785906979.git.skb99@linux.ibm.com?part=6