Re: [Bpf] [PATCH bpf-next] The original document has some inconsistency.

[[e-mail]]

David Vernet <[e-mail]> writes: 
> Hi Aoyang,
> 
> Thanks a lot for your contribution. I agree that we need to fix the
document
> to be consistent, though I'm afraid that I think this patch goes in the
wrong
> direction by making everything match the jump instruction class. More
below.

I disagree, and I agree with Aoyang's direction.

> nit: Could you please update the patch subject to be more self-describing.
For
> example, something like:
> 
> Use consistent numerical widths in instructions.rst encodings

I agree with that subject.

> > For example:
> > 1. 1.3.1 Arithmetic instructions use '8 bits length' encoding to
> >    express the 'code' value, e.g., BPF_ADD=0x00, BPF_SUB=0x10,
> >    BPF_MUL=0x20. However the length of the 'code' is 4 bits. On the
> >    other hand, 1.3.3 Jump instructions use '4 bits length' encoding,
> >    e.g., BPF_JEQ=0x1 and BPF_JGT=0x2.
> > 2. There are also many places that use '8 bits length' encoding to
> >    express the corresponding contents, e.g., 1.4 Load and store
> >    instructions, BPF_ABS=0x20, BPF_IND=0x40. However, the length of
> >    'mode modifier' is 3 bits.
> >
> > To summarize, the only place that has inconsistent encoding is Jump
> > instructions. After discussing with Dave, [e-mail],
> > we agree that the document should be more clear.
> >
> > Signed-off-by: Aoyang Fang <[e-mail]>
> >
> > ---
> >  .../bpf/standardization/instruction-set.rst   | 170 +++++++++---------
> >  1 file changed, 85 insertions(+), 85 deletions(-)
> >
> > diff --git a/Documentation/bpf/standardization/instruction-set.rst
> > b/Documentation/bpf/standardization/instruction-set.rst
> > index 245b6defc..57dd1fa00 100644
> > --- a/Documentation/bpf/standardization/instruction-set.rst
> > +++ b/Documentation/bpf/standardization/instruction-set.rst
> > @@ -172,18 +172,18 @@ Instruction classes
> >
> >  The three LSB bits of the 'opcode' field store the instruction class:
> >
> > -=========  =====  ===============================
> ===================================
> > -class      value  description                      reference
> > -=========  =====  ===============================
> ===================================
> > -BPF_LD     0x00   non-standard load operations     `Load and store
> instructions`_
> > -BPF_LDX    0x01   load into register operations    `Load and store
> instructions`_
> > -BPF_ST     0x02   store from immediate operations  `Load and store
> instructions`_
> > -BPF_STX    0x03   store from register operations   `Load and store
> instructions`_
> > -BPF_ALU    0x04   32-bit arithmetic operations     `Arithmetic and jump
> instructions`_
> > -BPF_JMP    0x05   64-bit jump operations           `Arithmetic and jump
> instructions`_
> > -BPF_JMP32  0x06   32-bit jump operations           `Arithmetic and jump
> instructions`_
> > -BPF_ALU64  0x07   64-bit arithmetic operations     `Arithmetic and jump
> instructions`_
> > -=========  =====  ===============================
> > ===================================
> > +=========  =============  ===============================
> ===================================
> > +class      value(3 bits)  description                      reference
> > +=========  =============  ===============================
> ===================================
> > +BPF_LD     0x0            non-standard load operations     `Load and
store
> instructions`_
> > +BPF_LDX    0x1            load into register operations    `Load and
store
> instructions`_
> > +BPF_ST     0x2            store from immediate operations  `Load and
store
> instructions`_
> > +BPF_STX    0x3            store from register operations   `Load and
store
> instructions`_
> > +BPF_ALU    0x4            32-bit arithmetic operations     `Arithmetic
and jump
> instructions`_
> > +BPF_JMP    0x5            64-bit jump operations           `Arithmetic
and jump
> instructions`_
> > +BPF_JMP32  0x6            32-bit jump operations           `Arithmetic
and jump
> instructions`_
> > +BPF_ALU64  0x7            64-bit arithmetic operations     `Arithmetic
and jump
> instructions`_
> > +=========  =============  ===============================
> > +===================================
> 
> Hmm, I presonally think this is more confusing. The opcode field is 8
bits. We
> already specify that the value is the three LSB of the opcode field. It's
> certainly subjective, but I think we should have the value reflect the
actual
> value in the field it's embedded in. In my opinion, changing the value to
not
> reflect its place in the actual opcode in my opinion imposes a burden on
the
> reader to go back and reference where the field actually belongs in the
full
> opcode. It's a tradeoff, but I think we're already on the winning end of
that
> tradeoff.

This document is an IETF standards specification so it's worth looking at
what
typical RFC conventions are.

* RFC 791 section 3.1 defines the IPv4 header, where the Version field is
the high
   4 bits of a byte.  It defines the value as 4, not 0x40.
   It also defines the Type of Service bits which are 1 bit fields with
value 0 or 1
   (not, say 0x40).
* RFC 8200 section 3 defines the IPv6 header, where the Version field is the
high
   4 bits of a byte.  It defines the value as 6, not 0x60.

Etc.  Offhand I am not aware of any RFC that uses the convention you
suggest,
though perhaps others are?

