diff mbox series

[bpf-next] bpf, docs: Add extended call instructions

Message ID 20230310232144.4077-1-dthaler1968@googlemail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] bpf, docs: Add extended call instructions | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 20 this patch: 20
netdev/cc_maintainers warning 14 maintainers not CCed: void@manifault.com andrii@kernel.org song@kernel.org sdf@google.com corbet@lwn.net haoluo@google.com yhs@fb.com linux-doc@vger.kernel.org daniel@iogearbox.net john.fastabend@gmail.com kpsingh@kernel.org jolsa@kernel.org martin.lau@linux.dev ast@kernel.org
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 20 this patch: 20
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 62 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-22 fail Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for test_progs on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 fail Logs for test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-19 fail Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 fail Logs for test_progs_no_alu32 on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-23 fail Logs for test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_progs_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_verifier on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-11 fail Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 fail Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for test_progs on s390x with gcc

Commit Message

Dave Thaler March 10, 2023, 11:21 p.m. UTC
From: Dave Thaler <dthaler@microsoft.com>

Add extended call instructions.  Since BPF can be used in userland
or SmartNICs, this uses the more generic "runtime functions"
rather than the kernel specific "kfuncs" as a term.

Signed-off-by: Dave Thaler <dthaler@microsoft.com>
---
 Documentation/bpf/instruction-set.rst | 50 +++++++++++++++++----------
 1 file changed, 32 insertions(+), 18 deletions(-)

Comments

David Vernet March 11, 2023, 7:21 p.m. UTC | #1
On Fri, Mar 10, 2023 at 11:21:44PM +0000, Dave Thaler wrote:
> From: Dave Thaler <dthaler@microsoft.com>
> 
> Add extended call instructions.  Since BPF can be used in userland
> or SmartNICs, this uses the more generic "runtime functions"
> rather than the kernel specific "kfuncs" as a term.
> 
> Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Hi Dave,

Thanks for sending out the patch. It's a nice improvement to the doc to
disambiguate these instruction call types. Left a few comments below.

> ---
>  Documentation/bpf/instruction-set.rst | 50 +++++++++++++++++----------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst
> index 5e43e14abe8..bc2319a7707 100644
> --- a/Documentation/bpf/instruction-set.rst
> +++ b/Documentation/bpf/instruction-set.rst
> @@ -242,24 +242,26 @@ Jump instructions
>  otherwise identical operations.
>  The 'code' field encodes the operation as below:
>  
> -========  =====  =========================  ============
> -code      value  description                notes
> -========  =====  =========================  ============
> -BPF_JA    0x00   PC += off                  BPF_JMP only
> -BPF_JEQ   0x10   PC += off if dst == src
> -BPF_JGT   0x20   PC += off if dst > src     unsigned
> -BPF_JGE   0x30   PC += off if dst >= src    unsigned
> -BPF_JSET  0x40   PC += off if dst & src
> -BPF_JNE   0x50   PC += off if dst != src
> -BPF_JSGT  0x60   PC += off if dst > src     signed
> -BPF_JSGE  0x70   PC += off if dst >= src    signed
> -BPF_CALL  0x80   function call              see `Helper functions`_
> -BPF_EXIT  0x90   function / program return  BPF_JMP only
> -BPF_JLT   0xa0   PC += off if dst < src     unsigned
> -BPF_JLE   0xb0   PC += off if dst <= src    unsigned
> -BPF_JSLT  0xc0   PC += off if dst < src     signed
> -BPF_JSLE  0xd0   PC += off if dst <= src    signed
> -========  =====  =========================  ============
> +========  =====  ===  ==========================  ========================
> +code      value  src  description                 notes
> +========  =====  ===  ==========================  ========================
> +BPF_JA    0x0    0x0  PC += offset                BPF_JMP only
> +BPF_JEQ   0x1    any  PC += offset if dst == src
> +BPF_JGT   0x2    any  PC += offset if dst > src   unsigned
> +BPF_JGE   0x3    any  PC += offset if dst >= src  unsigned
> +BPF_JSET  0x4    any  PC += offset if dst & src
> +BPF_JNE   0x5    any  PC += offset if dst != src
> +BPF_JSGT  0x6    any  PC += offset if dst > src   signed
> +BPF_JSGE  0x7    any  PC += offset if dst >= src  signed
> +BPF_CALL  0x8    0x0  call helper function imm    see `Helper functions`_
> +BPF_CALL  0x8    0x1  call PC += offset           see `eBPF functions`_
> +BPF_CALL  0x8    0x2  call runtime function imm   see `Runtime functions`_

