| Message ID | 05fb9d72-d1a7-5346-b55b-4495cdf54124@web.de |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] libbpf: Fix register in PT_REGS MIPS macros | expand |
On Thu, Jul 30, 2020 at 4:45 AM Jerry Cruntime <jerry.c.t@web.de> wrote: > > The o32, n32 and n64 calling conventions require the return > value to be stored in $v0 which maps to $2 register, i.e., > the second register. > > Fixes: c1932cd ("bpf: Add MIPS support to samples/bpf.") > --- > tools/lib/bpf/bpf_tracing.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > index 58eceb884..ae205dcf8 100644 > --- a/tools/lib/bpf/bpf_tracing.h > +++ b/tools/lib/bpf/bpf_tracing.h > @@ -215,7 +215,7 @@ struct pt_regs; > #define PT_REGS_PARM5(x) ((x)->regs[8]) I've quickly looked up some doc on MIPS calling convention, doesn't seem like regs[8] is actually used for 5th input argument (the doc I found documented only the use of $4 through $7 for first 4 args). Should we drop PT_REGS_PARM5() for MIPS, while at it? > #define PT_REGS_RET(x) ((x)->regs[31]) > #define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with > CONFIG_FRAME_POINTER */ > -#define PT_REGS_RC(x) ((x)->regs[1]) > +#define PT_REGS_RC(x) ((x)->regs[2]) This looks good, though. > #define PT_REGS_SP(x) ((x)->regs[29]) > #define PT_REGS_IP(x) ((x)->cp0_epc) > > -- > 2.17.1
Hi, > I've quickly looked up some doc on MIPS calling convention, doesn't > seem like regs[8] is actually used for 5th input argument (the doc I > found documented only the use of $4 through $7 for first 4 args). > Should we drop PT_REGS_PARM5() for MIPS, while at it? My understanding is that with o32 only 4 arguments can be passed in registers ($4-$7). But n32 and n64 extended it to pass 8 arguments in registers ($4-$11). My source is "MIPS Run, Second Edition" from Dominic Sweetman table 11.2 on page 327. It is also described here: https://en.wikipedia.org/wiki/Calling_convention#MIPS On 7/30/20 9:55 PM, Andrii Nakryiko wrote: > On Thu, Jul 30, 2020 at 4:45 AM Jerry Cruntime <jerry.c.t@web.de> wrote: >> >> The o32, n32 and n64 calling conventions require the return >> value to be stored in $v0 which maps to $2 register, i.e., >> the second register. >> >> Fixes: c1932cd ("bpf: Add MIPS support to samples/bpf.") >> --- >> tools/lib/bpf/bpf_tracing.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h >> index 58eceb884..ae205dcf8 100644 >> --- a/tools/lib/bpf/bpf_tracing.h >> +++ b/tools/lib/bpf/bpf_tracing.h >> @@ -215,7 +215,7 @@ struct pt_regs; >> #define PT_REGS_PARM5(x) ((x)->regs[8]) > > I've quickly looked up some doc on MIPS calling convention, doesn't > seem like regs[8] is actually used for 5th input argument (the doc I > found documented only the use of $4 through $7 for first 4 args). > Should we drop PT_REGS_PARM5() for MIPS, while at it? > >> #define PT_REGS_RET(x) ((x)->regs[31]) >> #define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with >> CONFIG_FRAME_POINTER */ >> -#define PT_REGS_RC(x) ((x)->regs[1]) >> +#define PT_REGS_RC(x) ((x)->regs[2]) > > This looks good, though. > >> #define PT_REGS_SP(x) ((x)->regs[29]) >> #define PT_REGS_IP(x) ((x)->cp0_epc) >> >> -- >> 2.17.1
On Thu, Jul 30, 2020 at 1:38 PM Jerry Cruntime <jerry.c.t@web.de> wrote: > > Hi, > > > I've quickly looked up some doc on MIPS calling convention, doesn't > > seem like regs[8] is actually used for 5th input argument (the doc I > > found documented only the use of $4 through $7 for first 4 args). > > Should we drop PT_REGS_PARM5() for MIPS, while at it? > > My understanding is that with o32 only 4 arguments can be passed in > registers ($4-$7). But n32 and n64 extended it to pass 8 arguments in > registers ($4-$11). > > My source is "MIPS Run, Second Edition" from Dominic Sweetman table 11.2 > on page 327. It is also described here: > > https://en.wikipedia.org/wiki/Calling_convention#MIPS > Oh, right, I should have used Wikipedia instead. :) Acked-by: Andrii Nakryiko <andriin@fb.com> > > On 7/30/20 9:55 PM, Andrii Nakryiko wrote: > > On Thu, Jul 30, 2020 at 4:45 AM Jerry Cruntime <jerry.c.t@web.de> wrote: > >> > >> The o32, n32 and n64 calling conventions require the return > >> value to be stored in $v0 which maps to $2 register, i.e., > >> the second register. > >> > >> Fixes: c1932cd ("bpf: Add MIPS support to samples/bpf.") > >> --- > >> tools/lib/bpf/bpf_tracing.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > >> index 58eceb884..ae205dcf8 100644 > >> --- a/tools/lib/bpf/bpf_tracing.h > >> +++ b/tools/lib/bpf/bpf_tracing.h > >> @@ -215,7 +215,7 @@ struct pt_regs; > >> #define PT_REGS_PARM5(x) ((x)->regs[8]) > > > > I've quickly looked up some doc on MIPS calling convention, doesn't > > seem like regs[8] is actually used for 5th input argument (the doc I > > found documented only the use of $4 through $7 for first 4 args). > > Should we drop PT_REGS_PARM5() for MIPS, while at it? > > > >> #define PT_REGS_RET(x) ((x)->regs[31]) > >> #define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with > >> CONFIG_FRAME_POINTER */ > >> -#define PT_REGS_RC(x) ((x)->regs[1]) > >> +#define PT_REGS_RC(x) ((x)->regs[2]) > > > > This looks good, though. > > > >> #define PT_REGS_SP(x) ((x)->regs[29]) > >> #define PT_REGS_IP(x) ((x)->cp0_epc) > >> > >> -- > >> 2.17.1
On 7/30/20 1:44 PM, Jerry Cruntime wrote: > The o32, n32 and n64 calling conventions require the return > value to be stored in $v0 which maps to $2 register, i.e., > the second register. > > Fixes: c1932cd ("bpf: Add MIPS support to samples/bpf.") Jerry, your patch is missing a Signed-off-by from you. It should be enough if you just reply with one in here that I'll add to the commit message and I'll take it via bpf tree then, thanks. > --- > tools/lib/bpf/bpf_tracing.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > index 58eceb884..ae205dcf8 100644 > --- a/tools/lib/bpf/bpf_tracing.h > +++ b/tools/lib/bpf/bpf_tracing.h > @@ -215,7 +215,7 @@ struct pt_regs; > #define PT_REGS_PARM5(x) ((x)->regs[8]) > #define PT_REGS_RET(x) ((x)->regs[31]) > #define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with > CONFIG_FRAME_POINTER */ > -#define PT_REGS_RC(x) ((x)->regs[1]) > +#define PT_REGS_RC(x) ((x)->regs[2]) > #define PT_REGS_SP(x) ((x)->regs[29]) > #define PT_REGS_IP(x) ((x)->cp0_epc) > > -- > 2.17.1
Hi, > Jerry, your patch is missing a Signed-off-by from you. Signed-off-by: Jerry Crunchtime <jerry.c.t@web.de> On 7/31/20 1:00 AM, Daniel Borkmann wrote: > On 7/30/20 1:44 PM, Jerry Crunchtime wrote: >> The o32, n32 and n64 calling conventions require the return >> value to be stored in $v0 which maps to $2 register, i.e., >> the second register. >> >> Fixes: c1932cd ("bpf: Add MIPS support to samples/bpf.") > > Jerry, your patch is missing a Signed-off-by from you. It should be > enough if > you just reply with one in here that I'll add to the commit message and > I'll > take it via bpf tree then, thanks. > >> --- >> tools/lib/bpf/bpf_tracing.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h >> index 58eceb884..ae205dcf8 100644 >> --- a/tools/lib/bpf/bpf_tracing.h >> +++ b/tools/lib/bpf/bpf_tracing.h >> @@ -215,7 +215,7 @@ struct pt_regs; >> #define PT_REGS_PARM5(x) ((x)->regs[8]) >> #define PT_REGS_RET(x) ((x)->regs[31]) >> #define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with >> CONFIG_FRAME_POINTER */ >> -#define PT_REGS_RC(x) ((x)->regs[1]) >> +#define PT_REGS_RC(x) ((x)->regs[2]) >> #define PT_REGS_SP(x) ((x)->regs[29]) >> #define PT_REGS_IP(x) ((x)->cp0_epc) >> >> -- >> 2.17.1 >
On 7/31/20 11:56 AM, Jerry Crunchtime wrote: > Hi, > > > Jerry, your patch is missing a Signed-off-by from you. > > Signed-off-by: Jerry Crunchtime <jerry.c.t@web.de> Thanks! One more comment below: > On 7/31/20 1:00 AM, Daniel Borkmann wrote: >> On 7/30/20 1:44 PM, Jerry Crunchtime wrote: >>> The o32, n32 and n64 calling conventions require the return >>> value to be stored in $v0 which maps to $2 register, i.e., >>> the second register. >>> >>> Fixes: c1932cd ("bpf: Add MIPS support to samples/bpf.") >> >> Jerry, your patch is missing a Signed-off-by from you. It should be >> enough if >> you just reply with one in here that I'll add to the commit message and >> I'll >> take it via bpf tree then, thanks. >> >>> --- >>> tools/lib/bpf/bpf_tracing.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h >>> index 58eceb884..ae205dcf8 100644 >>> --- a/tools/lib/bpf/bpf_tracing.h >>> +++ b/tools/lib/bpf/bpf_tracing.h >>> @@ -215,7 +215,7 @@ struct pt_regs; >>> #define PT_REGS_PARM5(x) ((x)->regs[8]) >>> #define PT_REGS_RET(x) ((x)->regs[31]) >>> #define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with >>> CONFIG_FRAME_POINTER */ >>> -#define PT_REGS_RC(x) ((x)->regs[1]) >>> +#define PT_REGS_RC(x) ((x)->regs[2]) >>> #define PT_REGS_SP(x) ((x)->regs[29]) >>> #define PT_REGS_IP(x) ((x)->cp0_epc) While in process of applying, I noticed that there is one more thing broken; you fixed the PT_REGS_RC() but by that logic at the same time we would also need to fix PT_REGS_RC_CORE() given it still points to regs[1] as well: #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), regs[1]) Please fix up and resubmit. Thanks, Daniel
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h index 58eceb884..ae205dcf8 100644 --- a/tools/lib/bpf/bpf_tracing.h +++ b/tools/lib/bpf/bpf_tracing.h @@ -215,7 +215,7 @@ struct pt_regs; #define PT_REGS_PARM5(x) ((x)->regs[8]) #define PT_REGS_RET(x) ((x)->regs[31]) #define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with CONFIG_FRAME_POINTER */ -#define PT_REGS_RC(x) ((x)->regs[1]) +#define PT_REGS_RC(x) ((x)->regs[2]) #define PT_REGS_SP(x) ((x)->regs[29]) #define PT_REGS_IP(x) ((x)->cp0_epc)
The o32, n32 and n64 calling conventions require the return value to be stored in $v0 which maps to $2 register, i.e., the second register. Fixes: c1932cd ("bpf: Add MIPS support to samples/bpf.") --- tools/lib/bpf/bpf_tracing.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.17.1