> >  Arithmetic and jump instructions
> >  ================================
> > @@ -203,12 +203,12 @@ code            source  instruction class
> >  **source**
> >    the source operand location, which unless otherwise specified is one
of:
> >
> > -  ======  =====  ==============================================
> > -  source  value  description
> > -  ======  =====  ==============================================
> > -  BPF_K   0x00   use 32-bit 'imm' value as source operand
> > -  BPF_X   0x08   use 'src_reg' register value as source operand
> > -  ======  =====  ==============================================
> > +  ======  ============
> > + ==============================================
> > +  source  value(1 bit)  description
> > +  ======  ============
> ==============================================
> > +  BPF_K   0x0           use 32-bit 'imm' value as source operand
> > +  BPF_X   0x1           use 'src_reg' register value as source operand
> > +  ======  ============
> > + ==============================================
> 
> Same here as well. The value isn't really 0x1, it's 0x8. And 0x08 is even
more
> clear yet, given that we're representing the value of the bit in the 8 bit
opcode
> field.

Its 1, in the same sense as the TOS bits in RFC 791 are 1.

> >  **instruction class**
> >    the instruction class (see `Instruction classes`_) @@ -221,27
> > +221,27 @@ otherwise identical operations.
> >  The 'code' field encodes the operation as below, where 'src' and
> > 'dst' refer  to the values of the source and destination registers,
respectively.
> >
> > -=========  =====  =======
> ==========================================================
> > -code       value  offset   description
> > -=========  =====  =======
> ==========================================================
> > -BPF_ADD    0x00   0        dst += src
> > -BPF_SUB    0x10   0        dst -= src
> > -BPF_MUL    0x20   0        dst \*= src
> > -BPF_DIV    0x30   0        dst = (src != 0) ? (dst / src) : 0
> > -BPF_SDIV   0x30   1        dst = (src != 0) ? (dst s/ src) : 0
> > -BPF_OR     0x40   0        dst \|= src
> > -BPF_AND    0x50   0        dst &= src
> > -BPF_LSH    0x60   0        dst <<= (src & mask)
> > -BPF_RSH    0x70   0        dst >>= (src & mask)
> > -BPF_NEG    0x80   0        dst = -dst
> > -BPF_MOD    0x90   0        dst = (src != 0) ? (dst % src) : dst
> > -BPF_SMOD   0x90   1        dst = (src != 0) ? (dst s% src) : dst
> > -BPF_XOR    0xa0   0        dst ^= src
> > -BPF_MOV    0xb0   0        dst = src
> > -BPF_MOVSX  0xb0   8/16/32  dst = (s8,s16,s32)src
> > -BPF_ARSH   0xc0   0        :term:`sign extending<Sign Extend>` dst >>=
(src &
> mask)
> > -BPF_END    0xd0   0        byte swap operations (see `Byte swap
instructions`_
> below)
> >
> > -=========  =====  =======
> > ==========================================================
> > +=========  =============  =======
> ==========================================================
> > +code       value(4 bits)  offset   description
> > +=========  =============  =======
> ==========================================================
> > +BPF_ADD    0x0            0        dst += src
> > +BPF_SUB    0x1            0        dst -= src
> > +BPF_MUL    0x2            0        dst \*= src
> > +BPF_DIV    0x3            0        dst = (src != 0) ? (dst / src) : 0
> > +BPF_SDIV   0x3            1        dst = (src != 0) ? (dst s/ src) : 0
> > +BPF_OR     0x4            0        dst \|= src
> > +BPF_AND    0x5            0        dst &= src
> > +BPF_LSH    0x6            0        dst <<= (src & mask)
> > +BPF_RSH    0x7            0        dst >>= (src & mask)
> > +BPF_NEG    0x8            0        dst = -dst
> > +BPF_MOD    0x9            0        dst = (src != 0) ? (dst % src) : dst
> > +BPF_SMOD   0x9            1        dst = (src != 0) ? (dst s% src) :
dst
> > +BPF_XOR    0xa            0        dst ^= src
> > +BPF_MOV    0xb            0        dst = src
> > +BPF_MOVSX  0xb            8/16/32  dst = (s8,s16,s32)src
> > +BPF_ARSH   0xc            0        :term:`sign extending<Sign Extend>`
dst >>=
> (src & mask)
> > +BPF_END    0xd            0        byte swap operations (see `Byte swap
> instructions`_ below)
> > +=========  =============  =======
> > +==========================================================
> 
> Same here.
> 
> >  Underflow and overflow are allowed during arithmetic operations,
> > meaning  the 64-bit or 32-bit value will wrap. If BPF program
> > execution would @@ -314,13 +314,13 @@ select what byte order the
> > operation converts from or to. For  ``BPF_ALU64``, the 1-bit source
> > operand field in the opcode is reserved  and must be set to 0.
> >
> > -=========  =========  =====
> =================================================
> > -class      source     value  description
> > -=========  =========  =====
> =================================================
> > -BPF_ALU    BPF_TO_LE  0x00   convert between host byte order and little
> endian
> > -BPF_ALU    BPF_TO_BE  0x08   convert between host byte order and big
> endian
> > -BPF_ALU64  Reserved   0x00   do byte swap unconditionally
> > -=========  =========  =====
> > =================================================
> > +=========  =========  ============
> =================================================
> > +class      source     value(1 bit)  description
> > +=========  =========  ============
> =================================================
> > +BPF_ALU    BPF_TO_LE  0x0           convert between host byte order and
little
> endian
> > +BPF_ALU    BPF_TO_BE  0x1           convert between host byte order and
big
> endian
> > +BPF_ALU64  Reserved   0x0           do byte swap unconditionally
> > +=========  =========  ============
> > +=================================================
> 
> Same here. Which bit does the 0x1 actually correspond to? It's
self-evident in
> the former, not the latter.

Would you then say that RFC 791 (and many RFCs since) is not self-evident?

If the WG chooses to diverge from the most common ways the IETF defines
bit formats, that might be ok but may need a section explaining the
divergent convention.   My personal preference though is to stay consistent
with the normal IETF convention, which part of the ISA doc already did.

Dave