The names "Helper functions", "eBPF functions", and "Runtime functions"
feel, for lack of a better term, insufficiently distinct. I realize that
it's very tricky to get the naming right here given that some of these
terms (helpers + runtime functions) are conceptually the same thing, but
as a reader with no background I expect I'd be confused by what the
distinctions are as they all kind of sound like the same thing. What do
you think of this as an alternative:

1. Standard helper functions
2. BPF-local functions
3. Platform-specific helper functions

The idea with the latter is of course that the platform can choose to
implement whatever helper functions (kfuncs for Linux) apply exclusively
to that platform. I think we'd need some formal encoding for this in the
standard, so it seems appropriate to apply it here. What do you think?

> +BPF_EXIT  0x9    0x0  return                      BPF_JMP only
> +BPF_JLT   0xa    any  PC += offset if dst < src   unsigned
> +BPF_JLE   0xb    any  PC += offset if dst <= src  unsigned
> +BPF_JSLT  0xc    any  PC += offset if dst < src   signed
> +BPF_JSLE  0xd    any  PC += offset if dst <= src  signed
> +========  =====  ===  ==========================  ========================
>  
>  The eBPF program needs to store the return value into register R0 before doing a
>  BPF_EXIT.
> @@ -272,6 +274,18 @@ set of function calls exposed by the runtime.  Each helper
>  function is identified by an integer used in a ``BPF_CALL`` instruction.
>  The available helper functions may differ for each program type.
>  
> +Runtime functions
> +~~~~~~~~~~~~~~~~~
> +Runtime functions are like helper functions except that they are not specific
> +to eBPF programs.  They use a different numbering space from helper functions,

Per my suggestion above, should we rephrase this as "platform-specific"
helper functions? E.g. something like:

Platform-specific helper functions are helper functions that may be
unique to a particular platform. An encoding for a platform-specific
function on one platform may or may not correspond to the same function
on another platform. Platforms are not required to implement any
platform-specific function.

As alluded to above, the fact that they're not specific to BPF seems
like an implementation detail from the perspective of the encoding /
standard. Thoughts?

> +but otherwise the same considerations apply.
> +
> +eBPF functions
> +~~~~~~~~~~~~~~
> +eBPF functions are functions exposed by the same eBPF program as the caller,
> +and are referenced by offset from the call instruction, similar to ``BPF_JA``.
> +A ``BPF_EXIT`` within the eBPF function will return to the caller.

Suggestion: Can we simply say BPF instead of eBPF? At this point I'm not
sure what the 'e' distinction really buys us, though I'm sure I'm
missing context from (many) prior discussions. I also don't want to
bikeshed too much on this point for your patch, so if it becomes a
"thing" then feel free to ignore.

Thanks,
David
Dave Thaler March 11, 2023, 9 p.m. UTC | #2
David Vernet <void@manifault.com> wrote:
[...]
> > +BPF_CALL  0x8    0x0  call helper function imm    see `Helper functions`_
> > +BPF_CALL  0x8    0x1  call PC += offset           see `eBPF functions`_
> > +BPF_CALL  0x8    0x2  call runtime function imm   see `Runtime functions`_
> 
> The names "Helper functions", "eBPF functions", and "Runtime functions"
> feel, for lack of a better term, insufficiently distinct. I realize that it's very tricky
> to get the naming right here given that some of these terms (helpers +
> runtime functions) are conceptually the same thing, but as a reader with no
> background I expect I'd be confused by what the distinctions are as they all
> kind of sound like the same thing. What do you think of this as an alternative:
> 
> 1. Standard helper functions
> 2. BPF-local functions
> 3. Platform-specific helper functions

I like where you're going with this.  However, the fact is that some of the existing
Helper functions are actually very platform-specific and won't apply to other
platforms. So retroactively labeling all of them "standard" is somewhat problematic
in my view.

I do like the idea of using "<some adjective> helper functions" for both 1 and 3
though.  Since we might not choose to standardize all the existing type 1 functions,
maybe "Platform-agnostic helper functions" is synonymous and pairs nicely
With "Platform-specific helper functions" as a term.  And then we could just have
a note in the linux-notes.rst saying Linux has some platform-specific helper functions that for historical reasons are used with the platform-agnostic helper function
Instruction.

