[PATCH] elf.h: Add BPF relocation types.

[PATCH] elf.h: Add BPF relocation types.

[Mark Wielaard]

The BPF ELF format has new relocation types R_BPF_64_64 and R_BPF_64_32.
The existing R_BPF_MAP_FD was an extension that never got implemented.
Remove it, because its constant conflicts with the official R_BPF_64_64.
---
 ChangeLog | 5 +++++
 elf/elf.h | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 9dd87eebb8..40175d254a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2018-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* elf/elf.h (R_BPF_MAP_FD): Removed.
+	(R_BPF_64_64, R_BPF_64_32): New.
+
 2018-06-15  Samuel Thibault  <samuel.thibault@ens-lyon.org>
 
 	* include/sys/sendfile.h (__sendfile64): Declare hidden prototype.
diff --git a/elf/elf.h b/elf/elf.h
index a5b2811ce0..75043bcbf9 100644
--- a/elf/elf.h
+++ b/elf/elf.h
@@ -3850,7 +3850,8 @@ enum
 /* BPF specific declarations.  */
 
 #define R_BPF_NONE		0	/* No reloc */
-#define R_BPF_MAP_FD		1	/* Map fd to pointer */
+#define R_BPF_64_64		1
+#define R_BPF_64_32		10
 
 /* Imagination Meta specific relocations. */
 
-- 
2.17.0

Re: [PATCH] elf.h: Add BPF relocation types.

[Florian Weimer]

On 06/16/2018 11:45 PM, Mark Wielaard wrote:
> The BPF ELF format has new relocation types R_BPF_64_64 and R_BPF_64_32.
> The existing R_BPF_MAP_FD was an extension that never got implemented.
> Remove it, because its constant conflicts with the official R_BPF_64_64.

Is there an ABI manual against which we could review this change?

The last change said that this was added to the Generic ABI 
<http://www.sco.com/developers/gabi/latest/contents.html>, but there is 
no evidence of that.

Thanks,
Florian

Re: [PATCH] elf.h: Add BPF relocation types.

[Mark Wielaard]

On Tue, 2018-06-19 at 22:06 +0200, Florian Weimer wrote:
> On 06/16/2018 11:45 PM, Mark Wielaard wrote:
> > The BPF ELF format has new relocation types R_BPF_64_64 and R_BPF_64_32.
> > The existing R_BPF_MAP_FD was an extension that never got implemented.
> > Remove it, because its constant conflicts with the official R_BPF_64_64.
> 
> Is there an ABI manual against which we could review this change?
> 
> The last change said that this was added to the Generic ABI 
> > <http://www.sco.com/developers/gabi/latest/contents.html>, but there is 
> no evidence of that.

Only the EM values are, since they are generic.
It just takes a while before they make it to the public website.
rth has an email from the maintainer though with the assigned value.

