| Message ID | 87y1oj7yvu.fsf@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [V2] bpf, docs: Document BPF insn encoding in term of stored bytes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Feb 27, 2023 at 07:21:25PM +0100, Jose E. Marchesi wrote: > > [Differences from V1: > - Use rst literal blocks for figures. > - Avoid using | in the basic instruction/pseudo instruction figure. > - Rebased to today's bpf-next master branch.] > > This patch modifies instruction-set.rst so it documents the encoding > of BPF instructions in terms of how the bytes are stored (be it in an > ELF file or as bytes in a memory buffer to be loaded into the kernel > or some other BPF consumer) as opposed to how the instruction looks > like once loaded. > > This is hopefully easier to understand by implementors looking to > generate and/or consume bytes conforming BPF instructions. > > The patch also clarifies that the unused bytes in a pseudo-instruction > shall be cleared with zeros. > > Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com> Thanks Jose, this looks a lot better. Just left a couple more small suggestions + questions below before stamping. > > --- > Documentation/bpf/instruction-set.rst | 44 +++++++++++++-------------- > 1 file changed, 22 insertions(+), 22 deletions(-) > > diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst > index 01802ed9b29b..3341bfe20e4d 100644 > --- a/Documentation/bpf/instruction-set.rst > +++ b/Documentation/bpf/instruction-set.rst > @@ -38,15 +38,13 @@ eBPF has two instruction encodings: > * the wide instruction encoding, which appends a second 64-bit immediate (i.e., > constant) value after the basic instruction for a total of 128 bits. > > -The basic instruction encoding looks as follows for a little-endian processor, > -where MSB and LSB mean the most significant bits and least significant bits, > -respectively: > +The fields conforming an encoded basic instruction are stored in the > +following order:: > > -============= ======= ======= ======= ============ > -32 bits (MSB) 16 bits 4 bits 4 bits 8 bits (LSB) > -============= ======= ======= ======= ============ > -imm offset src_reg dst_reg opcode > -============= ======= ======= ======= ============ > + opcode:8 src:4 dst:4 offset:16 imm:32 // In little-endian BPF. > + opcode:8 dst:4 src:4 offset:16 imm:32 // In big-endian BPF. The terms below use src_reg and dst_reg. Can we either update these code blocks to match, or change the term definitions to "src" and "dst"? I'd vote for the latter, given that we explain that it's the source / destination register number where they're defined. > + > +Where, IMO, we can probably remove this "Where,". I think it's pretty clear that the following terms are referring to the code block above. Wdyt? > > **imm** > signed integer immediate value > @@ -64,16 +62,17 @@ imm offset src_reg dst_reg opcode > **opcode** > operation to perform > > -and as follows for a big-endian processor: > +Note that the contents of multi-byte fields ('imm' and 'offset') are > +stored using big-endian byte ordering in big-endian BPF and > +little-endian byte ordering in little-endian BPF. > > -============= ======= ======= ======= ============ > -32 bits (MSB) 16 bits 4 bits 4 bits 8 bits (LSB) > -============= ======= ======= ======= ============ > -imm offset dst_reg src_reg opcode > -============= ======= ======= ======= ============ > +For example:: > > -Multi-byte fields ('imm' and 'offset') are similarly stored in > -the byte order of the processor. > + opcode offset imm assembly > + src dst > + 07 0 1 00 00 44 33 22 11 r1 += 0x11223344 // little > + dst src > + 07 1 0 00 00 11 22 33 44 r1 += 0x11223344 // big > > Note that most instructions do not use all of the fields. > Unused fields shall be cleared to zero. > @@ -84,18 +83,19 @@ 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. > > -================= ================== > -64 bits (MSB) 64 bits (LSB) > -================= ================== > -basic instruction pseudo instruction > -================= ================== > +This is depicted in the following figure:: > + > + basic_instruction pseudo instruction > + ------------------------------- ------------------ > + code:8 regs:16 offset:16 imm:32 unused:32 imm:32 Don't want to bikeshed too much, but this seems a bit hard to read. It kind of all looks like one line, though perhaps that was the intention. Wdyt about this? This is depicted in the following figure:: code:8 regs:16 offset:16 imm:32 // MSB: basic instruction reserved:32 imm:32 // LSB: 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. > +following the basic instruction. The unused bytes in the pseudo > +instruction shall be cleared to zero. Also apologies if this is also a bikeshed and has already been discussed, but should we say, "The unused bits in the pseudo instruction are reserved" rather than saying they should be cleared to zero? Implemenations should interpret "reserved" to mean that the bits should be zeroed, no? Or at least that seems like the norm in technical manuals. For example, reserved bits in control registers on x86 should be cleared to avoid unexpected side effects if and when those bits are eventually actually used for something in future releases. Thanks, David
> On Mon, Feb 27, 2023 at 07:21:25PM +0100, Jose E. Marchesi wrote: >> >> [Differences from V1: >> - Use rst literal blocks for figures. >> - Avoid using | in the basic instruction/pseudo instruction figure. >> - Rebased to today's bpf-next master branch.] >> >> This patch modifies instruction-set.rst so it documents the encoding >> of BPF instructions in terms of how the bytes are stored (be it in an >> ELF file or as bytes in a memory buffer to be loaded into the kernel >> or some other BPF consumer) as opposed to how the instruction looks >> like once loaded. >> >> This is hopefully easier to understand by implementors looking to >> generate and/or consume bytes conforming BPF instructions. >> >> The patch also clarifies that the unused bytes in a pseudo-instruction >> shall be cleared with zeros. >> >> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com> > > Thanks Jose, this looks a lot better. Just left a couple more small > suggestions + questions below before stamping. > >> >> --- >> Documentation/bpf/instruction-set.rst | 44 +++++++++++++-------------- >> 1 file changed, 22 insertions(+), 22 deletions(-) >> >> diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst >> index 01802ed9b29b..3341bfe20e4d 100644 >> --- a/Documentation/bpf/instruction-set.rst >> +++ b/Documentation/bpf/instruction-set.rst >> @@ -38,15 +38,13 @@ eBPF has two instruction encodings: >> * the wide instruction encoding, which appends a second 64-bit immediate (i.e., >> constant) value after the basic instruction for a total of 128 bits. >> >> -The basic instruction encoding looks as follows for a little-endian processor, >> -where MSB and LSB mean the most significant bits and least significant bits, >> -respectively: >> +The fields conforming an encoded basic instruction are stored in the >> +following order:: >> >> -============= ======= ======= ======= ============ >> -32 bits (MSB) 16 bits 4 bits 4 bits 8 bits (LSB) >> -============= ======= ======= ======= ============ >> -imm offset src_reg dst_reg opcode >> -============= ======= ======= ======= ============ >> + opcode:8 src:4 dst:4 offset:16 imm:32 // In little-endian BPF. >> + opcode:8 dst:4 src:4 offset:16 imm:32 // In big-endian BPF. > > The terms below use src_reg and dst_reg. Can we either update these code > blocks to match, or change the term definitions to "src" and "dst"? I'd > vote for the latter, given that we explain that it's the source / > destination register number where they're defined. Will change. >> + >> +Where, > > IMO, we can probably remove this "Where,". I think it's pretty clear > that the following terms are referring to the code block above. Wdyt? I added the "Where," to make the reading a little bit more fluid, but I don't have a strong opinion on that. > >> >> **imm** >> signed integer immediate value >> @@ -64,16 +62,17 @@ imm offset src_reg dst_reg opcode >> **opcode** >> operation to perform >> >> -and as follows for a big-endian processor: >> +Note that the contents of multi-byte fields ('imm' and 'offset') are >> +stored using big-endian byte ordering in big-endian BPF and >> +little-endian byte ordering in little-endian BPF. >> >> -============= ======= ======= ======= ============ >> -32 bits (MSB) 16 bits 4 bits 4 bits 8 bits (LSB) >> -============= ======= ======= ======= ============ >> -imm offset dst_reg src_reg opcode >> -============= ======= ======= ======= ============ >> +For example:: >> >> -Multi-byte fields ('imm' and 'offset') are similarly stored in >> -the byte order of the processor. >> + opcode offset imm assembly >> + src dst >> + 07 0 1 00 00 44 33 22 11 r1 += 0x11223344 // little >> + dst src >> + 07 1 0 00 00 11 22 33 44 r1 += 0x11223344 // big >> >> Note that most instructions do not use all of the fields. >> Unused fields shall be cleared to zero. >> @@ -84,18 +83,19 @@ 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. >> >> -================= ================== >> -64 bits (MSB) 64 bits (LSB) >> -================= ================== >> -basic instruction pseudo instruction >> -================= ================== >> +This is depicted in the following figure:: >> + >> + basic_instruction pseudo instruction >> + ------------------------------- ------------------ >> + code:8 regs:16 offset:16 imm:32 unused:32 imm:32 > > Don't want to bikeshed too much, but this seems a bit hard to read. It > kind of all looks like one line, though perhaps that was the intention. > Wdyt about this? > > This is depicted in the following figure:: > > code:8 regs:16 offset:16 imm:32 // MSB: basic instruction > reserved:32 imm:32 // LSB: pseudo instruction Hmm, yes, that was the intention, to depict the fact that a 128-bit instruction is composed by the sequence of a basic instruction followed by a pseudo instruction... This would be the bombastic ASCII-art version of what I had in mind :) basic_instruction .-----------------------------. | | code:8 regs:16 offset:16 imm:32 unused:32 imm:32 | | '--------------' pseudo instruction I don't think MSB and LSB marks are meaningful in this context. Bytes in a file or in memory are no more or less significative: they just have addresses (or file offsets) which can be lower or higher than the addresses (or file offsets) of other bytes. >> >> 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. >> +following the basic instruction. The unused bytes in the pseudo >> +instruction shall be cleared to zero. > > Also apologies if this is also a bikeshed and has already been > discussed, but should we say, "The unused bits in the pseudo instruction > are reserved" rather than saying they should be cleared to zero? > Implemenations should interpret "reserved" to mean that the bits should > be zeroed, no? Or at least that seems like the norm in technical > manuals. For example, reserved bits in control registers on x86 should > be cleared to avoid unexpected side effects if and when those bits are > eventually actually used for something in future releases. That is a good point. I agree that "reserved" almost always means zero, but I think it is very common to say so explicitly. A couple of examples: Example 1, from the x86 MBR spec: * Offset Size (bytes) Description * * 0x000 440 MBR Bootstrap (flat binary executable code) * 0x1B8 4 Optional "Unique Disk ID / Signature" * 0x1BC 2 Optional, reserved 0x0000 * 0x1BE 16 First partition table entry * 0x1CE 16 Second partition table entry * 0x1DE 16 Third partition table entry * 0x1EE 16 Fourth partition table entry * 0x1FE 2 (0x55, 0xAA) "Valid bootsector" signature bytes Example 2, this is how the PE Object File specification defines a reserved field: reserved, A description of a field that indicates that the value must be 0 of the field must be zero for generators and consumers must ignore the field. What about this: "The unused bits in the pseudo instruction are reserved and shall be cleared to zero."
On Mon, Feb 27, 2023 at 08:31:33PM +0100, Jose E. Marchesi wrote: > > > On Mon, Feb 27, 2023 at 07:21:25PM +0100, Jose E. Marchesi wrote: > >> > >> [Differences from V1: > >> - Use rst literal blocks for figures. > >> - Avoid using | in the basic instruction/pseudo instruction figure. > >> - Rebased to today's bpf-next master branch.] > >> > >> This patch modifies instruction-set.rst so it documents the encoding > >> of BPF instructions in terms of how the bytes are stored (be it in an > >> ELF file or as bytes in a memory buffer to be loaded into the kernel > >> or some other BPF consumer) as opposed to how the instruction looks > >> like once loaded. > >> > >> This is hopefully easier to understand by implementors looking to > >> generate and/or consume bytes conforming BPF instructions. > >> > >> The patch also clarifies that the unused bytes in a pseudo-instruction > >> shall be cleared with zeros. > >> > >> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com> > > > > Thanks Jose, this looks a lot better. Just left a couple more small > > suggestions + questions below before stamping. > > > >> > >> --- > >> Documentation/bpf/instruction-set.rst | 44 +++++++++++++-------------- > >> 1 file changed, 22 insertions(+), 22 deletions(-) > >> > >> diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst > >> index 01802ed9b29b..3341bfe20e4d 100644 > >> --- a/Documentation/bpf/instruction-set.rst > >> +++ b/Documentation/bpf/instruction-set.rst > >> @@ -38,15 +38,13 @@ eBPF has two instruction encodings: > >> * the wide instruction encoding, which appends a second 64-bit immediate (i.e., > >> constant) value after the basic instruction for a total of 128 bits. > >> > >> -The basic instruction encoding looks as follows for a little-endian processor, > >> -where MSB and LSB mean the most significant bits and least significant bits, > >> -respectively: > >> +The fields conforming an encoded basic instruction are stored in the > >> +following order:: > >> > >> -============= ======= ======= ======= ============ > >> -32 bits (MSB) 16 bits 4 bits 4 bits 8 bits (LSB) > >> -============= ======= ======= ======= ============ > >> -imm offset src_reg dst_reg opcode > >> -============= ======= ======= ======= ============ > >> + opcode:8 src:4 dst:4 offset:16 imm:32 // In little-endian BPF. > >> + opcode:8 dst:4 src:4 offset:16 imm:32 // In big-endian BPF. > > > > The terms below use src_reg and dst_reg. Can we either update these code > > blocks to match, or change the term definitions to "src" and "dst"? I'd > > vote for the latter, given that we explain that it's the source / > > destination register number where they're defined. > > Will change. > > >> + > >> +Where, > > > > IMO, we can probably remove this "Where,". I think it's pretty clear > > that the following terms are referring to the code block above. Wdyt? > > I added the "Where," to make the reading a little bit more fluid, but I > don't have a strong opinion on that. > > > > >> > >> **imm** > >> signed integer immediate value > >> @@ -64,16 +62,17 @@ imm offset src_reg dst_reg opcode > >> **opcode** > >> operation to perform > >> > >> -and as follows for a big-endian processor: > >> +Note that the contents of multi-byte fields ('imm' and 'offset') are > >> +stored using big-endian byte ordering in big-endian BPF and > >> +little-endian byte ordering in little-endian BPF. > >> > >> -============= ======= ======= ======= ============ > >> -32 bits (MSB) 16 bits 4 bits 4 bits 8 bits (LSB) > >> -============= ======= ======= ======= ============ > >> -imm offset dst_reg src_reg opcode > >> -============= ======= ======= ======= ============ > >> +For example:: > >> > >> -Multi-byte fields ('imm' and 'offset') are similarly stored in > >> -the byte order of the processor. > >> + opcode offset imm assembly > >> + src dst > >> + 07 0 1 00 00 44 33 22 11 r1 += 0x11223344 // little > >> + dst src > >> + 07 1 0 00 00 11 22 33 44 r1 += 0x11223344 // big > >> > >> Note that most instructions do not use all of the fields. > >> Unused fields shall be cleared to zero. > >> @@ -84,18 +83,19 @@ 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. > >> > >> -================= ================== > >> -64 bits (MSB) 64 bits (LSB) > >> -================= ================== > >> -basic instruction pseudo instruction > >> -================= ================== > >> +This is depicted in the following figure:: > >> + > >> + basic_instruction pseudo instruction > >> + ------------------------------- ------------------ > >> + code:8 regs:16 offset:16 imm:32 unused:32 imm:32 > > > > Don't want to bikeshed too much, but this seems a bit hard to read. It > > kind of all looks like one line, though perhaps that was the intention. > > Wdyt about this? > > > > This is depicted in the following figure:: > > > > code:8 regs:16 offset:16 imm:32 // MSB: basic instruction > > reserved:32 imm:32 // LSB: pseudo instruction > > Hmm, yes, that was the intention, to depict the fact that a 128-bit > instruction is composed by the sequence of a basic instruction followed > by a pseudo instruction... > > This would be the bombastic ASCII-art version of what I had in mind :) > > basic_instruction > .-----------------------------. > | | > code:8 regs:16 offset:16 imm:32 unused:32 imm:32 > | | > '--------------' > pseudo instruction Oh man, I love ASCII-art, bombastic or not. I personally prefer this over the single-line version, but I think either way is fine. It's pretty clear what it's conveying. > > I don't think MSB and LSB marks are meaningful in this context. Bytes > in a file or in memory are no more or less significative: they just have > addresses (or file offsets) which can be lower or higher than the > addresses (or file offsets) of other bytes. Fair enough, let's leave off MSB / LSB. > >> > >> 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. > >> +following the basic instruction. The unused bytes in the pseudo > >> +instruction shall be cleared to zero. > > > > Also apologies if this is also a bikeshed and has already been > > discussed, but should we say, "The unused bits in the pseudo instruction > > are reserved" rather than saying they should be cleared to zero? > > Implemenations should interpret "reserved" to mean that the bits should > > be zeroed, no? Or at least that seems like the norm in technical > > manuals. For example, reserved bits in control registers on x86 should > > be cleared to avoid unexpected side effects if and when those bits are > > eventually actually used for something in future releases. > > That is a good point. > > I agree that "reserved" almost always means zero, but I think it is very > common to say so explicitly. A couple of examples: Someone should standardize how manuals describe reserved bits ;-) > Example 1, from the x86 MBR spec: > > * Offset Size (bytes) Description > * > * 0x000 440 MBR Bootstrap (flat binary executable code) > * 0x1B8 4 Optional "Unique Disk ID / Signature" > * 0x1BC 2 Optional, reserved 0x0000 > * 0x1BE 16 First partition table entry > * 0x1CE 16 Second partition table entry > * 0x1DE 16 Third partition table entry > * 0x1EE 16 Fourth partition table entry > * 0x1FE 2 (0x55, 0xAA) "Valid bootsector" signature bytes > > Example 2, this is how the PE Object File specification defines a > reserved field: > > reserved, A description of a field that indicates that the value > must be 0 of the field must be zero for generators and consumers > must ignore the field. > > What about this: "The unused bits in the pseudo instruction are reserved > and shall be cleared to zero." Sounds good to me! Thanks, David
diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst index 01802ed9b29b..3341bfe20e4d 100644 --- a/Documentation/bpf/instruction-set.rst +++ b/Documentation/bpf/instruction-set.rst @@ -38,15 +38,13 @@ eBPF has two instruction encodings: * the wide instruction encoding, which appends a second 64-bit immediate (i.e., constant) value after the basic instruction for a total of 128 bits. -The basic instruction encoding looks as follows for a little-endian processor, -where MSB and LSB mean the most significant bits and least significant bits, -respectively: +The fields conforming an encoded basic instruction are stored in the +following order:: -============= ======= ======= ======= ============ -32 bits (MSB) 16 bits 4 bits 4 bits 8 bits (LSB) -============= ======= ======= ======= ============ -imm offset src_reg dst_reg opcode -============= ======= ======= ======= ============ + opcode:8 src:4 dst:4 offset:16 imm:32 // In little-endian BPF. + opcode:8 dst:4 src:4 offset:16 imm:32 // In big-endian BPF. + +Where, **imm** signed integer immediate value @@ -64,16 +62,17 @@ imm offset src_reg dst_reg opcode **opcode** operation to perform -and as follows for a big-endian processor: +Note that the contents of multi-byte fields ('imm' and 'offset') are +stored using big-endian byte ordering in big-endian BPF and +little-endian byte ordering in little-endian BPF. -============= ======= ======= ======= ============ -32 bits (MSB) 16 bits 4 bits 4 bits 8 bits (LSB) -============= ======= ======= ======= ============ -imm offset dst_reg src_reg opcode -============= ======= ======= ======= ============ +For example:: -Multi-byte fields ('imm' and 'offset') are similarly stored in -the byte order of the processor. + opcode offset imm assembly + src dst + 07 0 1 00 00 44 33 22 11 r1 += 0x11223344 // little + dst src + 07 1 0 00 00 11 22 33 44 r1 += 0x11223344 // big Note that most instructions do not use all of the fields. Unused fields shall be cleared to zero. @@ -84,18 +83,19 @@ 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. -================= ================== -64 bits (MSB) 64 bits (LSB) -================= ================== -basic instruction pseudo instruction -================= ================== +This is depicted in the following figure:: + + basic_instruction pseudo instruction + ------------------------------- ------------------ + code:8 regs:16 offset:16 imm:32 unused:32 imm:32 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. +following the basic instruction. The unused bytes in the pseudo +instruction shall be cleared to zero. Instruction classes -------------------
[Differences from V1: - Use rst literal blocks for figures. - Avoid using | in the basic instruction/pseudo instruction figure. - Rebased to today's bpf-next master branch.] This patch modifies instruction-set.rst so it documents the encoding of BPF instructions in terms of how the bytes are stored (be it in an ELF file or as bytes in a memory buffer to be loaded into the kernel or some other BPF consumer) as opposed to how the instruction looks like once loaded. This is hopefully easier to understand by implementors looking to generate and/or consume bytes conforming BPF instructions. The patch also clarifies that the unused bytes in a pseudo-instruction shall be cleared with zeros. Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com> --- Documentation/bpf/instruction-set.rst | 44 +++++++++++++-------------- 1 file changed, 22 insertions(+), 22 deletions(-)