> The idea with the latter is of course that the platform can choose to
> implement whatever helper functions (kfuncs for Linux) apply exclusively to
> that platform. I think we'd need some formal encoding for this in the
> standard, so it seems appropriate to apply it here. What do you think?

Agree with that.

> > +BPF_EXIT  0x9    0x0  return                      BPF_JMP only
> > +BPF_JLT   0xa    any  PC += offset if dst < src   unsigned
> > +BPF_JLE   0xb    any  PC += offset if dst <= src  unsigned
> > +BPF_JSLT  0xc    any  PC += offset if dst < src   signed
> > +BPF_JSLE  0xd    any  PC += offset if dst <= src  signed
> > +========  =====  ===  ==========================
> > +========================
> >
> >  The eBPF program needs to store the return value into register R0
> > before doing a  BPF_EXIT.
> > @@ -272,6 +274,18 @@ set of function calls exposed by the runtime.
> > Each helper  function is identified by an integer used in a ``BPF_CALL``
> instruction.
> >  The available helper functions may differ for each program type.
> >
> > +Runtime functions
> > +~~~~~~~~~~~~~~~~~
> > +Runtime functions are like helper functions except that they are not
> > +specific to eBPF programs.  They use a different numbering space from
> > +helper functions,
> 
> Per my suggestion above, should we rephrase this as "platform-specific"
> helper functions? E.g. something like:
> 
> Platform-specific helper functions are helper functions that may be unique to
> a particular platform. An encoding for a platform-specific function on one
> platform may or may not correspond to the same function on another
> platform. Platforms are not required to implement any platform-specific
> function.

That looks good to me, will incorporate.

> 
> As alluded to above, the fact that they're not specific to BPF seems like an
> implementation detail from the perspective of the encoding / standard.
> Thoughts?
> 
> > +but otherwise the same considerations apply.
> > +
> > +eBPF functions
> > +~~~~~~~~~~~~~~
> > +eBPF functions are functions exposed by the same eBPF program as the
> > +caller, and are referenced by offset from the call instruction, similar to
> ``BPF_JA``.
> > +A ``BPF_EXIT`` within the eBPF function will return to the caller.
> 
> Suggestion: Can we simply say BPF instead of eBPF? At this point I'm not sure
> what the 'e' distinction really buys us, though I'm sure I'm missing context
> from (many) prior discussions. I also don't want to bikeshed too much on this
> point for your patch, so if it becomes a "thing" then feel free to ignore.

Will remove for consistency with the other patches I submitted that already
omitted the "e".   I think Alexei had the same comment a while back and
I missed updating this proposed section at the time.  Thanks.

Dave

> 
> Thanks,
> David
David Vernet March 11, 2023, 9:53 p.m. UTC | #3
On Sat, Mar 11, 2023 at 09:00:19PM +0000, Dave Thaler wrote:
> David Vernet <void@manifault.com> wrote:
> [...]
> > > +BPF_CALL  0x8    0x0  call helper function imm    see `Helper functions`_
> > > +BPF_CALL  0x8    0x1  call PC += offset           see `eBPF functions`_
> > > +BPF_CALL  0x8    0x2  call runtime function imm   see `Runtime functions`_
> > 
> > The names "Helper functions", "eBPF functions", and "Runtime functions"
> > feel, for lack of a better term, insufficiently distinct. I realize that it's very tricky
> > to get the naming right here given that some of these terms (helpers +
> > runtime functions) are conceptually the same thing, but as a reader with no
> > background I expect I'd be confused by what the distinctions are as they all
> > kind of sound like the same thing. What do you think of this as an alternative:
> > 
> > 1. Standard helper functions
> > 2. BPF-local functions
> > 3. Platform-specific helper functions
> 
> I like where you're going with this.  However, the fact is that some of the existing
> Helper functions are actually very platform-specific and won't apply to other
> platforms. So retroactively labeling all of them "standard" is somewhat problematic
> in my view.

That makes sense. For what it's worth, I was envisioning us specifying
both the helper number (and likely the semantics of those helpers) in
the standard, and just skipping over any which are Linux-specific.
That's of course assuming we do decide to include functions in the
standard, which to my understanding is not yet finalized.

