Re: [PATCH bpf-next 10/12] selftests/bpf: Add inline-asm and subprog tests for R0:R2 returns

[Yonghong Song <yonghong.song@linux.dev>]

On 7/9/26 6:38 PM, Eduard Zingerman wrote:
> On Wed, 2026-07-08 at 13:10 -0700, Yonghong Song wrote:
>> Add inline-asm and subprogram tests for more coverage, including
>> BPF-to-BPF cases, kfunc cases, backtracking and liveness. Both positive
>> and negative tests are added.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
> [...]
>
>> @@ -93,4 +97,25 @@ void test_aggregate_ret(void)
>>   		run_prog(skel_union_c->progs.aggregate_ret_union_c_test, true);
>>   
>>   	aggregate_ret_union_c__destroy(skel_union_c);
>> +
>> +	skel_run = aggregate_ret_run__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel_run, "skel_run_open_load"))
>> +		return;
>> +
>> +	/* Inline-asm variant: compiler-version independent, always runs. */
>> +	if (test__start_subtest("asm"))
>> +		run_prog(skel_run->progs.aggregate_ret_asm_test, false);
>> +
>> +	/* Struct-by-value kfunc returns, also compiler-version independent. */
>> +	if (test__start_subtest("struct"))
>> +		run_prog(skel_run->progs.aggregate_ret_struct_test, false);
>> +
>> +	/* Union-by-value kfunc return, compiler-version independent. */
>> +	if (test__start_subtest("union"))
>> +		run_prog(skel_run->progs.aggregate_ret_union_test, false);
>> +
>> +	aggregate_ret_run__destroy(skel_run);
>> +
>> +	RUN_TESTS(aggregate_ret_func);
>> +	RUN_TESTS(aggregate_ret_kfunc);
> This can be folded into tools/testing/selftests/bpf/prog_tests/verifier.c.
>
>>   }
>> diff --git a/tools/testing/selftests/bpf/progs/aggregate_ret_func.c b/tools/testing/selftests/bpf/progs/aggregate_ret_func.c
>> new file mode 100644
>> index 000000000000..31e176103374
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/aggregate_ret_func.c
>> @@ -0,0 +1,361 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include "bpf_misc.h"
>> +
>> +typedef unsigned __int128 u128;
>> +
>> +__naked u128 global_agg_good(void)
>> +{
>> +	asm volatile (
>> +	"r0 = 0x1234;"	/* low 64 bits */
>> +	"r2 = 0x5678;"	/* high 64 bits */
>> +	"exit;"
>> +	);
>> +}
>> +
>> +__naked u128 global_agg_bad(void)
>> +{
>> +	asm volatile (
>> +	"r0 = 0;"
>> +	"exit;"
>> +	);
>> +}
> C version of tests from a previous patch already checks global vs.
> local functions. I suggest dropping that version from C tests and keep
> in assembly only to reduce the duplication.
>
> [...]
>
>> +SEC("tc")
>> +__success __retval(0)
>> +int aggregate_ret_global_union(void *ctx)
>> +{
>> +	__u64 lo, hi;
>> +
>> +	asm volatile (
>> +	"call %[global_ret_union];"
>> +	"%[lo] = r0;"
>> +	"%[hi] = r2;"
>> +	: [lo]"=r"(lo), [hi]"=r"(hi)
>> +	: __imm(global_ret_union)
>> +	: "r0", "r1", "r2", "r3", "r4", "r5");
>> +	if (lo != 0x1234)
>> +		return 1;
>> +	if (hi != 0x5678)
>> +		return 2;
>> +	return 0;
>> +}
> And here I'd only keep the C version.
>
>> +__naked struct with_ptr global_ret_struct_ptr(void)
>> +{
>> +	asm volatile (
>> +	"r0 = 0;"
>> +	"r2 = 0;"
>> +	"exit;"
>> +	);
>> +}
> [...]
>
>> +static __naked u128 agg_callee(void)
>> +{
>> +	asm volatile (
>> +	"r0 = 1;"
>> +	"r2 = 2;"
>> +	"exit;"
>> +	);
>> +}
>> +
>> +SEC("tc")
>> +__log_level(2)
>> +__msg("Live regs before insn:")
>> +__msg("0: .12345.... (85) call pc+2")
>> +__msg("1: ..2....... (bf) r0 = r2")
>> +__msg("2: 0......... (95) exit")
>> +__msg("3: .......... (b7) r0 = 1")
>> +__msg("4: 0......... (b7) r2 = 2")
>> +__msg("5: 0.2....... (95) exit")
>> +__naked int aggregate_ret_live(void)
>> +{
>> +	asm volatile (
>> +	"call %[agg_callee];"
>> +	"r0 = r2;"
>> +	"exit;"
>> +	:
>> +	: [agg_callee]"i"(agg_callee)
>> +	: __clobber_all);
>> +}
> Nit: usually we add such tests to compute_live_registers.c.

I can move the test to compute_live_registers.c.

>
> [...]
>
>> diff --git a/tools/testing/selftests/bpf/progs/aggregate_ret_run.c b/tools/testing/selftests/bpf/progs/aggregate_ret_run.c
> I think the test cases in this file are by a large degree redundant.
> A single test case calling a kfunc returning a small struct and
> checking returned values would suffice.

Ack. I tried to test for both C and asm test cases. They are indeed some duplicates.
I will try to remove the redundant ones in next revision.

>
> [...]