Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

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

On 8/16/24 1:27 PM, Eduard Zingerman wrote:
> On Fri, 2024-08-16 at 10:27 -0700, Martin KaFai Lau wrote:
> 
> [...]
> 
>> Thanks for checking!
>>
>> I think the bpf_map__attach_struct_ops() is not done such that st_ops is NULL.
>>
>> It probably needs another tag in the SEC("syscall") program to tell which st_ops
>> map should be attached first before executing the "syscall" program.
>>
>> I like the idea of using the __xlated macro to check the patched prologue, ctx
>> pointer saving, and epilogue. I will add this test in the respin. I will keep
>> the current way in this patch to exercise syscall and the ops/func in st_ops for
>> now. We can iterate on it later and use it as an example on what supports are
>> needed on the test_loader side for st_ops map testing. On the repetitive-enough
>> to worth test_loader refactoring side, I suspect some of the existing st_ops
>> load-success/load-failure tests may be worth to look at also. Thoughts?
> 
> You are correct, this happens because bpf_map__attach_struct_ops() is
> not called. Fortunately, the change for test_loader.c is not very big.
> Please check two patches in the attachment.

The patch looks good. I tried and it works. I will add it in the next respin.
That will help to cover the __xlated check on the instructions generated by 
gen_pro/epilogue and also check the syscall return value for the common case.

Except the tail_call test which needs to load a struct_ops program that does 
bpf_tail_call and another struct_ops program that was used in the prog_array. 
These two struct_ops programs need to be used in two separate struct_ops maps to 
be able to load. The way that test_loader attaching all maps in your patch will 
fail because bpf_testmod does not support attaching more than one struct_ops map.

I don't want to further polish on the tail_call testing side. I will stay with 
the current way to do the tail_call test which also allows the more regular 
trampoline "unsigned long long *ctx" for the main struct_ops prog and also 
allows using ctx_in in the SEC("syscall") prog.

Thanks.