Re: [PATCH v4 bpf-next 6/9] selftests/bpf: Test gen_prologue and gen_epilogue

[Martin KaFai Lau <martin.lau@linux.dev>]

On 8/29/24 12:27 AM, Eduard Zingerman wrote:
> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> This test adds a new struct_ops "bpf_testmod_st_ops" in bpf_testmod.
>> The ops of the bpf_testmod_st_ops is triggered by new kfunc calls
>> "bpf_kfunc_st_ops_test_*logue". These new kfunc calls are
>> primarily used by the SEC("syscall") program. The test triggering
>> sequence is like:
>>      SEC("syscall")
>>      syscall_prologue_subprog(struct st_ops_args *args)
>>          bpf_kfunc_st_op_test_prologue(args)
>> 	    st_ops->test_prologue(args)
>>
>> .gen_prologue adds 1000 to args->a
>> .gen_epilogue adds 10000 to args->a
>> .gen_epilogue will also set the r0 to 2 * args->a.
>>
>> The .gen_prologue and .gen_epilogue of the bpf_testmod_st_ops
>> will test the prog->aux->attach_func_name to decide if
>> it needs to generate codes.
>>
>> The main programs of the pro_epilogue_subprog.c will call a subprog()
>> which does "args->a += 1".
>>
>> The main programs of the pro_epilogue_kfunc.c will call a
>> new kfunc bpf_kfunc_st_ops_inc10 which does "args->a += 10".
>>
>> This patch uses the test_loader infra to check the __xlated
>> instructions patched after gen_prologue and/or gen_epilogue.
>> The __xlated check is based on Eduard's example (Thanks!) in v1.
>>
>> args->a is returned by the struct_ops prog (either the main prog
>> or the epilogue). Thus, the __retval of the SEC("syscall") prog
>> is checked. For example, when triggering the ops in the
>> 'SEC("struct_ops/test_epilogue_subprog") int test_epilogue_subprog'
>> The expected args->a is +1 (subprog call) + 10000 (.gen_epilogue) = 10001.
>> The expected return value is 2 * 10001 (.gen_epilogue).
>>
>> Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> [...]
> 
>> diff --git a/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c b/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
>> new file mode 100644
>> index 000000000000..7d1124cf4942
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/pro_epilogue_kfunc.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
>> +
>> +#include <vmlinux.h>
>> +#include <bpf/bpf_tracing.h>
>> +#include "bpf_misc.h"
>> +#include "../bpf_testmod/bpf_testmod.h"
>> +#include "../bpf_testmod/bpf_testmod_kfunc.h"
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +void __kfunc_btf_root(void)
>> +{
>> +	struct st_ops_args args = {};
>> +
>> +	bpf_kfunc_st_ops_inc10(&args);
> 
> Nit: 'bpf_kfunc_st_ops_inc10(0);' would also work.

sgtm. I think it will make it obvious that it won't be executed also.

> 
>> +}
> 
> As a side note, I think that kfunc and subprog sets of tests could be
> combined in order to have less code. Probably does not matter.

ok. I will drop the _subprog.c and only keep the _kfunc.c.

The _kfunc.c calls a subprog and a kfunc which should have already covered the 
_subprog.c cases.