| Message ID | 20230206191647.2075-1-dthaler1968@googlemail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] bpf, docs: Explain helper functions | expand |
On Mon, Feb 06, 2023 at 07:16:47PM +0000, Dave Thaler wrote: > From: Dave Thaler <dthaler@microsoft.com> > > Add text explaining helper functions. > Note that text about runtime functions (kfuncs) is part of a separate patch, > not this one. > > Signed-off-by: Dave Thaler <dthaler@microsoft.com> > --- > V1 -> V2: addressed comments from Alexei and Stanislav > --- > Documentation/bpf/clang-notes.rst | 5 +++++ > Documentation/bpf/instruction-set.rst | 22 +++++++++++++++++++++- > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/Documentation/bpf/clang-notes.rst b/Documentation/bpf/clang-notes.rst > index 528feddf2db..40c6185513a 100644 > --- a/Documentation/bpf/clang-notes.rst > +++ b/Documentation/bpf/clang-notes.rst > @@ -20,6 +20,11 @@ Arithmetic instructions > For CPU versions prior to 3, Clang v7.0 and later can enable ``BPF_ALU`` support with > ``-Xclang -target-feature -Xclang +alu32``. In CPU version 3, support is automatically included. > > +Reserved instructions > +==================== small nit: Missing a = > + > +Clang will generate the reserved ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d) instruction if ``-O0`` is used. Are we calling this out here to say that BPF_CALL in clang -O0 builds is not supported? That would seem to be the case given that we say that BPF_CALL | BPF_X | BPF_JMP in reserved and not permitted in instruction-set.rst. If that's not the case, can we add a bit more verbiage here describing why this is done / why it's interesting and/or relevant to the reader? FWIW, most of our selftests don't seem to compile with clang -O0. > +Note that ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d), where the helper function integer > +would be read from a specified register, is reserved and currently not permitted. > + > Atomic operations > ================= > > diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst > index 2d3fe59bd26..89a13f1cdeb 100644 > --- a/Documentation/bpf/instruction-set.rst > +++ b/Documentation/bpf/instruction-set.rst > @@ -191,7 +191,7 @@ 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 > +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 > @@ -202,6 +202,26 @@ BPF_JSLE 0xd0 PC += off if dst <= src signed > The eBPF program needs to store the return value into register R0 before doing a > BPF_EXIT. > > +Helper functions > +~~~~~~~~~~~~~~~~ > + > +Helper functions are a concept whereby BPF programs can call into a > +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. > + > +Conceptually, each helper function is implemented with a commonly shared function > +signature defined as: > + > + u64 function(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5) > + > +In actuality, each helper function is defined as taking between 0 and 5 arguments, > +with the remaining registers being ignored. The definition of a helper function > +is responsible for specifying the type (e.g., integer, pointer, etc.) of the value returned, > +the number of arguments, and the type of each argument. > + > +Note that ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d), where the helper function integer > +would be read from a specified register, is reserved and currently not permitted. > > Load and store instructions > =========================== > -- > 2.33.4 > > -- > Bpf mailing list > Bpf@ietf.org > https://www.ietf.org/mailman/listinfo/bpf
David Vernet wrote: > > +Reserved instructions > > +==================== > > small nit: Missing a = Ack. > > +Clang will generate the reserved ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d) > instruction if ``-O0`` is used. > > Are we calling this out here to say that BPF_CALL in clang -O0 builds is not > supported? That would seem to be the case given that we say that BPF_CALL > | BPF_X | BPF_JMP in reserved and not permitted in instruction-set.rst. Yes, exactly. I could update the language to add something like "... so BPF_CALL in clang -O0 builds is not supported". Dave
On Wed, Feb 08, 2023 at 05:26:34PM +0000, Dave Thaler wrote: > David Vernet wrote: > > > +Reserved instructions > > > +==================== > > > > small nit: Missing a = > > Ack. > > > > +Clang will generate the reserved ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d) > > instruction if ``-O0`` is used. > > > > Are we calling this out here to say that BPF_CALL in clang -O0 builds is not > > supported? That would seem to be the case given that we say that BPF_CALL > > | BPF_X | BPF_JMP in reserved and not permitted in instruction-set.rst. > > Yes, exactly. I could update the language to add something like > "... so BPF_CALL in clang -O0 builds is not supported". Gotcha, that sounds good to me. Thanks for being so thorough in documenting all of this tribal knowledge.
On Wed, Feb 8, 2023 at 9:26 AM Dave Thaler <dthaler=40microsoft.com@dmarc.ietf.org> wrote: > > David Vernet wrote: > > > +Reserved instructions > > > +==================== > > > > small nit: Missing a = > > Ack. > > > > +Clang will generate the reserved ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d) > > instruction if ``-O0`` is used. > > > > Are we calling this out here to say that BPF_CALL in clang -O0 builds is not > > supported? That would seem to be the case given that we say that BPF_CALL > > | BPF_X | BPF_JMP in reserved and not permitted in instruction-set.rst. > > Yes, exactly. I could update the language to add something like > "... so BPF_CALL in clang -O0 builds is not supported". That will not be a correct statement. BPF_CALL is a valid insn regardless of optimization flags. BPF_CALLX will be a valid insn when the verifier support is added. Compilers need to make a choice which insn to use on a case by case basis. When compilers have no choice, but to use call by register they will use callx. That what happens with = (void *)1 hack that we use for helpers. It can happen with -O2 just as well.
On Wed, Feb 08, 2023 at 09:31:18AM -0800, Alexei Starovoitov wrote: > On Wed, Feb 8, 2023 at 9:26 AM Dave Thaler > <dthaler=40microsoft.com@dmarc.ietf.org> wrote: > > > > David Vernet wrote: > > > > +Reserved instructions > > > > +==================== > > > > > > small nit: Missing a = > > > > Ack. > > > > > > +Clang will generate the reserved ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d) > > > instruction if ``-O0`` is used. > > > > > > Are we calling this out here to say that BPF_CALL in clang -O0 builds is not > > > supported? That would seem to be the case given that we say that BPF_CALL > > > | BPF_X | BPF_JMP in reserved and not permitted in instruction-set.rst. > > > > Yes, exactly. I could update the language to add something like > > "... so BPF_CALL in clang -O0 builds is not supported". > > That will not be a correct statement. > BPF_CALL is a valid insn regardless of optimization flags. > BPF_CALLX will be a valid insn when the verifier support is added. > Compilers need to make a choice which insn to use on a case by case basis. > When compilers have no choice, but to use call by register they will > use callx. That what happens with = (void *)1 hack that we use for > helpers. > It can happen with -O2 just as well. In that case, I suggest we update the verbiage in instruction-set.rst to say: Note that ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d), where the helper function integer would be read from a specified register, is not currently supported by the verifier. Any programs with this instruction will fail to load until such support is added. And then we can update this section to say something similar, or just remove it altogether per Alexei's point that it's an implementation detail of the compiler which could change at any time.
> -----Original Message----- > From: David Vernet <void@manifault.com> > Sent: Wednesday, February 8, 2023 9:40 AM > To: Alexei Starovoitov <alexei.starovoitov@gmail.com> > Cc: Dave Thaler <dthaler@microsoft.com>; Dave Thaler > <dthaler1968@googlemail.com>; bpf@vger.kernel.org; bpf@ietf.org > Subject: Re: [Bpf] [PATCH bpf-next v2] bpf, docs: Explain helper functions > > On Wed, Feb 08, 2023 at 09:31:18AM -0800, Alexei Starovoitov wrote: > > On Wed, Feb 8, 2023 at 9:26 AM Dave Thaler > > <dthaler=40microsoft.com@dmarc.ietf.org> wrote: > > > > > > David Vernet wrote: > > > > > +Reserved instructions > > > > > +==================== > > > > > > > > small nit: Missing a = > > > > > > Ack. > > > > > > > > +Clang will generate the reserved ``BPF_CALL | BPF_X | BPF_JMP`` > > > > > +(0x8d) > > > > instruction if ``-O0`` is used. > > > > > > > > Are we calling this out here to say that BPF_CALL in clang -O0 > > > > builds is not supported? That would seem to be the case given that > > > > we say that BPF_CALL > > > > | BPF_X | BPF_JMP in reserved and not permitted in instruction-set.rst. > > > > > > Yes, exactly. I could update the language to add something like > > > "... so BPF_CALL in clang -O0 builds is not supported". > > > > That will not be a correct statement. > > BPF_CALL is a valid insn regardless of optimization flags. > > BPF_CALLX will be a valid insn when the verifier support is added. > > Compilers need to make a choice which insn to use on a case by case basis. > > When compilers have no choice, but to use call by register they will > > use callx. That what happens with = (void *)1 hack that we use for > > helpers. > > It can happen with -O2 just as well. > > In that case, I suggest we update the verbiage in instruction-set.rst to > say: > > Note that ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d), where the helper function > integer would be read from a specified register, is not currently supported by > the verifier. Any programs with this instruction will fail to load until such > support is added. The problem with that wording is that it implies that there is "the" verifier, whereas the point of standard documentation (since this file is also being used to generate the IETF spec) is to keep statements about any specific verifier or compiler out of instruction-set.rst. That's why there's separate files like clang-notes.rst for the clang compiler, etc. The instruction set rst is, in my view, should apply across all compilers, all verifiers, all runtimes, etc. It could potentially say certain things are optional to support, but there is a distinction between "defined" vs "reserved" where it currently means such support is "reserved" not "defined". Dave
On Wed, Feb 08, 2023 at 05:45:59PM +0000, Dave Thaler wrote: > > -----Original Message----- > > From: David Vernet <void@manifault.com> > > Sent: Wednesday, February 8, 2023 9:40 AM > > To: Alexei Starovoitov <alexei.starovoitov@gmail.com> > > Cc: Dave Thaler <dthaler@microsoft.com>; Dave Thaler > > <dthaler1968@googlemail.com>; bpf@vger.kernel.org; bpf@ietf.org > > Subject: Re: [Bpf] [PATCH bpf-next v2] bpf, docs: Explain helper functions > > > > On Wed, Feb 08, 2023 at 09:31:18AM -0800, Alexei Starovoitov wrote: > > > On Wed, Feb 8, 2023 at 9:26 AM Dave Thaler > > > <dthaler=40microsoft.com@dmarc.ietf.org> wrote: > > > > > > > > David Vernet wrote: > > > > > > +Reserved instructions > > > > > > +==================== > > > > > > > > > > small nit: Missing a = > > > > > > > > Ack. > > > > > > > > > > +Clang will generate the reserved ``BPF_CALL | BPF_X | BPF_JMP`` > > > > > > +(0x8d) > > > > > instruction if ``-O0`` is used. > > > > > > > > > > Are we calling this out here to say that BPF_CALL in clang -O0 > > > > > builds is not supported? That would seem to be the case given that > > > > > we say that BPF_CALL > > > > > | BPF_X | BPF_JMP in reserved and not permitted in instruction-set.rst. > > > > > > > > Yes, exactly. I could update the language to add something like > > > > "... so BPF_CALL in clang -O0 builds is not supported". > > > > > > That will not be a correct statement. > > > BPF_CALL is a valid insn regardless of optimization flags. > > > BPF_CALLX will be a valid insn when the verifier support is added. > > > Compilers need to make a choice which insn to use on a case by case basis. > > > When compilers have no choice, but to use call by register they will > > > use callx. That what happens with = (void *)1 hack that we use for > > > helpers. > > > It can happen with -O2 just as well. > > > > In that case, I suggest we update the verbiage in instruction-set.rst to > > say: > > > > Note that ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d), where the helper function > > integer would be read from a specified register, is not currently supported by > > the verifier. Any programs with this instruction will fail to load until such > > support is added. > > The problem with that wording is that it implies that there is "the" verifier, > whereas the point of standard documentation (since this file is also being used > to generate the IETF spec) is to keep statements about any specific verifier > or compiler out of instruction-set.rst. That's why there's separate files like Yes, good point. > clang-notes.rst for the clang compiler, etc. The instruction set rst is, > in my view, should apply across all compilers, all verifiers, all runtimes, etc. > It could potentially say certain things are optional to support, but there is > a distinction between "defined" vs "reserved" where it currently means > such support is "reserved" not "defined". That makes sense. IMO we should just say that the instruction is valid then, and not make a distinction. 'reserved' should imply that the bits for the instruction in question have no definition whatsoever, e.g. reserved bits in control registers in x86, etc. In this case, the instruction is valid, we just haven't chosen to implement support for it yet in the Linux verifier. That's par for the course for implementing standards. Usually we don't implement something until it's needed.
Hi Dave,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Dave-Thaler/bpf-docs-Explain-helper-functions/20230207-031845
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230206191647.2075-1-dthaler1968%40googlemail.com
patch subject: [PATCH bpf-next v2] bpf, docs: Explain helper functions
reproduce:
# https://github.com/intel-lab-lkp/linux/commit/b579d93fed53b16ad7241911226cbeb3b42f8266
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Dave-Thaler/bpf-docs-Explain-helper-functions/20230207-031845
git checkout b579d93fed53b16ad7241911226cbeb3b42f8266
make menuconfig
# enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS
make htmldocs
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302130706.NBSii5FS-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> Documentation/bpf/clang-notes.rst:24: WARNING: Title underline too short.
vim +24 Documentation/bpf/clang-notes.rst
22
23 Reserved instructions
> 24 ====================
25
diff --git a/Documentation/bpf/clang-notes.rst b/Documentation/bpf/clang-notes.rst index 528feddf2db..40c6185513a 100644 --- a/Documentation/bpf/clang-notes.rst +++ b/Documentation/bpf/clang-notes.rst @@ -20,6 +20,11 @@ Arithmetic instructions For CPU versions prior to 3, Clang v7.0 and later can enable ``BPF_ALU`` support with ``-Xclang -target-feature -Xclang +alu32``. In CPU version 3, support is automatically included. +Reserved instructions +==================== + +Clang will generate the reserved ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d) instruction if ``-O0`` is used. + Atomic operations ================= diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst index 2d3fe59bd26..89a13f1cdeb 100644 --- a/Documentation/bpf/instruction-set.rst +++ b/Documentation/bpf/instruction-set.rst @@ -191,7 +191,7 @@ 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 +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 @@ -202,6 +202,26 @@ BPF_JSLE 0xd0 PC += off if dst <= src signed The eBPF program needs to store the return value into register R0 before doing a BPF_EXIT. +Helper functions +~~~~~~~~~~~~~~~~ + +Helper functions are a concept whereby BPF programs can call into a +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. + +Conceptually, each helper function is implemented with a commonly shared function +signature defined as: + + u64 function(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5) + +In actuality, each helper function is defined as taking between 0 and 5 arguments, +with the remaining registers being ignored. The definition of a helper function +is responsible for specifying the type (e.g., integer, pointer, etc.) of the value returned, +the number of arguments, and the type of each argument. + +Note that ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d), where the helper function integer +would be read from a specified register, is reserved and currently not permitted. Load and store instructions ===========================