[PATCH bpf] libbpf: Fix register in PT_REGS MIPS macros

[PATCH bpf] libbpf: Fix register in PT_REGS MIPS macros

[Jerry Cruntime]

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])
  #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

Re: [PATCH bpf] libbpf: Fix register in PT_REGS MIPS macros

[Andrii Nakryiko]

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

Re: [PATCH bpf] libbpf: Fix register in PT_REGS MIPS macros

[Jerry Cruntime]

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

Re: [PATCH bpf] libbpf: Fix register in PT_REGS MIPS macros

[Andrii Nakryiko]

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

Re: [PATCH bpf] libbpf: Fix register in PT_REGS MIPS macros

[Daniel Borkmann]

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

Re: [PATCH bpf] libbpf: Fix register in PT_REGS MIPS macros

[Jerry Crunchtime]

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
>

Re: [PATCH bpf] libbpf: Fix register in PT_REGS MIPS macros

[Daniel Borkmann]

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