diff mbox series

[bpf-next] bpf, docs: Expand set of initial conformance groups

Message ID 20240127170314.15881-1-dthaler1968@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] bpf, docs: Expand set of initial conformance groups | 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/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
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: 8 this patch: 8
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
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: 8 this patch: 8
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Dave Thaler <dthaler1968@googlemail.com>' != 'Signed-off-by: Dave Thaler <dthaler1968@gmail.com>'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Dave Thaler Jan. 27, 2024, 5:03 p.m. UTC 
This patch attempts to update the ISA specification according
to the latest mailing list discussion about conformance groups,
in a way that is intended to be consistent with IANA registry
processes and IETF 118 WG meeting discussion.

It does the following:
* Split basic into base32 and base64 for 32-bit vs 64-bit base
  instructions
* Split division/modulo instructions out of base groups
* Split atomic instructions out of base groups

There may be additional changes as discussion continues,
but there seems to be consensus on the principles above.

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

Comments

David Vernet Jan. 29, 2024, 9:04 p.m. UTC | #1 
On Sat, Jan 27, 2024 at 09:03:14AM -0800, Dave Thaler wrote:
> This patch attempts to update the ISA specification according
> to the latest mailing list discussion about conformance groups,
> in a way that is intended to be consistent with IANA registry
> processes and IETF 118 WG meeting discussion.
> 
> It does the following:
> * Split basic into base32 and base64 for 32-bit vs 64-bit base
>   instructions
> * Split division/modulo instructions out of base groups
> * Split atomic instructions out of base groups
> 
> There may be additional changes as discussion continues,
> but there seems to be consensus on the principles above.
> 
> Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Thanks a lot for working on this. I think it's getting very close. Left
a few comments below.

> ---
>  .../bpf/standardization/instruction-set.rst   | 44 ++++++++++++++-----
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
> index af43227b6..a090b5131 100644
> --- a/Documentation/bpf/standardization/instruction-set.rst
> +++ b/Documentation/bpf/standardization/instruction-set.rst
> @@ -102,7 +102,7 @@ Conformance groups
>  
>  An implementation does not need to support all instructions specified in this
>  document (e.g., deprecated instructions).  Instead, a number of conformance
> -groups are specified.  An implementation must support the "basic" conformance
> +groups are specified.  An implementation must support the base32 conformance
>  group and may support additional conformance groups, where supporting a
>  conformance group means it must support all instructions in that conformance
>  group.
> @@ -112,12 +112,20 @@ that executes instructions, and tools as such compilers that generate
>  instructions for the runtime.  Thus, capability discovery in terms of
>  conformance groups might be done manually by users or automatically by tools.
>  
> -Each conformance group has a short ASCII label (e.g., "basic") that
> +Each conformance group has a short ASCII label (e.g., "base32") that
>  corresponds to a set of instructions that are mandatory.  That is, each
>  instruction has one or more conformance groups of which it is a member.
>  
> -The "basic" conformance group includes all instructions defined in this
> -specification unless otherwise noted.
> +This document defines the following conformance groups:
> +* base32: includes all instructions defined in this
> +  specification unless otherwise noted.
> +* base64: includes base32, plus instructions explicited noted

s/explicited/explicitly

> +  as being in the base64 conformance group.
> +* atom32: includes 32-bit atomic operation instructions (see `Atomic operations`_).
> +* atom64: includes atom32, plus 64-bit atomic operation instructions.
> +* div32: includes 32-bit division and modulo instructions.

Did we want to separate division and modulo? It looks like Netronome
doesn't support modulo [0], presumably because it's not as useful as in
tracing.

Jakub -- can you confirm? If so, how difficult would it have been to add
modulo support, and do you think it would have provided any value?

[0]: https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/netronome/nfp/bpf/jit.c#L3421

