On 1/26/24 2:27 PM, dthaler1968@xxxxxxxxxxxxxx wrote:
Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
On 1/25/24 5:12 PM, dthaler1968@xxxxxxxxxxxxxx wrote:
The spec defines:
As discussed below in `64-bit immediate instructions`_, a 64-bit
immediate instruction uses a 64-bit immediate value that is constructed as
follows.
The 64 bits following the basic instruction contain a pseudo
instruction using the same format but with opcode, dst_reg, src_reg,
and offset all set to zero, and imm containing the high 32 bits of the
immediate value.
[...]
imm64 = (next_imm << 32) | imm
The 64-bit immediate instructions section then says:
Instructions with the ``BPF_IMM`` 'mode' modifier use the wide
instruction encoding defined in `Instruction encoding`_, and use the
'src' field of the basic instruction to hold an opcode subtype.
Some instructions then nicely state how to use the full 64 bit
immediate value, such as
BPF_IMM | BPF_DW | BPF_LD 0x18 0x0 dst = imm64
integer integer
BPF_IMM | BPF_DW | BPF_LD 0x18 0x2 dst = map_val(map_by_fd(imm))
+ next_imm map fd data pointer
BPF_IMM | BPF_DW | BPF_LD 0x18 0x6 dst = map_val(map_by_idx(imm))
+ next_imm map index data pointer
Others don't:
BPF_IMM | BPF_DW | BPF_LD 0x18 0x1 dst = map_by_fd(imm)
map fd map
BPF_IMM | BPF_DW | BPF_LD 0x18 0x3 dst = var_addr(imm)
variable id data pointer
BPF_IMM | BPF_DW | BPF_LD 0x18 0x4 dst = code_addr(imm)
integer code pointer
BPF_IMM | BPF_DW | BPF_LD 0x18 0x5 dst = map_by_idx(imm)
map index map
How is next_imm used in those four? Must it be 0? Or can it be anything and
it's ignored?
Or is it used for something?
The other four must have next_imm to be 0. No use of next_imm in thee four
insns kindly implies this.
See uapi bpf.h for details (search BPF_PSEUDO_MAP_FD).
Thanks for confirming. The "Instruction encoding" section has misleading text
in my opinion.
It nicely says:
Note that most instructions do not use all of the fields. Unused fields shall be cleared to zero.
But then goes on to say:
As discussed below in 64-bit immediate instructions (Section 4.4), a 64-bit immediate instruction
uses a 64-bit immediate value that is constructed as follows.
[...]
imm64 = (next_imm << 32) | imm
Under a normal English reading, that could imply that all 64-bit immediate instructions use imm64,
which is not the case. The whole imm64 discussion there only applies today to src=0 (though I
suppose it could be used by future 64-bit immediate instructions). Minimally I think
"a 64-bit immediate instruction uses" should be "some 64-bit immediate instructions use"
but at present there's only one.
It would actually be simpler to remove the imm64 text and just have the
definition of src 0x0 change from: "dst = imm64" to "dst = (next_imm << 32) | imm".
What do you think?
it does sound better. Something like below?
diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
index af43227b6ee4..fceacca46299 100644
--- a/Documentation/bpf/standardization/instruction-set.rst
+++ b/Documentation/bpf/standardization/instruction-set.rst
@@ -166,7 +166,7 @@ Note that most instructions do not use all of the fields.
Unused fields shall be cleared to zero.
As discussed below in `64-bit immediate instructions`_, a 64-bit immediate
-instruction uses a 64-bit immediate value that is constructed as follows.
+instruction uses two 32-bit immediate values that are constructed as follows.
The 64 bits following the basic instruction contain a pseudo instruction
using the same format but with opcode, dst_reg, src_reg, and offset all set to zero,
and imm containing the high 32 bits of the immediate value.
@@ -181,13 +181,8 @@ This is depicted in the following figure::
'--------------'
pseudo instruction
-Thus the 64-bit immediate value is constructed as follows:
-
- imm64 = (next_imm << 32) | imm
-
-where 'next_imm' refers to the imm value of the pseudo instruction
-following the basic instruction. The unused bytes in the pseudo
-instruction are reserved and shall be cleared to zero.
+Here, the imm value of the pseudo instruction is called 'next_imm'. The unused
+bytes in the pseudo instruction are reserved and shall be cleared to zero.
Instruction classes
-------------------
@@ -590,7 +585,7 @@ defined further below:
========================= ====== === ========================================= =========== ==============
opcode construction opcode src pseudocode imm type dst type
========================= ====== === ========================================= =========== ==============
-BPF_IMM | BPF_DW | BPF_LD 0x18 0x0 dst = imm64 integer integer
+BPF_IMM | BPF_DW | BPF_LD 0x18 0x0 dst = (next_imm << 32) | imm integer integer
BPF_IMM | BPF_DW | BPF_LD 0x18 0x1 dst = map_by_fd(imm) map fd map
BPF_IMM | BPF_DW | BPF_LD 0x18 0x2 dst = map_val(map_by_fd(imm)) + next_imm map fd data pointer
BPF_IMM | BPF_DW | BPF_LD 0x18 0x3 dst = var_addr(imm) variable id data pointer
Dave
