On Fri, Mar 11, 2022 at 10:37 AM Alan Jowett <Alan.Jowett@xxxxxxxxxxxxx> wrote:
>
> Thanks for the feedback, I appreciate it. Any suggestions on how to make the output more visually appealing?
>
> If there was a IANA equivalent for assigning helper function id's then it might be possible to support having the same IDs on multiple platforms, but as it exists today the various platforms are developed separately, with no simple way to co-ordinate. The benefit of using symbols over numeric id's is that it reduces the possibility of identifier collision, where different platforms might define helper ID # to mean different things, but it's less likely that different platforms will define bpf_foo_bar to have different semantics.
>
> Conceptually, this is like the relocations done using BTF data for CORE, where the offsets into structures can vary depending on the kernel version. In this scenario the helper IDs can vary with the relocation being done based on symbols stored in the ELF file.
Please don't top post, reply inline instead.
We have a BPF Foundation and BPF Steering Committee for coordinating
things like this across different platforms. Adding extra relocations
just for BPF helper IDs seems to go in the wrong direction from
minimizing the amount of runtime BPF instruction adjustments (to
simplify things like BPF signing, etc).
>
> Regards,
> Alan Jowett
>
> -----Original Message-----
> From: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> Sent: Tuesday, March 8, 2022 4:49 PM
> To: Alan Jowett <Alan.Jowett@xxxxxxxxxxxxx>
> Cc: bpf@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH bpf-next] Support load time binding of bpf-helpers.
>
> On Tue, Mar 8, 2022 at 9:20 AM Alan Jowett <Alan.Jowett@xxxxxxxxxxxxx> wrote:
> >
> > BPF helper function names are common across different platforms, but
> > the id assigned to helper functions may vary. This change provides an
> > option to generate eBPF ELF files with relocations for helper
> > functions which permits resolving helper function names to ids at load
> > time instead of at compile time.
> >
> > Add a level of indirection to bpf_doc.py (and generated
> > bpf_helper_defs.h) to permit compile time generation of bpf-helper
> > function declarations to facilitating late binding of bpf-helpers to
> > helper id for cases where different platforms use different helper
> > ids, but the same helper functions.
> >
> > Example use case would be:
> > "#define BPF_HELPER(return_type, name, args, id) \
> > extern return_type name args"
> >
> > To generate:
> > extern void * bpf_map_lookup_elem (void *map, const void *key);
> >
> > Instead of:
> > static void *(*bpf_map_lookup_elem) (void *map, const void *key) \
> > = (void*) 1;
> >
> > This would result in the bpf-helpers having external linkage and
> > permit late binding of BPF programs to helper function ids.
> >
>
> Ugh...
>
> BPF_HELPER(void*, bpf_map_lookup_elem, (void *map, const void *key), 1);
>
> Looks pretty awful :(
>
> But I also have the question about why different platforms should have different IDs for the same subset of BPF helpers? Wouldn't it be better to preserve IDs across platforms to simplify cross-platform BPF object files?
>
> We can probably also define some range for platform-specific BPF helpers starting from some high ID?
>
> > Signed-off-by: Alan Jowett <alanjo@xxxxxxxxxxxxx>
> > ---
> > scripts/bpf_helpers_doc.py | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
> > index 867ada23281c..442b5e87687e 100755
> > --- a/scripts/bpf_helpers_doc.py
> > +++ b/scripts/bpf_helpers_doc.py
> > @@ -519,6 +519,10 @@ class PrinterHelpers(Printer):
> > for fwd in self.type_fwds:
> > print('%s;' % fwd)
> > print('')
> > + print('#ifndef BPF_HELPER')
> > + print('#define BPF_HELPER(return_type, name, args, id) static return_type(*name) args = (void*) id')
> > + print('#endif')
> > + print('')
> >
> > def print_footer(self):
> > footer = ''
> > @@ -558,7 +562,7 @@ class PrinterHelpers(Printer):
> > print(' *{}{}'.format(' \t' if line else '', line))
> >
> > print(' */')
> > - print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
> > + print('BPF_HELPER(%s%s, %s, (' %
> > + (self.map_type(proto['ret_type']),
> > proto['ret_star'], proto['name']), end='')
> > comma = ''
> > for i, a in enumerate(proto['args']):
> > @@ -577,7 +581,7 @@ class PrinterHelpers(Printer):
> > comma = ', '
> > print(one_arg, end='')
> >
> > - print(') = (void *) %d;' % len(self.seen_helpers))
> > + print('), %d);' % len(self.seen_helpers))
> > print('')
> >
> >
> > ######################################################################
> > #########
> > --
> > 2.25.1