> +* div64: includes div32, plus 64-bit division and modulo instructions.
> +* legacy: deprecated packet access instructions.
>  
>  Instruction encoding
>  ====================
> @@ -239,7 +247,8 @@ Arithmetic instructions
>  -----------------------
>  
>  ``BPF_ALU`` uses 32-bit wide operands while ``BPF_ALU64`` uses 64-bit wide operands for
> -otherwise identical operations.
> +otherwise identical operations. ``BPF_ALU64`` instructions belong to the
> +base64 conformance group unless noted otherwise.
>  The 'code' field encodes the operation as below, where 'src' and 'dst' refer
>  to the values of the source and destination registers, respectively.
>  
> @@ -293,6 +302,9 @@ where '(u32)' indicates that the upper 32 bits are zeroed.
>  Note that most instructions have instruction offset of 0. Only three instructions
>  (``BPF_SDIV``, ``BPF_SMOD``, ``BPF_MOVSX``) have a non-zero offset.
>  
> +Division and modulo operations for ``BPF_ALU`` are part of the "div32"
> +conformance group, and division and modulo operations for ``BPF_ALU64``
> +are part of the "div64" conformance group.
>  The division and modulo operations support both unsigned and signed flavors.
>  
>  For unsigned operations (``BPF_DIV`` and ``BPF_MOD``), for ``BPF_ALU``,
> @@ -349,7 +361,9 @@ BPF_ALU64  Reserved   0x00   do byte swap unconditionally
>  =========  =========  =====  =================================================
>  
>  The 'imm' field encodes the width of the swap operations.  The following widths
> -are supported: 16, 32 and 64.
> +are supported: 16, 32 and 64.  Width 64 operations belong to the base64
> +conformance group and other swap operations belong to the base32
> +conformance group.
>  
>  Examples:
>  
> @@ -374,8 +388,8 @@ Examples:
>  Jump instructions
>  -----------------
>  
> -``BPF_JMP32`` uses 32-bit wide operands while ``BPF_JMP`` uses 64-bit wide operands for
> -otherwise identical operations.
> +``BPF_JMP32`` uses 32-bit wide operands and indicates the base32 conformance group, while ``BPF_JMP`` uses 64-bit wide operands for

Could you please wrap this line?

> +otherwise identical operations, and indicates the base64 conformance group unless otherwise specified.
>  The 'code' field encodes the operation as below:

[...]

Seems reasonable other than the division vs. modulo question above.
Alexei, Christoph, Jose, what do you think?

Thanks,
David
David Vernet Jan. 31, 2024, 7:26 p.m. UTC | #2 
On Mon, Jan 29, 2024 at 03:04:23PM -0600, David Vernet wrote:

[...]

> > +  as being in the base64 conformance group.
> > +* atom32: includes 32-bit atomic operation instructions (see `Atomic operations`_).
> > +* atom64: includes atom32, plus 64-bit atomic operation instructions.
> > +* div32: includes 32-bit division and modulo instructions.
> 
> Did we want to separate division and modulo? It looks like Netronome
> doesn't support modulo [0], presumably because it's not as useful as in
> tracing.
> 
> Jakub -- can you confirm? If so, how difficult would it have been to add
> modulo support, and do you think it would have provided any value?
> 
> [0]: https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/netronome/nfp/bpf/jit.c#L3421

I spoke about this offline with Jakub. It turns out that there was
actually neither division nor modulo in the silicon. They only supported
division by the kernel's reciprocal division library. We could choose to
apply Netronome's choice to the standard, but I really don't think we
should.  Kuba pointed out that Netronome is old silicon, and that most
vendors today would likely start with RISC-V.

To that point, I believe the most prudent thing is to just mirror the
smallest riscv32 instruction-set granularity for our conformance groups.
For the case of multiplication, division, and modulo, this would be the
"M" standard extension for Integer Multiplication and Division, which
provides signed and unsigned multiplication, division, and modulo
instructions.

My suggestion is for us to mirror this exactly, here. I think the
contours set by RISC-V are much stronger data points for what will make
sense for vendors than what Netronome did on what at this point is
rather old silicon.

How do we feel about having divmul32/64 conformance groups? Thus
removing multiplication from the base32/64 groups. This would leave us
with:

- base{32/64}   (reflecting RV32I and RV64I plus our call instructions,
		 which logically fit here given that RISC-V control flow
		 instructions are in RV{32,64}I as well)
- divmul{32/64} (the "M" instruction set provides both 32 and 64 bit
		 versions of MUL(W), DIV{U}(W), and REM{U}(W)
		 instructions respectively)
