On Wed, Jul 10, 2019 at 4:47 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
>
> > Am 09.07.2019 um 19:48 schrieb Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>:
> >
> > On Tue, Jul 9, 2019 at 8:19 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
> >>
> >> Right now, on certain architectures, these macros are usable only with
> >> kernel headers. This patch makes it possible to use them with userspace
> >> headers and, as a consequence, not only in BPF samples, but also in BPF
> >> selftests.
> >>
> >> On s390, provide the forward declaration of struct pt_regs and cast it
> >> to user_pt_regs in PT_REGS_* macros. This is necessary, because instead
> >> of the full struct pt_regs, s390 exposes only its first member
> >> user_pt_regs to userspace, and bpf_helpers.h is used with both userspace
> >> (in selftests) and kernel (in samples) headers. It was added in commit
> >> 466698e654e8 ("s390/bpf: correct broken uapi for
> >> BPF_PROG_TYPE_PERF_EVENT program type").
> >>
> >> Ditto on arm64.
> >>
> >> On x86, provide userspace versions of PT_REGS_* macros. Unlike s390 and
> >> arm64, x86 provides struct pt_regs to both userspace and kernel, however,
> >> with different member names.
> >>
> >> Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> >> ---
> >
> > Just curious, what did you use as a reference for which register
> > corresponds to which PARM, RET, etc for different archs? I've tried to
> > look it up the other day, and it wasn't as straightforward to find as
> > I hoped for, so maybe I'm missing something obvious.
>
> For this particular change I did not have to look it up, because it all
> was already in the code, I just needed to adapt it to userspace headers.
> Normally I would google for „abi supplement“ to find this information.
> A lazy way would be to simply ask the (cross-)compiler:
>
> cat <<HERE | aarch64-linux-gnu-gcc -x c -O3 -S - -o -
> int f(int a, int b, int c, int d, int e, int f, int g, int h, int i, int j);
> int g() { return -f(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); }
> HERE
Thanks for this trick! :)
>
> I’ve just double checked the supported arches, and noticed that:
>
> #define PT_REGS_PARM5(x) ((x)->uregs[4])
> for bpf_target_arm (arm-linux-gnueabihf) looks wrong:
> the 5th parameter should be passed on stack. This observation matches
> [1].
>
> #define PT_REGS_RC(x) ((x)->regs[1])
> for bpf_target_mips (mips64el-linux-gnuabi64) also looks wrong:
> the return value should be in register 2. This observation matches [2].
Now I'm glad I asked :)
>
> Since I’m not an expert on those architectures, my conclusions could be
> incorrect (e.g. becase a different ABI is normally used in practice).
> Adrian and David, could you please correct me if I’m wrong?
>
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf
> [2] ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/psABI_mips3.0.pdf
>
> >> tools/testing/selftests/bpf/bpf_helpers.h | 61 +++++++++++++++--------
> >> 1 file changed, 41 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> >> index 73071a94769a..212ec564e5c3 100644
> >> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> >> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> >> @@ -358,6 +358,7 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> >>
> >> #if defined(bpf_target_x86)
> >>
> >> +#ifdef __KERNEL__
> >> #define PT_REGS_PARM1(x) ((x)->di)
> >> #define PT_REGS_PARM2(x) ((x)->si)
> >> #define PT_REGS_PARM3(x) ((x)->dx)
> >> @@ -368,19 +369,35 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> >> #define PT_REGS_RC(x) ((x)->ax)
> >> #define PT_REGS_SP(x) ((x)->sp)
> >> #define PT_REGS_IP(x) ((x)->ip)
> >> +#else
> >> +#define PT_REGS_PARM1(x) ((x)->rdi)
> >> +#define PT_REGS_PARM2(x) ((x)->rsi)
> >> +#define PT_REGS_PARM3(x) ((x)->rdx)
> >> +#define PT_REGS_PARM4(x) ((x)->rcx)
> >> +#define PT_REGS_PARM5(x) ((x)->r8)
> >> +#define PT_REGS_RET(x) ((x)->rsp)
> >> +#define PT_REGS_FP(x) ((x)->rbp)
> >> +#define PT_REGS_RC(x) ((x)->rax)
> >> +#define PT_REGS_SP(x) ((x)->rsp)
> >> +#define PT_REGS_IP(x) ((x)->rip)
> >
> > Will this also work for 32-bit x86?
>
> Thanks, this is a good catch: this builds, but makes 64-bit accesses, as if it used the 64-bit
> variant of pt_regs. I will fix this.
Sounds good, thanks!