The relocation constants don't have a separate ABI manual
(because BPF isn't really a full blown architecture/abi).
Those are kept in sync between the projects handling BPF elf files
(which is this patch, which is a prerequisite for getting the same
constants into elfutils, because we see the glibc elf.h as the
master copy that holds all GNU/Linux ELF constants.)

Thanks,

Mark

Re: [PATCH] elf.h: Add BPF relocation types.

[Yonghong Song]

On 6/19/18 1:34 PM, Mark Wielaard wrote:
> On Tue, 2018-06-19 at 22:06 +0200, Florian Weimer wrote:
>> On 06/16/2018 11:45 PM, Mark Wielaard wrote:
>>> The BPF ELF format has new relocation types R_BPF_64_64 and R_BPF_64_32.
>>> The existing R_BPF_MAP_FD was an extension that never got implemented.
>>> Remove it, because its constant conflicts with the official R_BPF_64_64.
>>
>> Is there an ABI manual against which we could review this change?
>>
>> The last change said that this was added to the Generic ABI
>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.sco.com_developers_gabi_latest_contents.html&d=DwIFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=vat1LaeLAQ_XbOs1SRdavOCV310jx4Ctku_6Jcq72HY&s=r5vE3J-yD5ebnpdq_d0AkFfNFzewdpXShVKSvBULDzc&e=>, but there is
>> no evidence of that.
> 
> Only the EM values are, since they are generic.
> It just takes a while before they make it to the public website.
> rth has an email from the maintainer though with the assigned value.
> 
> The relocation constants don't have a separate ABI manual
> (because BPF isn't really a full blown architecture/abi).
> Those are kept in sync between the projects handling BPF elf files
> (which is this patch, which is a prerequisite for getting the same
> constants into elfutils, because we see the glibc elf.h as the
> master copy that holds all GNU/Linux ELF constants.)

The LLVM source code for the definition is here:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/BinaryFormat/ELFRelocs/BPF.def

Yes, BPF related documentation has been lacking. For example,
the support in binutils is not there yet. But we are starting
to work on it.

Thanks!

Yonghong

> 
> Thanks,
> 
> Mark
> 

Re: [PATCH] elf.h: Add BPF relocation types.

[Florian Weimer]

On 06/16/2018 11:45 PM, Mark Wielaard wrote:

> diff --git a/elf/elf.h b/elf/elf.h
> index a5b2811ce0..75043bcbf9 100644
> --- a/elf/elf.h
> +++ b/elf/elf.h
> @@ -3850,7 +3850,8 @@ enum
>   /* BPF specific declarations.  */
>   
>   #define R_BPF_NONE		0	/* No reloc */
> -#define R_BPF_MAP_FD		1	/* Map fd to pointer */
> +#define R_BPF_64_64		1
> +#define R_BPF_64_32		10
>   
>   /* Imagination Meta specific relocations. */

This still needs review from someone experienced with <elf.h> updates.

Thanks,
Florian

Re: [PATCH] elf.h: Add BPF relocation types.

[Carlos O'Donell]

On 06/16/2018 05:45 PM, Mark Wielaard wrote:
> The BPF ELF format has new relocation types R_BPF_64_64 and R_BPF_64_32.
> The existing R_BPF_MAP_FD was an extension that never got implemented.
> Remove it, because its constant conflicts with the official R_BPF_64_64.
> ---
>  ChangeLog | 5 +++++
>  elf/elf.h | 3 ++-
>  2 files changed, 7 insertions(+), 1 deletion(-)

I trust you that R_BPF_MAP_FD was never implemented, but I consider it
bad engineering to publish and then reuse a constant, even if you might
argue the entire pseudo-architecture is "unstable."

Having said that we should align the values here with the values being used
by llvm and Facebook. They represent the most recent values being used for
relocations in this pseudo-machine. Without an officially published ABI for
these relocations we have no real way to review what is canonical and what
is not.

I don't see any objections from Florian Weimer (the other reviewer), just
the same frowning face I have :/ at the lack of a proper engineering
specification.

OK to checkin.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/ChangeLog b/ChangeLog
> index 9dd87eebb8..40175d254a 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2018-06-16  Mark Wielaard  <mark@klomp.org>
> +
> +	* elf/elf.h (R_BPF_MAP_FD): Removed.
> +	(R_BPF_64_64, R_BPF_64_32): New.
> +
>  2018-06-15  Samuel Thibault  <samuel.thibault@ens-lyon.org>
>  
>  	* include/sys/sendfile.h (__sendfile64): Declare hidden prototype.
> diff --git a/elf/elf.h b/elf/elf.h
> index a5b2811ce0..75043bcbf9 100644
> --- a/elf/elf.h
> +++ b/elf/elf.h
> @@ -3850,7 +3850,8 @@ enum
>  /* BPF specific declarations.  */
>  
>  #define R_BPF_NONE		0	/* No reloc */
> -#define R_BPF_MAP_FD		1	/* Map fd to pointer */
> +#define R_BPF_64_64		1
> +#define R_BPF_64_32		10

This matches what is in LLVM upstream (not the git mirror).

Changed by this:

commit db476dbf2ee03f8ebdacc439bdde1cf4f0ee629a
Author: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Mon Nov 21 06:21:23 2016 +0000

    [bpf] fix dwarf elf relocs and line numbers
    
    - teach RelocVisitor to recognize bpf relocations
    - fix AsmInfo->PointerSize to make sure dwarf is emitted correctly
    - add a test for the above
    
    Signed-off-by: Alexei Starovoitov <ast@kernel.org>
    
    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@287521 91177308-0d34-0410-b5e6-96231b3b80d8

>  
>  /* Imagination Meta specific relocations. */
>  
> 

Re: [PATCH] elf.h: Add BPF relocation types.

[Mark Wielaard]

On Thu, Jun 21, 2018 at 11:29:26AM -0400, Carlos O'Donell wrote:
> Having said that we should align the values here with the values being used
> by llvm and Facebook. They represent the most recent values being used for
> relocations in this pseudo-machine. Without an officially published ABI for
> these relocations we have no real way to review what is canonical and what
> is not.

Right, that is really what this is commit is for.
To make sure that we have agreement over the constants used without
having a central authority for this "architecture".
 
> OK to checkin.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Thanks, pushed.

Mark