- atom{32/64}   (the "A" extension provides 32 and 64 bit instructions
		 for R32 and R64 respectively, just like with div/mod)
- legacy

This to me seems like both the most logical layout of instructions, as
well as the least likely to cause pain for vendors given the precedence
that's already been set by RISC-V.

[...]

Thanks,
David
Dave Thaler Jan. 31, 2024, 9:07 p.m. UTC | #3 
David Vernet <void@manifault.com> wrote: 
[...]
> > > +  as being in the base64 conformance group.
> > > +* atom32: includes 32-bit atomic operation instructions (see `Atomic
> operations`_).
> > > +* atom64: includes atom32, plus 64-bit atomic operation instructions.
> > > +* div32: includes 32-bit division and modulo instructions.
> >
> > Did we want to separate division and modulo? It looks like Netronome
> > doesn't support modulo [0], presumably because it's not as useful as
> > in tracing.
> >
> > Jakub -- can you confirm? If so, how difficult would it have been to
> > add modulo support, and do you think it would have provided any value?
> >
> > [0]:
> > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/net
> > ronome/nfp/bpf/jit.c#L3421
> 
> I spoke about this offline with Jakub. It turns out that there was
actually
> neither division nor modulo in the silicon. They only supported division
by the
> kernel's reciprocal division library. We could choose to apply Netronome's
> choice to the standard, but I really don't think we should.  Kuba pointed
out
> that Netronome is old silicon, and that most vendors today would likely
start
> with RISC-V.
> 
> To that point, I believe the most prudent thing is to just mirror the
smallest
> riscv32 instruction-set granularity for our conformance groups.
> For the case of multiplication, division, and modulo, this would be the
"M"
> standard extension for Integer Multiplication and Division, which provides
> signed and unsigned multiplication, division, and modulo instructions.
> 
> My suggestion is for us to mirror this exactly, here. I think the contours
set by
> RISC-V are much stronger data points for what will make sense for vendors
than
> what Netronome did on what at this point is rather old silicon.
> 
> How do we feel about having divmul32/64 conformance groups? Thus
> removing multiplication from the base32/64 groups. This would leave us
> with:
> 
> - base{32/64}   (reflecting RV32I and RV64I plus our call instructions,
> 		 which logically fit here given that RISC-V control flow
> 		 instructions are in RV{32,64}I as well)
> - divmul{32/64} (the "M" instruction set provides both 32 and 64 bit
> 		 versions of MUL(W), DIV{U}(W), and REM{U}(W)
> 		 instructions respectively)
> - atom{32/64}   (the "A" extension provides 32 and 64 bit instructions
> 		 for R32 and R64 respectively, just like with div/mod)
> - legacy
> 
> This to me seems like both the most logical layout of instructions, as
well as the
> least likely to cause pain for vendors given the precedence that's already
been
> set by RISC-V.

This sounds fine to me.

Dave
Christoph Hellwig Feb. 2, 2024, 7:07 a.m. UTC | #4 
Fine with me as well.
diff mbox series

Patch

diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
index af43227b6..a090b5131 100644
--- a/Documentation/bpf/standardization/instruction-set.rst
+++ b/Documentation/bpf/standardization/instruction-set.rst
@@ -102,7 +102,7 @@  Conformance groups
 
 An implementation does not need to support all instructions specified in this
 document (e.g., deprecated instructions).  Instead, a number of conformance
-groups are specified.  An implementation must support the "basic" conformance
+groups are specified.  An implementation must support the base32 conformance
 group and may support additional conformance groups, where supporting a
 conformance group means it must support all instructions in that conformance
 group.
@@ -112,12 +112,20 @@  that executes instructions, and tools as such compilers that generate
 instructions for the runtime.  Thus, capability discovery in terms of
 conformance groups might be done manually by users or automatically by tools.
 
-Each conformance group has a short ASCII label (e.g., "basic") that
+Each conformance group has a short ASCII label (e.g., "base32") that
 corresponds to a set of instructions that are mandatory.  That is, each
 instruction has one or more conformance groups of which it is a member.
 