Regardless, I'll certainly defer to your expertise on when it's
appropriate to use the word "standard", and I could see why it would be
problematic to do so here.

> 
> I do like the idea of using "<some adjective> helper functions" for both 1 and 3
> though.  Since we might not choose to standardize all the existing type 1 functions,
> maybe "Platform-agnostic helper functions" is synonymous and pairs nicely
> With "Platform-specific helper functions" as a term.  And then we could just have
> a note in the linux-notes.rst saying Linux has some platform-specific helper functions that for historical reasons are used with the platform-agnostic helper function
> Instruction.

That's a reasonable option as well. The only thing that gives me pause
is that, as you know, the plan of record for now in Linux is to have all
new BPF -> main kernel functions added as kfuncs. That includes features
which are "platform agnostic", such as BPF iterators. I know you've
previously raised the idea of having the traditional helpers be used as
standard / platform-agnostic helpers in BPF office hours, so this isn't
out of the blue. It seems that the time has come to discuss it more
concretely.

> 
> > The idea with the latter is of course that the platform can choose to
> > implement whatever helper functions (kfuncs for Linux) apply exclusively to
> > that platform. I think we'd need some formal encoding for this in the
> > standard, so it seems appropriate to apply it here. What do you think?
> 
> Agree with that.
> 
> > > +BPF_EXIT  0x9    0x0  return                      BPF_JMP only
> > > +BPF_JLT   0xa    any  PC += offset if dst < src   unsigned
> > > +BPF_JLE   0xb    any  PC += offset if dst <= src  unsigned
> > > +BPF_JSLT  0xc    any  PC += offset if dst < src   signed
> > > +BPF_JSLE  0xd    any  PC += offset if dst <= src  signed
> > > +========  =====  ===  ==========================
> > > +========================
> > >
> > >  The eBPF program needs to store the return value into register R0
> > > before doing a  BPF_EXIT.
> > > @@ -272,6 +274,18 @@ set of function calls exposed by the runtime.
> > > Each helper  function is identified by an integer used in a ``BPF_CALL``
> > instruction.
> > >  The available helper functions may differ for each program type.
> > >
> > > +Runtime functions
> > > +~~~~~~~~~~~~~~~~~
> > > +Runtime functions are like helper functions except that they are not
> > > +specific to eBPF programs.  They use a different numbering space from
> > > +helper functions,
> > 
> > Per my suggestion above, should we rephrase this as "platform-specific"
> > helper functions? E.g. something like:
> > 
> > Platform-specific helper functions are helper functions that may be unique to
> > a particular platform. An encoding for a platform-specific function on one
> > platform may or may not correspond to the same function on another
> > platform. Platforms are not required to implement any platform-specific
> > function.
> 
> That looks good to me, will incorporate.
> 
> > 
> > As alluded to above, the fact that they're not specific to BPF seems like an
> > implementation detail from the perspective of the encoding / standard.
> > Thoughts?
> > 
> > > +but otherwise the same considerations apply.
> > > +
> > > +eBPF functions
> > > +~~~~~~~~~~~~~~
> > > +eBPF functions are functions exposed by the same eBPF program as the
> > > +caller, and are referenced by offset from the call instruction, similar to
> > ``BPF_JA``.
> > > +A ``BPF_EXIT`` within the eBPF function will return to the caller.
> > 
> > Suggestion: Can we simply say BPF instead of eBPF? At this point I'm not sure
> > what the 'e' distinction really buys us, though I'm sure I'm missing context
> > from (many) prior discussions. I also don't want to bikeshed too much on this
> > point for your patch, so if it becomes a "thing" then feel free to ignore.
> 
> Will remove for consistency with the other patches I submitted that already
> omitted the "e".   I think Alexei had the same comment a while back and
> I missed updating this proposed section at the time.  Thanks.
> 
> Dave
> 
> > 
> > Thanks,
> > David
Dave Thaler March 11, 2023, 10:06 p.m. UTC | #4
David Vernet <void@manifault.com> wrote: 
> On Sat, Mar 11, 2023 at 09:00:19PM +0000, Dave Thaler wrote:
> > David Vernet <void@manifault.com> wrote:
> > [...]
> > > > +BPF_CALL  0x8    0x0  call helper function imm    see `Helper functions`_
> > > > +BPF_CALL  0x8    0x1  call PC += offset           see `eBPF functions`_
> > > > +BPF_CALL  0x8    0x2  call runtime function imm   see `Runtime
> functions`_
> > >
> > > The names "Helper functions", "eBPF functions", and "Runtime functions"
> > > feel, for lack of a better term, insufficiently distinct. I realize
> > > that it's very tricky to get the naming right here given that some
> > > of these terms (helpers + runtime functions) are conceptually the
> > > same thing, but as a reader with no background I expect I'd be
> > > confused by what the distinctions are as they all kind of sound like the
> same thing. What do you think of this as an alternative:
> > >
> > > 1. Standard helper functions
> > > 2. BPF-local functions
> > > 3. Platform-specific helper functions
> >
> > I like where you're going with this.  However, the fact is that some
> > of the existing Helper functions are actually very platform-specific
> > and won't apply to other platforms. So retroactively labeling all of
> > them "standard" is somewhat problematic in my view.
> 
> That makes sense. For what it's worth, I was envisioning us specifying both
> the helper number (and likely the semantics of those helpers) in the standard,
> and just skipping over any which are Linux-specific.

