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@xxxxxxxxxx>
> >
> > 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
