Re: [PATCH bpf-next 2/4] selftests/bpf: convenience macro for use with 'asm volatile' blocks

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2022-12-20 at 13:05 -0800, Andrii Nakryiko wrote:
> On Fri, Dec 16, 2022 at 6:17 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > A set of macros useful for writing naked BPF functions using inline
> > assembly. E.g. as follows:
> > 
> > struct map_struct {
> >         ...
> > } map SEC(".maps");
> > 
> > SEC(...)
> > __naked int foo_test(void)
> > {
> >         asm volatile(
> >                 "r0 = 0;"
> >                 "*(u64*)(r10 - 8) = r0;"
> >                 "r1 = %[map] ll;"
> >                 "r2 = r10;"
> >                 "r2 += -8;"
> >                 "call %[bpf_map_lookup_elem];"
> >                 "r0 = 0;"
> >                 "exit;"
> >                 :
> >                 : __imm(bpf_map_lookup_elem),
> >                   __imm_addr(map)
> >                 : __clobber_all);
> > }
> > 
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >  tools/testing/selftests/bpf/progs/bpf_misc.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
> > index a42363a3fef1..bbf56ad95636 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
> > +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> > @@ -8,6 +8,12 @@
> >  #define __log_level(lvl)       __attribute__((btf_decl_tag("comment:test_log_level="#lvl)))
> >  #define __test_state_freq      __attribute__((btf_decl_tag("comment:test_state_freq")))
> > 
> > +/* Convenience macro for use with 'asm volatile' blocks */
> > +#define __naked __attribute__((naked))
> > +#define __clobber_all "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "memory"
> 
> I found that this one doesn't work well when passing some inputs as
> registers (e.g., for address of a variable on stack). Compiler
> complains that it couldn't find any free registers to use. So I ended
> up using
> 
> #define __asm_common_clobbers "r0", "r1", "r2", "r3", "r4", "r5", "memory"
> 
> and adding "r6", "r7", etc manually, depending on the test.
> 
> So maybe let's add it upfront as `__clobber_common` as well?

Will do.

> 
> But changes look good to me:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> > +#define __imm(name) [name]"i"(name)
> > +#define __imm_addr(name) [name]"i"(&name)
> > +
> >  #if defined(__TARGET_ARCH_x86)
> >  #define SYSCALL_WRAPPER 1
> >  #define SYS_PREFIX "__x64_"
> > --
> > 2.38.2
> >