| Message ID | 20190709151809.37539-4-iii@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | selftests/bpf: fix compiling loop{1,2,3}.c on s390 | expand |
On Tue, Jul 9, 2019 at 8:19 AM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com> > --- 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. > 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? > +#endif <snip>
> Am 09.07.2019 um 19:48 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: > > On Tue, Jul 9, 2019 at 8:19 AM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com> >> --- > > 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 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]. 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.
On Wed, Jul 10, 2019 at 4:47 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > > Am 09.07.2019 um 19:48 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: > > > > On Tue, Jul 9, 2019 at 8:19 AM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com> > >> --- > > > > 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!
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) +#endif #elif defined(bpf_target_s390) -#define PT_REGS_PARM1(x) ((x)->gprs[2]) -#define PT_REGS_PARM2(x) ((x)->gprs[3]) -#define PT_REGS_PARM3(x) ((x)->gprs[4]) -#define PT_REGS_PARM4(x) ((x)->gprs[5]) -#define PT_REGS_PARM5(x) ((x)->gprs[6]) -#define PT_REGS_RET(x) ((x)->gprs[14]) -#define PT_REGS_FP(x) ((x)->gprs[11]) /* Works only with CONFIG_FRAME_POINTER */ -#define PT_REGS_RC(x) ((x)->gprs[2]) -#define PT_REGS_SP(x) ((x)->gprs[15]) -#define PT_REGS_IP(x) ((x)->psw.addr) +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */ +struct pt_regs; +#define PT_REGS_S390 const volatile user_pt_regs +#define PT_REGS_PARM1(x) (((PT_REGS_S390 *)(x))->gprs[2]) +#define PT_REGS_PARM2(x) (((PT_REGS_S390 *)(x))->gprs[3]) +#define PT_REGS_PARM3(x) (((PT_REGS_S390 *)(x))->gprs[4]) +#define PT_REGS_PARM4(x) (((PT_REGS_S390 *)(x))->gprs[5]) +#define PT_REGS_PARM5(x) (((PT_REGS_S390 *)(x))->gprs[6]) +#define PT_REGS_RET(x) (((PT_REGS_S390 *)(x))->gprs[14]) +/* Works only with CONFIG_FRAME_POINTER */ +#define PT_REGS_FP(x) (((PT_REGS_S390 *)(x))->gprs[11]) +#define PT_REGS_RC(x) (((PT_REGS_S390 *)(x))->gprs[2]) +#define PT_REGS_SP(x) (((PT_REGS_S390 *)(x))->gprs[15]) +#define PT_REGS_IP(x) (((PT_REGS_S390 *)(x))->psw.addr) #elif defined(bpf_target_arm) @@ -397,16 +414,20 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode, #elif defined(bpf_target_arm64) -#define PT_REGS_PARM1(x) ((x)->regs[0]) -#define PT_REGS_PARM2(x) ((x)->regs[1]) -#define PT_REGS_PARM3(x) ((x)->regs[2]) -#define PT_REGS_PARM4(x) ((x)->regs[3]) -#define PT_REGS_PARM5(x) ((x)->regs[4]) -#define PT_REGS_RET(x) ((x)->regs[30]) -#define PT_REGS_FP(x) ((x)->regs[29]) /* Works only with CONFIG_FRAME_POINTER */ -#define PT_REGS_RC(x) ((x)->regs[0]) -#define PT_REGS_SP(x) ((x)->sp) -#define PT_REGS_IP(x) ((x)->pc) +/* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */ +struct pt_regs; +#define PT_REGS_ARM64 const volatile struct user_pt_regs +#define PT_REGS_PARM1(x) (((PT_REGS_ARM64 *)(x))->regs[0]) +#define PT_REGS_PARM2(x) (((PT_REGS_ARM64 *)(x))->regs[1]) +#define PT_REGS_PARM3(x) (((PT_REGS_ARM64 *)(x))->regs[2]) +#define PT_REGS_PARM4(x) (((PT_REGS_ARM64 *)(x))->regs[3]) +#define PT_REGS_PARM5(x) (((PT_REGS_ARM64 *)(x))->regs[4]) +#define PT_REGS_RET(x) (((PT_REGS_ARM64 *)(x))->regs[30]) +/* Works only with CONFIG_FRAME_POINTER */ +#define PT_REGS_FP(x) (((PT_REGS_ARM64 *)(x))->regs[29]) +#define PT_REGS_RC(x) (((PT_REGS_ARM64 *)(x))->regs[0]) +#define PT_REGS_SP(x) (((PT_REGS_ARM64 *)(x))->sp) +#define PT_REGS_IP(x) (((PT_REGS_ARM64 *)(x))->pc) #elif defined(bpf_target_mips)
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@linux.ibm.com> --- tools/testing/selftests/bpf/bpf_helpers.h | 61 +++++++++++++++-------- 1 file changed, 41 insertions(+), 20 deletions(-)