-The "basic" conformance group includes all instructions defined in this
-specification unless otherwise noted.
+This document defines the following conformance groups:
+* base32: includes all instructions defined in this
+  specification unless otherwise noted.
+* base64: includes base32, plus instructions explicited noted
+  as being in the base64 conformance group.
+* atom32: includes 32-bit atomic operation instructions (see `Atomic operations`_).
+* atom64: includes atom32, plus 64-bit atomic operation instructions.
+* div32: includes 32-bit division and modulo instructions.
+* div64: includes div32, plus 64-bit division and modulo instructions.
+* legacy: deprecated packet access instructions.
 
 Instruction encoding
 ====================
@@ -239,7 +247,8 @@  Arithmetic instructions
 -----------------------
 
 ``BPF_ALU`` uses 32-bit wide operands while ``BPF_ALU64`` uses 64-bit wide operands for
-otherwise identical operations.
+otherwise identical operations. ``BPF_ALU64`` instructions belong to the
+base64 conformance group unless noted otherwise.
 The 'code' field encodes the operation as below, where 'src' and 'dst' refer
 to the values of the source and destination registers, respectively.
 
@@ -293,6 +302,9 @@  where '(u32)' indicates that the upper 32 bits are zeroed.
 Note that most instructions have instruction offset of 0. Only three instructions
 (``BPF_SDIV``, ``BPF_SMOD``, ``BPF_MOVSX``) have a non-zero offset.
 
+Division and modulo operations for ``BPF_ALU`` are part of the "div32"
+conformance group, and division and modulo operations for ``BPF_ALU64``
+are part of the "div64" conformance group.
 The division and modulo operations support both unsigned and signed flavors.
 
 For unsigned operations (``BPF_DIV`` and ``BPF_MOD``), for ``BPF_ALU``,
@@ -349,7 +361,9 @@  BPF_ALU64  Reserved   0x00   do byte swap unconditionally
 =========  =========  =====  =================================================
 
 The 'imm' field encodes the width of the swap operations.  The following widths
-are supported: 16, 32 and 64.
+are supported: 16, 32 and 64.  Width 64 operations belong to the base64
+conformance group and other swap operations belong to the base32
+conformance group.
 
 Examples:
 
@@ -374,8 +388,8 @@  Examples:
 Jump instructions
 -----------------
 
-``BPF_JMP32`` uses 32-bit wide operands while ``BPF_JMP`` uses 64-bit wide operands for
-otherwise identical operations.
+``BPF_JMP32`` uses 32-bit wide operands and indicates the base32 conformance group, while ``BPF_JMP`` uses 64-bit wide operands for
+otherwise identical operations, and indicates the base64 conformance group unless otherwise specified.
 The 'code' field encodes the operation as below:
 
 ========  =====  ===  ===============================  =============================================
@@ -424,6 +438,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.
+
 Helper functions
 ~~~~~~~~~~~~~~~~
 
@@ -481,6 +498,8 @@  The size modifier is one of:
   BPF_DW         0x18   double word (8 bytes)
   =============  =====  =====================
 
+Instructions using ``BPF_DW`` belong to the base64 conformance group.
+
 Regular load and store operations
 ---------------------------------
 
@@ -525,8 +544,10 @@  by other BPF programs or means outside of this specification.
 All atomic operations supported by BPF are encoded as store operations
 that use the ``BPF_ATOMIC`` mode modifier as follows:
 
-* ``BPF_ATOMIC | BPF_W | BPF_STX`` for 32-bit operations
-* ``BPF_ATOMIC | BPF_DW | BPF_STX`` for 64-bit operations
+* ``BPF_ATOMIC | BPF_W | BPF_STX`` for 32-bit operations, which are
+  part of the "atom32" conformance group.
+* ``BPF_ATOMIC | BPF_DW | BPF_STX`` for 64-bit operations, which are
+  part of the "atom64" conformance group.
 * 8-bit and 16-bit wide atomic operations are not supported.
 
 The 'imm' field is used to encode the actual atomic operation.
@@ -637,5 +658,4 @@  Legacy BPF Packet access instructions
 BPF previously introduced special instructions for access to packet data that were
 carried over from classic BPF. However, these instructions are
 deprecated and should no longer be used.  All legacy packet access
-instructions belong to the "legacy" conformance group instead of the "basic"
-conformance group.
+instructions belong to the "legacy" conformance group.