bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: jose.marchesi@oracle.com (Jose E. Marchesi)
To: bpf@vger.kernel.org
Subject: [GCC,LLVM] bpf_helpers.h
Date: Thu, 12 Sep 2019 21:26:57 +0200	[thread overview]
Message-ID: <87lfutgvsu.fsf@oracle.com> (raw)


Hi people!

First of all, many thanks for the lots of feedback I got at the LPC
conference.  It was very useful and made these days totally worth it :)

In order to advance in the direction of having a single bpf_helpers.h
header that works with both llvm and gcc, I would like to suggest a few
changes for the kernel's header.

Kernel helpers
--------------

First, there is the issue of kernel helpers.  You people made it very
clear at LPC that having a compiler built-in function per kernel helper
is way too restrictive, since it makes it impossible to use new kernel
helpers without patching the compiler.  I agree.

However, I still think that the function pointer hack currently used in
bpf_helpers.h is way too fragile, depending on the optimization level
and the particular behavior of the compiler.

Thinking about a more robust and flexible solution, today I wrote a
patch for GCC that adds a target-specific function attribute:

   __attribute__ ((kernel_helper (NUM)))

Then I changed my bpf-helpers.h to define the kernel helpers like:

   void *bpf_map_lookup_elem (void *map, const void *key)
      __attribute__ ((kernel_helper (1)));

This new mechanism allows the user to mark any function prototype as a
kernel helper, so the flexibility is total.  It also allowed me to get
rid of the table of helpers in the GCC backend proper, which is awesome
:)

Would you consider implementing this attribute in llvm and adapt the
kernel's bpf_header accordingly?  In that respect, note that it is
possible to pass enum entries to the attribute (at least in GCC.)  So
you once you implement the attribute, you should be able to do:

   void *bpf_map_lookup_elem (void *map, const void *key)
       __attribute__ ((kernel_helper (BPF_FUNC_map_lookup_elem)));

instead of the current:

   static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
	(void *) BPF_FUNC_map_lookup_elem;

Please let me know what do you think.

SKB load built-ins
------------------

bpf_helpers.h contains the following llvm-isms:

   /* llvm builtin functions that eBPF C program may use to
    * emit BPF_LD_ABS and BPF_LD_IND instructions
    */
   struct sk_buff;
   unsigned long long load_byte(void *skb,
                                unsigned long long off) asm("llvm.bpf.load.byte");
   unsigned long long load_half(void *skb,
			        unsigned long long off) asm("llvm.bpf.load.half");
   unsigned long long load_word(void *skb,
			        unsigned long long off) asm("llvm.bpf.load.word");

Would you consider adopting more standard built-ins in llvm, like I
implemented in GCC?  These are:

   __builtin_bpf_load_byte (unsigned long long off)
   __builtin_bpf_load_half (unsigned long long off)
   __builtin_bpf_load_word (unsigned long long off)

Note that I didn't add an SKB argument to the builtins, as it is not
used: the pointer to the skb is implied by the instructions to be in
some predefined register.  I added compatibility wrappers in my
bpf-helpers.h:

  #define load_byte(SKB,OFF) __builtin_bpf_load_byte ((OFF))
  #define load_half(SKB,OFF) __builtin_bpf_load_half ((OFF))
  #define load_word(SKB,OFF) __builtin_bpf_load_word ((OFF))

Would you consider removing the unused SKB arguments from the built-ins
in llvm?  Or is there a good reason for having them, other than maybe
backwards compatibility?  In case backwards compatibility is a must, I
can add the unused argument to my builtins.

Thanks!

             reply	other threads:[~2019-09-12 19:27 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12 19:26 Jose E. Marchesi [this message]
2019-09-16 16:18 ` Alexei Starovoitov
2019-09-17 13:17   ` Jose E. Marchesi
2019-10-24 16:56 ` initiated discussion to support attribute address_space in clang Yonghong Song
2019-11-04 10:21   ` Jose E. Marchesi
2019-11-07  6:49     ` Yonghong Song

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87lfutgvsu.fsf@oracle.com \
    --to=jose.marchesi@oracle.com \
    --cc=bpf@vger.kernel.org \
    --subject='Re: [GCC,LLVM] bpf_helpers.h' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).