Outside the scope of this patch per set, but...
FYI, in ebpf-for-windows, we do not currently have a goal to use the same integer
numbers as Linux has, only the same prototypes.  If there is a strong technical
reason to do so, it can be considered, but right now I don't see any need
to standardize the actual integer values.   We claim BPF program source code
compatibility, not byte code compatibility at present.  But if the standardization
effort does see a need to standardize integer values, we will accommodate.

> That's of course assuming we do decide to include functions in the standard,
> which to my understanding is not yet finalized.

Platform-agnostic helper functions are on the proposed list of things to
standardize and for ebpf-for-windows, we do want to standardize a bunch
of them that are now common between Linux and Windows and in my view
make sense for other platforms too.

> Regardless, I'll certainly defer to your expertise on when it's appropriate to
> use the word "standard", and I could see why it would be problematic to do
> so here.
> 
> > I do like the idea of using "<some adjective> helper functions" for
> > both 1 and 3 though.  Since we might not choose to standardize all the
> > existing type 1 functions, maybe "Platform-agnostic helper functions"
> > is synonymous and pairs nicely With "Platform-specific helper
> > functions" as a term.  And then we could just have a note in the
> > linux-notes.rst saying Linux has some platform-specific helper functions that
> for historical reasons are used with the platform-agnostic helper function
> Instruction.
> 
> That's a reasonable option as well. The only thing that gives me pause is that,
> as you know, the plan of record for now in Linux is to have all new BPF ->
> main kernel functions added as kfuncs. That includes features which are
> "platform agnostic", such as BPF iterators. I know you've previously raised the
> idea of having the traditional helpers be used as standard / platform-agnostic
> helpers in BPF office hours, so this isn't out of the blue. It seems that the time
> has come to discuss it more concretely.
[...]

Yes, my view which I have expressed in the office hours meetings, is that Linux
can do so.   But when the time comes to standardize something cross-platform
(platform-agnostic), then it gets an integer out of the platform-agnostic space.
That means at that point it's not a kfunc, but a classic helper function.  But
they can start as kfuncs in Linux and do that once standardization is done,
potentially having an integer in both numbering spaces if a breaking change
is undesired.  Other platforms, like Windows, can do the same thing with their
platform-specific helper function if one later becomes a platform-agnostic
standard.   I don't think this affects this patchset right now though, but
may affect future ones.

