[PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

[PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

[Dave Thaler]

* Add a "callx" conformance group
* Add callx row to table
* Update helper function to section to be agnostic between BPF_K vs
  BPF_X
* Rename "legacy" conformance group to "packet"

Based on mailing list discussion at
https://mailarchive.ietf.org/arch/msg/bpf/l5tNEgL-Wo7qSEuaGssOl5VChKk/

Only src=0 is currently listed for callx. Neither clang nor gcc
use src=1 or src=2, and both use exactly the same semantics for
src=0 which was agreed between them (Yonghong and Jose). Since src=0
semantics are agreed upon by both and is already implemented, src=0
is documented as implemented.

v1->v2: Incorporated feedback from Will Hawkins

v2->v3: Use "dst" not "imm" field

v3->v4: Only use src=0

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
---
 .../bpf/standardization/instruction-set.rst   | 29 ++++++++++++-------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
index bdfe0cd0e..a68445899 100644
--- a/Documentation/bpf/standardization/instruction-set.rst
+++ b/Documentation/bpf/standardization/instruction-set.rst
@@ -127,7 +127,7 @@ This document defines the following conformance groups:
 * divmul32: includes 32-bit division, multiplication, and modulo instructions.
 * divmul64: includes divmul32, plus 64-bit division, multiplication,
   and modulo instructions.
-* legacy: deprecated packet access instructions.
+* packet: deprecated packet access instructions.
 
 Instruction encoding
 ====================
@@ -404,9 +404,10 @@ 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 by address  BPF_JMP | BPF_K only, see `Helper functions`_
+BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
+BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only
 BPF_CALL  0x8    0x1  call PC += imm                   BPF_JMP | BPF_K only, see `Program-local functions`_
-BPF_CALL  0x8    0x2  call helper function by BTF ID   BPF_JMP | BPF_K only, see `Helper functions`_
+BPF_CALL  0x8    0x2  call_by_btfid(imm)               BPF_JMP | BPF_K only
 BPF_EXIT  0x9    0x0  return                           BPF_JMP | BPF_K only
 BPF_JLT   0xa    any  PC += offset if dst < src        unsigned
 BPF_JLE   0xb    any  PC += offset if dst <= src       unsigned
@@ -414,6 +415,11 @@ BPF_JSLT  0xc    any  PC += offset if dst < src        signed
 BPF_JSLE  0xd    any  PC += offset if dst <= src       signed
 ========  =====  ===  ===============================  =============================================
 
+where
+
+* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)
+* call_by_btfid(value) means to call a helper function by the BTF ID specified by 'value' (see `Helper functions`_ for details)
+
 The BPF program needs to store the return value into register R0 before doing a
 ``BPF_EXIT``.
 
@@ -438,8 +444,9 @@ specified by the 'imm' field. A > 16-bit conditional jump may be
 converted to a < 16-bit conditional jump plus a 32-bit unconditional
 jump.
 
-All ``BPF_CALL`` and ``BPF_JA`` instructions belong to the
-base32 conformance group.
+The ``BPF_CALL | BPF_X`` instruction belongs to the callx
+conformance group.  All other ``BPF_CALL`` instructions and all
+``BPF_JA`` instructions belong to the base32 conformance group.
 
 Helper functions
 ~~~~~~~~~~~~~~~~
@@ -447,13 +454,13 @@ Helper functions
 Helper functions are a concept whereby BPF programs can call into a
 set of function calls exposed by the underlying platform.
 
-Historically, each helper function was identified by an address
-encoded in the imm field.  The available helper functions may differ
-for each program type, but address values are unique across all program types.
+Historically, each helper function was identified by an address.
+The available helper functions may differ for each program type,
+but address values are unique across all program types.
 
 Platforms that support the BPF Type Format (BTF) support identifying
-a helper function by a BTF ID encoded in the imm field, where the BTF ID
-identifies the helper name and type.
+a helper function by a BTF ID, where the BTF ID identifies the helper
+name and type.
 
 Program-local functions
 ~~~~~~~~~~~~~~~~~~~~~~~
@@ -660,4 +667,4 @@ carried over from classic BPF. These instructions used an instruction
 class of BPF_LD, a size modifier of BPF_W, BPF_H, or BPF_B, and a
 mode modifier of BPF_ABS or BPF_IND.  However, these instructions are
 deprecated and should no longer be used.  All legacy packet access
-instructions belong to the "legacy" conformance group.
+instructions belong to the "packet" conformance group.
-- 
2.40.1

[Bpf] [PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

[Dave Thaler]

* Add a "callx" conformance group
* Add callx row to table
* Update helper function to section to be agnostic between BPF_K vs
  BPF_X
* Rename "legacy" conformance group to "packet"

Based on mailing list discussion at
https://mailarchive.ietf.org/arch/msg/bpf/l5tNEgL-Wo7qSEuaGssOl5VChKk/

Only src=0 is currently listed for callx. Neither clang nor gcc
use src=1 or src=2, and both use exactly the same semantics for
src=0 which was agreed between them (Yonghong and Jose). Since src=0
semantics are agreed upon by both and is already implemented, src=0
is documented as implemented.

v1->v2: Incorporated feedback from Will Hawkins

v2->v3: Use "dst" not "imm" field

v3->v4: Only use src=0

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
---
 .../bpf/standardization/instruction-set.rst   | 29 ++++++++++++-------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
index bdfe0cd0e..a68445899 100644
--- a/Documentation/bpf/standardization/instruction-set.rst
+++ b/Documentation/bpf/standardization/instruction-set.rst
@@ -127,7 +127,7 @@ This document defines the following conformance groups:
 * divmul32: includes 32-bit division, multiplication, and modulo instructions.
 * divmul64: includes divmul32, plus 64-bit division, multiplication,
   and modulo instructions.
-* legacy: deprecated packet access instructions.
+* packet: deprecated packet access instructions.
 
 Instruction encoding
 ====================
@@ -404,9 +404,10 @@ 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 by address  BPF_JMP | BPF_K only, see `Helper functions`_
+BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
+BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only
 BPF_CALL  0x8    0x1  call PC += imm                   BPF_JMP | BPF_K only, see `Program-local functions`_
-BPF_CALL  0x8    0x2  call helper function by BTF ID   BPF_JMP | BPF_K only, see `Helper functions`_
+BPF_CALL  0x8    0x2  call_by_btfid(imm)               BPF_JMP | BPF_K only
 BPF_EXIT  0x9    0x0  return                           BPF_JMP | BPF_K only
 BPF_JLT   0xa    any  PC += offset if dst < src        unsigned
 BPF_JLE   0xb    any  PC += offset if dst <= src       unsigned
@@ -414,6 +415,11 @@ BPF_JSLT  0xc    any  PC += offset if dst < src        signed
 BPF_JSLE  0xd    any  PC += offset if dst <= src       signed
 ========  =====  ===  ===============================  =============================================
 
+where
+
+* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)
+* call_by_btfid(value) means to call a helper function by the BTF ID specified by 'value' (see `Helper functions`_ for details)
+
 The BPF program needs to store the return value into register R0 before doing a
 ``BPF_EXIT``.
 
@@ -438,8 +444,9 @@ specified by the 'imm' field. A > 16-bit conditional jump may be
 converted to a < 16-bit conditional jump plus a 32-bit unconditional
 jump.
 
-All ``BPF_CALL`` and ``BPF_JA`` instructions belong to the
-base32 conformance group.
+The ``BPF_CALL | BPF_X`` instruction belongs to the callx
+conformance group.  All other ``BPF_CALL`` instructions and all
+``BPF_JA`` instructions belong to the base32 conformance group.
 
 Helper functions
 ~~~~~~~~~~~~~~~~
@@ -447,13 +454,13 @@ Helper functions
 Helper functions are a concept whereby BPF programs can call into a
 set of function calls exposed by the underlying platform.
 
-Historically, each helper function was identified by an address
-encoded in the imm field.  The available helper functions may differ
-for each program type, but address values are unique across all program types.
+Historically, each helper function was identified by an address.
+The available helper functions may differ for each program type,
+but address values are unique across all program types.
 
 Platforms that support the BPF Type Format (BTF) support identifying
-a helper function by a BTF ID encoded in the imm field, where the BTF ID
-identifies the helper name and type.
+a helper function by a BTF ID, where the BTF ID identifies the helper
+name and type.
 
 Program-local functions
 ~~~~~~~~~~~~~~~~~~~~~~~
@@ -660,4 +667,4 @@ carried over from classic BPF. These instructions used an instruction
 class of BPF_LD, a size modifier of BPF_W, BPF_H, or BPF_B, and a
 mode modifier of BPF_ABS or BPF_IND.  However, these instructions are
 deprecated and should no longer be used.  All legacy packet access
-instructions belong to the "legacy" conformance group.
+instructions belong to the "packet" conformance group.
-- 
2.40.1

-- 
Bpf mailing list
Bpf@ietf.org
https://www.ietf.org/mailman/listinfo/bpf

Re: [Bpf] [PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

[David Vernet]

[-- Attachment #1: Type: text/plain, Size: 5709 bytes --]

On Wed, Feb 21, 2024 at 11:17:25AM -0800, Dave Thaler wrote:
> * Add a "callx" conformance group
> * Add callx row to table
> * Update helper function to section to be agnostic between BPF_K vs
>   BPF_X
> * Rename "legacy" conformance group to "packet"
> 
> Based on mailing list discussion at
> https://mailarchive.ietf.org/arch/msg/bpf/l5tNEgL-Wo7qSEuaGssOl5VChKk/
> 
> Only src=0 is currently listed for callx. Neither clang nor gcc
> use src=1 or src=2, and both use exactly the same semantics for
> src=0 which was agreed between them (Yonghong and Jose). Since src=0
> semantics are agreed upon by both and is already implemented, src=0
> is documented as implemented.

If the semantics for src=0 were already decided for both clang and gcc,
then this seems fine to me. Agreed as well with leaving src > 0 for
later, as Alexei said on the v3 thread. We can decide how to best deal
with indirect calls at a later time.

Alexei -- is this acceptable?

> v1->v2: Incorporated feedback from Will Hawkins
> 
> v2->v3: Use "dst" not "imm" field
> 
> v3->v4: Only use src=0
> 
> Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
> ---
>  .../bpf/standardization/instruction-set.rst   | 29 ++++++++++++-------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
> index bdfe0cd0e..a68445899 100644
> --- a/Documentation/bpf/standardization/instruction-set.rst
> +++ b/Documentation/bpf/standardization/instruction-set.rst
> @@ -127,7 +127,7 @@ This document defines the following conformance groups:
>  * divmul32: includes 32-bit division, multiplication, and modulo instructions.
>  * divmul64: includes divmul32, plus 64-bit division, multiplication,
>    and modulo instructions.
> -* legacy: deprecated packet access instructions.
> +* packet: deprecated packet access instructions.
>  
>  Instruction encoding
>  ====================
> @@ -404,9 +404,10 @@ 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 by address  BPF_JMP | BPF_K only, see `Helper functions`_
> +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only
>  BPF_CALL  0x8    0x1  call PC += imm                   BPF_JMP | BPF_K only, see `Program-local functions`_
> -BPF_CALL  0x8    0x2  call helper function by BTF ID   BPF_JMP | BPF_K only, see `Helper functions`_
> +BPF_CALL  0x8    0x2  call_by_btfid(imm)               BPF_JMP | BPF_K only
>  BPF_EXIT  0x9    0x0  return                           BPF_JMP | BPF_K only
>  BPF_JLT   0xa    any  PC += offset if dst < src        unsigned
>  BPF_JLE   0xb    any  PC += offset if dst <= src       unsigned
> @@ -414,6 +415,11 @@ BPF_JSLT  0xc    any  PC += offset if dst < src        signed
>  BPF_JSLE  0xd    any  PC += offset if dst <= src       signed
>  ========  =====  ===  ===============================  =============================================
>  
> +where
> +
> +* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)
> +* call_by_btfid(value) means to call a helper function by the BTF ID specified by 'value' (see `Helper functions`_ for details)
> +
>  The BPF program needs to store the return value into register R0 before doing a
>  ``BPF_EXIT``.
>  
> @@ -438,8 +444,9 @@ specified by the 'imm' field. A > 16-bit conditional jump may be
>  converted to a < 16-bit conditional jump plus a 32-bit unconditional
>  jump.
>  
> -All ``BPF_CALL`` and ``BPF_JA`` instructions belong to the
> -base32 conformance group.
> +The ``BPF_CALL | BPF_X`` instruction belongs to the callx
> +conformance group.  All other ``BPF_CALL`` instructions and all
> +``BPF_JA`` instructions belong to the base32 conformance group.
>  
>  Helper functions
>  ~~~~~~~~~~~~~~~~
> @@ -447,13 +454,13 @@ Helper functions
>  Helper functions are a concept whereby BPF programs can call into a
>  set of function calls exposed by the underlying platform.
>  
> -Historically, each helper function was identified by an address
> -encoded in the imm field.  The available helper functions may differ
> -for each program type, but address values are unique across all program types.
> +Historically, each helper function was identified by an address.
> +The available helper functions may differ for each program type,
> +but address values are unique across all program types.
>  
>  Platforms that support the BPF Type Format (BTF) support identifying
> -a helper function by a BTF ID encoded in the imm field, where the BTF ID
> -identifies the helper name and type.
> +a helper function by a BTF ID, where the BTF ID identifies the helper
> +name and type.
>  
>  Program-local functions
>  ~~~~~~~~~~~~~~~~~~~~~~~
> @@ -660,4 +667,4 @@ carried over from classic BPF. These instructions used an instruction
>  class of BPF_LD, a size modifier of BPF_W, BPF_H, or BPF_B, and a
>  mode modifier of BPF_ABS or BPF_IND.  However, these instructions are
>  deprecated and should no longer be used.  All legacy packet access
> -instructions belong to the "legacy" conformance group.
> +instructions belong to the "packet" conformance group.
> -- 
> 2.40.1
> 
> -- 
> Bpf mailing list
> Bpf@ietf.org
> https://www.ietf.org/mailman/listinfo/bpf

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [Bpf] [PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

[David Vernet]

[-- Attachment #1.1: Type: text/plain, Size: 5709 bytes --]

On Wed, Feb 21, 2024 at 11:17:25AM -0800, Dave Thaler wrote:
> * Add a "callx" conformance group
> * Add callx row to table
> * Update helper function to section to be agnostic between BPF_K vs
>   BPF_X
> * Rename "legacy" conformance group to "packet"
> 
> Based on mailing list discussion at
> https://mailarchive.ietf.org/arch/msg/bpf/l5tNEgL-Wo7qSEuaGssOl5VChKk/
> 
> Only src=0 is currently listed for callx. Neither clang nor gcc
> use src=1 or src=2, and both use exactly the same semantics for
> src=0 which was agreed between them (Yonghong and Jose). Since src=0
> semantics are agreed upon by both and is already implemented, src=0
> is documented as implemented.

If the semantics for src=0 were already decided for both clang and gcc,
then this seems fine to me. Agreed as well with leaving src > 0 for
later, as Alexei said on the v3 thread. We can decide how to best deal
with indirect calls at a later time.

Alexei -- is this acceptable?

> v1->v2: Incorporated feedback from Will Hawkins
> 
> v2->v3: Use "dst" not "imm" field
> 
> v3->v4: Only use src=0
> 
> Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
> ---
>  .../bpf/standardization/instruction-set.rst   | 29 ++++++++++++-------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
> index bdfe0cd0e..a68445899 100644
> --- a/Documentation/bpf/standardization/instruction-set.rst
> +++ b/Documentation/bpf/standardization/instruction-set.rst
> @@ -127,7 +127,7 @@ This document defines the following conformance groups:
>  * divmul32: includes 32-bit division, multiplication, and modulo instructions.
>  * divmul64: includes divmul32, plus 64-bit division, multiplication,
>    and modulo instructions.
> -* legacy: deprecated packet access instructions.
> +* packet: deprecated packet access instructions.
>  
>  Instruction encoding
>  ====================
> @@ -404,9 +404,10 @@ 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 by address  BPF_JMP | BPF_K only, see `Helper functions`_
> +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only
>  BPF_CALL  0x8    0x1  call PC += imm                   BPF_JMP | BPF_K only, see `Program-local functions`_
> -BPF_CALL  0x8    0x2  call helper function by BTF ID   BPF_JMP | BPF_K only, see `Helper functions`_
> +BPF_CALL  0x8    0x2  call_by_btfid(imm)               BPF_JMP | BPF_K only
>  BPF_EXIT  0x9    0x0  return                           BPF_JMP | BPF_K only
>  BPF_JLT   0xa    any  PC += offset if dst < src        unsigned
>  BPF_JLE   0xb    any  PC += offset if dst <= src       unsigned
> @@ -414,6 +415,11 @@ BPF_JSLT  0xc    any  PC += offset if dst < src        signed
>  BPF_JSLE  0xd    any  PC += offset if dst <= src       signed
>  ========  =====  ===  ===============================  =============================================
>  
> +where
> +
> +* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)
> +* call_by_btfid(value) means to call a helper function by the BTF ID specified by 'value' (see `Helper functions`_ for details)
> +
>  The BPF program needs to store the return value into register R0 before doing a
>  ``BPF_EXIT``.
>  
> @@ -438,8 +444,9 @@ specified by the 'imm' field. A > 16-bit conditional jump may be
>  converted to a < 16-bit conditional jump plus a 32-bit unconditional
>  jump.
>  
> -All ``BPF_CALL`` and ``BPF_JA`` instructions belong to the
> -base32 conformance group.
> +The ``BPF_CALL | BPF_X`` instruction belongs to the callx
> +conformance group.  All other ``BPF_CALL`` instructions and all
> +``BPF_JA`` instructions belong to the base32 conformance group.
>  
>  Helper functions
>  ~~~~~~~~~~~~~~~~
> @@ -447,13 +454,13 @@ Helper functions
>  Helper functions are a concept whereby BPF programs can call into a
>  set of function calls exposed by the underlying platform.
>  
> -Historically, each helper function was identified by an address
> -encoded in the imm field.  The available helper functions may differ
> -for each program type, but address values are unique across all program types.
> +Historically, each helper function was identified by an address.
> +The available helper functions may differ for each program type,
> +but address values are unique across all program types.
>  
>  Platforms that support the BPF Type Format (BTF) support identifying
> -a helper function by a BTF ID encoded in the imm field, where the BTF ID
> -identifies the helper name and type.
> +a helper function by a BTF ID, where the BTF ID identifies the helper
> +name and type.
>  
>  Program-local functions
>  ~~~~~~~~~~~~~~~~~~~~~~~
> @@ -660,4 +667,4 @@ carried over from classic BPF. These instructions used an instruction
>  class of BPF_LD, a size modifier of BPF_W, BPF_H, or BPF_B, and a
>  mode modifier of BPF_ABS or BPF_IND.  However, these instructions are
>  deprecated and should no longer be used.  All legacy packet access
> -instructions belong to the "legacy" conformance group.
> +instructions belong to the "packet" conformance group.
> -- 
> 2.40.1
> 
> -- 
> Bpf mailing list
> Bpf@ietf.org
> https://www.ietf.org/mailman/listinfo/bpf

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 76 bytes --]

-- 
Bpf mailing list
Bpf@ietf.org
https://www.ietf.org/mailman/listinfo/bpf

Re: [PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

[Alexei Starovoitov]

On Wed, Feb 21, 2024 at 11:17 AM Dave Thaler <dthaler1968@googlemail.com> wrote:
>
> -BPF_CALL  0x8    0x0  call helper function by address  BPF_JMP | BPF_K only, see `Helper functions`_
> +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only

...

> +* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)


Sorry, we're not going to take this path in the kernel verifier.
I understand that you went with this semantics in PREVAIL verifier,
but this is user space and I suspect once PREVAIL folks realize
that it's not that useful you will change that.
User space has a luxury to change. The kernel doesn't
and we won't be able to change such things in the standard either.

Essentially what you're proposing is to treat
callx dst_reg
as calling any of the existing helpers by a number.
Let's look at the first ~6:
id = 1  void *bpf_map_lookup_elem(struct bpf_map *map, const void *key)
id = 2 long bpf_map_update_elem(struct bpf_map *map, const void *key,
const void *value, u64 flags)
...
id = 6 long bpf_trace_printk(const char *fmt, u32 fmt_size, ...)

They have almost nothing in common.
In C that would be an indirect call of "long (*fn)(...)"
just call anything and hope it works.
This is not useful in practice.

Also commit log is wrong:

> Only src=0 is currently listed for callx. Neither clang nor gcc
> use src=1 or src=2, and both use exactly the same semantics for
> src=0 which was agreed between them (Yonghong and Jose).

this is not at all what gcc and clang are doing.
They emit "callx dst_reg" when they need to compile a normal indirect call
which address is in dst_reg.
It's the real address of the function and not a helper ID.

Hence these two:
> +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only

are not correct.
call imm is a call of helper with a given ID.
callx dst_reg is a call of a function by its real address.

This is _prelminary_ definition of callx dst_reg from compiler pov,
but there is no implementation of it in the kernel, so
it's way too early to hard code such semantics in the standard.

pw-bot: cr

Re: [Bpf] [PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

[Alexei Starovoitov]

On Wed, Feb 21, 2024 at 11:17 AM Dave Thaler <dthaler1968@googlemail.com> wrote:
>
> -BPF_CALL  0x8    0x0  call helper function by address  BPF_JMP | BPF_K only, see `Helper functions`_
> +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only

...

> +* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)


Sorry, we're not going to take this path in the kernel verifier.
I understand that you went with this semantics in PREVAIL verifier,
but this is user space and I suspect once PREVAIL folks realize
that it's not that useful you will change that.
User space has a luxury to change. The kernel doesn't
and we won't be able to change such things in the standard either.

Essentially what you're proposing is to treat
callx dst_reg
as calling any of the existing helpers by a number.
Let's look at the first ~6:
id = 1  void *bpf_map_lookup_elem(struct bpf_map *map, const void *key)
id = 2 long bpf_map_update_elem(struct bpf_map *map, const void *key,
const void *value, u64 flags)
...
id = 6 long bpf_trace_printk(const char *fmt, u32 fmt_size, ...)

They have almost nothing in common.
In C that would be an indirect call of "long (*fn)(...)"
just call anything and hope it works.
This is not useful in practice.

Also commit log is wrong:

> Only src=0 is currently listed for callx. Neither clang nor gcc
> use src=1 or src=2, and both use exactly the same semantics for
> src=0 which was agreed between them (Yonghong and Jose).

this is not at all what gcc and clang are doing.
They emit "callx dst_reg" when they need to compile a normal indirect call
which address is in dst_reg.
It's the real address of the function and not a helper ID.

Hence these two:
> +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only

are not correct.
call imm is a call of helper with a given ID.
callx dst_reg is a call of a function by its real address.

This is _prelminary_ definition of callx dst_reg from compiler pov,
but there is no implementation of it in the kernel, so
it's way too early to hard code such semantics in the standard.

pw-bot: cr

-- 
Bpf mailing list
Bpf@ietf.org
https://www.ietf.org/mailman/listinfo/bpf

Re: [Bpf] [PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

[David Vernet]

[-- Attachment #1: Type: text/plain, Size: 2905 bytes --]

On Thu, Feb 22, 2024 at 09:28:47AM -0800, Alexei Starovoitov wrote:
> On Wed, Feb 21, 2024 at 11:17 AM Dave Thaler <dthaler1968@googlemail.com> wrote:
> >
> > -BPF_CALL  0x8    0x0  call helper function by address  BPF_JMP | BPF_K only, see `Helper functions`_
> > +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> > +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only
> 
> ...
> 
> > +* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)
> 
> 
> Sorry, we're not going to take this path in the kernel verifier.
> I understand that you went with this semantics in PREVAIL verifier,
> but this is user space and I suspect once PREVAIL folks realize
> that it's not that useful you will change that.
> User space has a luxury to change. The kernel doesn't
> and we won't be able to change such things in the standard either.
> 
> Essentially what you're proposing is to treat
> callx dst_reg
> as calling any of the existing helpers by a number.
> Let's look at the first ~6:
> id = 1  void *bpf_map_lookup_elem(struct bpf_map *map, const void *key)
> id = 2 long bpf_map_update_elem(struct bpf_map *map, const void *key,
> const void *value, u64 flags)
> ...
> id = 6 long bpf_trace_printk(const char *fmt, u32 fmt_size, ...)
> 
> They have almost nothing in common.
> In C that would be an indirect call of "long (*fn)(...)"
> just call anything and hope it works.
> This is not useful in practice.
> 
> Also commit log is wrong:
> 
> > Only src=0 is currently listed for callx. Neither clang nor gcc
> > use src=1 or src=2, and both use exactly the same semantics for
> > src=0 which was agreed between them (Yonghong and Jose).
> 
> this is not at all what gcc and clang are doing.
> They emit "callx dst_reg" when they need to compile a normal indirect call
> which address is in dst_reg.
> It's the real address of the function and not a helper ID.
> 
> Hence these two:
> > +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> > +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only
> 
> are not correct.
> call imm is a call of helper with a given ID.
> callx dst_reg is a call of a function by its real address.
> 
> This is _prelminary_ definition of callx dst_reg from compiler pov,
> but there is no implementation of it in the kernel, so
> it's way too early to hard code such semantics in the standard.

Dave -- are you OK with us just reserving the semantics for all callx
instructions, including src=0? At this point I think it's probably just
best for us to boot the whole thing to an extension.

I'm happy to send a patch for that if you agree (or please feel free to
send a v5 of this series which just reserves the group).

Thanks,
David

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [Bpf] [PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

[David Vernet]

[-- Attachment #1.1: Type: text/plain, Size: 2905 bytes --]

On Thu, Feb 22, 2024 at 09:28:47AM -0800, Alexei Starovoitov wrote:
> On Wed, Feb 21, 2024 at 11:17 AM Dave Thaler <dthaler1968@googlemail.com> wrote:
> >
> > -BPF_CALL  0x8    0x0  call helper function by address  BPF_JMP | BPF_K only, see `Helper functions`_
> > +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> > +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only
> 
> ...
> 
> > +* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)
> 
> 
> Sorry, we're not going to take this path in the kernel verifier.
> I understand that you went with this semantics in PREVAIL verifier,
> but this is user space and I suspect once PREVAIL folks realize
> that it's not that useful you will change that.
> User space has a luxury to change. The kernel doesn't
> and we won't be able to change such things in the standard either.
> 
> Essentially what you're proposing is to treat
> callx dst_reg
> as calling any of the existing helpers by a number.
> Let's look at the first ~6:
> id = 1  void *bpf_map_lookup_elem(struct bpf_map *map, const void *key)
> id = 2 long bpf_map_update_elem(struct bpf_map *map, const void *key,
> const void *value, u64 flags)
> ...
> id = 6 long bpf_trace_printk(const char *fmt, u32 fmt_size, ...)
> 
> They have almost nothing in common.
> In C that would be an indirect call of "long (*fn)(...)"
> just call anything and hope it works.
> This is not useful in practice.
> 
> Also commit log is wrong:
> 
> > Only src=0 is currently listed for callx. Neither clang nor gcc
> > use src=1 or src=2, and both use exactly the same semantics for
> > src=0 which was agreed between them (Yonghong and Jose).
> 
> this is not at all what gcc and clang are doing.
> They emit "callx dst_reg" when they need to compile a normal indirect call
> which address is in dst_reg.
> It's the real address of the function and not a helper ID.
> 
> Hence these two:
> > +BPF_CALL  0x8    0x0  call_by_address(imm)             BPF_JMP | BPF_K only
> > +BPF_CALL  0x8    0x0  call_by_address(dst)             BPF_JMP | BPF_X only
> 
> are not correct.
> call imm is a call of helper with a given ID.
> callx dst_reg is a call of a function by its real address.
> 
> This is _prelminary_ definition of callx dst_reg from compiler pov,
> but there is no implementation of it in the kernel, so
> it's way too early to hard code such semantics in the standard.

Dave -- are you OK with us just reserving the semantics for all callx
instructions, including src=0? At this point I think it's probably just
best for us to boot the whole thing to an extension.

I'm happy to send a patch for that if you agree (or please feel free to
send a v5 of this series which just reserves the group).

Thanks,
David

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 76 bytes --]

-- 
Bpf mailing list
Bpf@ietf.org
https://www.ietf.org/mailman/listinfo/bpf