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

[David Vernet <void@manifault.com>]

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