-Dave
David Vernet March 11, 2023, 10:07 p.m. UTC | #5
On Sat, Mar 11, 2023 at 03:53:47PM -0600, David Vernet wrote:
> On Sat, Mar 11, 2023 at 09:00:19PM +0000, Dave Thaler wrote:
> > David Vernet <void@manifault.com> wrote:
> > [...]
> > > > +BPF_CALL  0x8    0x0  call helper function imm    see `Helper functions`_
> > > > +BPF_CALL  0x8    0x1  call PC += offset           see `eBPF functions`_
> > > > +BPF_CALL  0x8    0x2  call runtime function imm   see `Runtime functions`_
> > > 
> > > The names "Helper functions", "eBPF functions", and "Runtime functions"
> > > feel, for lack of a better term, insufficiently distinct. I realize that it's very tricky
> > > to get the naming right here given that some of these terms (helpers +
> > > runtime functions) are conceptually the same thing, but as a reader with no
> > > background I expect I'd be confused by what the distinctions are as they all
> > > kind of sound like the same thing. What do you think of this as an alternative:
> > > 
> > > 1. Standard helper functions
> > > 2. BPF-local functions
> > > 3. Platform-specific helper functions
> > 
> > I like where you're going with this.  However, the fact is that some of the existing
> > Helper functions are actually very platform-specific and won't apply to other
> > platforms. So retroactively labeling all of them "standard" is somewhat problematic
> > in my view.
> 
> That makes sense. For what it's worth, I was envisioning us specifying
> both the helper number (and likely the semantics of those helpers) in
> the standard, and just skipping over any which are Linux-specific.
> That's of course assuming we do decide to include functions in the
> standard, which to my understanding is not yet finalized.
> 
> Regardless, I'll certainly defer to your expertise on when it's
> appropriate to use the word "standard", and I could see why it would be
> problematic to do so here.
> 
> > 
> > I do like the idea of using "<some adjective> helper functions" for both 1 and 3
> > though.  Since we might not choose to standardize all the existing type 1 functions,
> > maybe "Platform-agnostic helper functions" is synonymous and pairs nicely
> > With "Platform-specific helper functions" as a term.  And then we could just have
> > a note in the linux-notes.rst saying Linux has some platform-specific helper functions that for historical reasons are used with the platform-agnostic helper function
> > Instruction.
> 
> That's a reasonable option as well. The only thing that gives me pause
> is that, as you know, the plan of record for now in Linux is to have all
> new BPF -> main kernel functions added as kfuncs. That includes features
> which are "platform agnostic", such as BPF iterators. I know you've
> previously raised the idea of having the traditional helpers be used as
> standard / platform-agnostic helpers in BPF office hours, so this isn't
> out of the blue. It seems that the time has come to discuss it more
> concretely.

One thing to clarify -- I'm _not_ saying we should revisit the kfunc vs.
BPF helper discussion. Rather, just that we should decide exactly what
the older BPF helper instruction encoding means in terms of a generic
BPF instruction set.

> > 
> > > The idea with the latter is of course that the platform can choose to
> > > implement whatever helper functions (kfuncs for Linux) apply exclusively to
> > > that platform. I think we'd need some formal encoding for this in the
> > > standard, so it seems appropriate to apply it here. What do you think?
> > 
> > Agree with that.
> > 
> > > > +BPF_EXIT  0x9    0x0  return                      BPF_JMP only
> > > > +BPF_JLT   0xa    any  PC += offset if dst < src   unsigned
> > > > +BPF_JLE   0xb    any  PC += offset if dst <= src  unsigned
> > > > +BPF_JSLT  0xc    any  PC += offset if dst < src   signed
> > > > +BPF_JSLE  0xd    any  PC += offset if dst <= src  signed
> > > > +========  =====  ===  ==========================
> > > > +========================
> > > >
> > > >  The eBPF program needs to store the return value into register R0
> > > > before doing a  BPF_EXIT.
> > > > @@ -272,6 +274,18 @@ set of function calls exposed by the runtime.
> > > > Each helper  function is identified by an integer used in a ``BPF_CALL``
> > > instruction.
> > > >  The available helper functions may differ for each program type.
> > > >
> > > > +Runtime functions
> > > > +~~~~~~~~~~~~~~~~~
> > > > +Runtime functions are like helper functions except that they are not
> > > > +specific to eBPF programs.  They use a different numbering space from
> > > > +helper functions,
> > > 
> > > Per my suggestion above, should we rephrase this as "platform-specific"
> > > helper functions? E.g. something like:
> > > 
> > > Platform-specific helper functions are helper functions that may be unique to
> > > a particular platform. An encoding for a platform-specific function on one
> > > platform may or may not correspond to the same function on another
> > > platform. Platforms are not required to implement any platform-specific
> > > function.
> > 
> > That looks good to me, will incorporate.
> > 
> > > 
> > > As alluded to above, the fact that they're not specific to BPF seems like an
> > > implementation detail from the perspective of the encoding / standard.
> > > Thoughts?
> > > 
> > > > +but otherwise the same considerations apply.
> > > > +
> > > > +eBPF functions
> > > > +~~~~~~~~~~~~~~
> > > > +eBPF functions are functions exposed by the same eBPF program as the
> > > > +caller, and are referenced by offset from the call instruction, similar to
> > > ``BPF_JA``.
> > > > +A ``BPF_EXIT`` within the eBPF function will return to the caller.
> > > 
> > > Suggestion: Can we simply say BPF instead of eBPF? At this point I'm not sure
> > > what the 'e' distinction really buys us, though I'm sure I'm missing context
> > > from (many) prior discussions. I also don't want to bikeshed too much on this
> > > point for your patch, so if it becomes a "thing" then feel free to ignore.
> > 
> > Will remove for consistency with the other patches I submitted that already
> > omitted the "e".   I think Alexei had the same comment a while back and
> > I missed updating this proposed section at the time.  Thanks.
> > 
> > Dave
> > 
> > > 
> > > Thanks,
> > > David
diff mbox series

Patch

diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst
index 5e43e14abe8..bc2319a7707 100644
--- a/Documentation/bpf/instruction-set.rst
+++ b/Documentation/bpf/instruction-set.rst
@@ -242,24 +242,26 @@  Jump instructions
 otherwise identical operations.
 The 'code' field encodes the operation as below:
 
-========  =====  =========================  ============
-code      value  description                notes
-========  =====  =========================  ============
-BPF_JA    0x00   PC += off                  BPF_JMP only
-BPF_JEQ   0x10   PC += off if dst == src
-BPF_JGT   0x20   PC += off if dst > src     unsigned
-BPF_JGE   0x30   PC += off if dst >= src    unsigned
-BPF_JSET  0x40   PC += off if dst & src
-BPF_JNE   0x50   PC += off if dst != src
-BPF_JSGT  0x60   PC += off if dst > src     signed
-BPF_JSGE  0x70   PC += off if dst >= src    signed
-BPF_CALL  0x80   function call              see `Helper functions`_
-BPF_EXIT  0x90   function / program return  BPF_JMP only
-BPF_JLT   0xa0   PC += off if dst < src     unsigned
-BPF_JLE   0xb0   PC += off if dst <= src    unsigned
-BPF_JSLT  0xc0   PC += off if dst < src     signed
-BPF_JSLE  0xd0   PC += off if dst <= src    signed
-========  =====  =========================  ============
+========  =====  ===  ==========================  ========================
+code      value  src  description                 notes
+========  =====  ===  ==========================  ========================
+BPF_JA    0x0    0x0  PC += offset                BPF_JMP only
+BPF_JEQ   0x1    any  PC += offset if dst == src
+BPF_JGT   0x2    any  PC += offset if dst > src   unsigned
+BPF_JGE   0x3    any  PC += offset if dst >= src  unsigned
+BPF_JSET  0x4    any  PC += offset if dst & src
+BPF_JNE   0x5    any  PC += offset if dst != src
+BPF_JSGT  0x6    any  PC += offset if dst > src   signed
+BPF_JSGE  0x7    any  PC += offset if dst >= src  signed
+BPF_CALL  0x8    0x0  call helper function imm    see `Helper functions`_
+BPF_CALL  0x8    0x1  call PC += offset           see `eBPF functions`_
+BPF_CALL  0x8    0x2  call runtime function imm   see `Runtime functions`_
+BPF_EXIT  0x9    0x0  return                      BPF_JMP only
+BPF_JLT   0xa    any  PC += offset if dst < src   unsigned
+BPF_JLE   0xb    any  PC += offset if dst <= src  unsigned
+BPF_JSLT  0xc    any  PC += offset if dst < src   signed
+BPF_JSLE  0xd    any  PC += offset if dst <= src  signed
+========  =====  ===  ==========================  ========================
 
 The eBPF program needs to store the return value into register R0 before doing a
 BPF_EXIT.
@@ -272,6 +274,18 @@  set of function calls exposed by the runtime.  Each helper
 function is identified by an integer used in a ``BPF_CALL`` instruction.
 The available helper functions may differ for each program type.
 
+Runtime functions
+~~~~~~~~~~~~~~~~~
+Runtime functions are like helper functions except that they are not specific
+to eBPF programs.  They use a different numbering space from helper functions,
+but otherwise the same considerations apply.
+
+eBPF functions
+~~~~~~~~~~~~~~
+eBPF functions are functions exposed by the same eBPF program as the caller,
+and are referenced by offset from the call instruction, similar to ``BPF_JA``.
+A ``BPF_EXIT`` within the eBPF function will return to the caller.
+
 Load and store instructions
 ===========================