Re: [PATCH bpf-next v2 15/15] docs/bpf: Add documentation for new instructions

From: Yonghong Song <yhs@meta.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Fangrui Song <maskray@google.com>,
	kernel-team@fb.com
Subject: Re: [PATCH bpf-next v2 15/15] docs/bpf: Add documentation for new instructions
Date: Fri, 14 Jul 2023 16:26:12 -0700	[thread overview]
Message-ID: <5d530e78-a532-3544-f005-d33c962c61a6@meta.com> (raw)
In-Reply-To: <20230714182805.rvfrf4y4ctmrqbav@MacBook-Pro-8.local>

On 7/14/23 11:28 AM, Alexei Starovoitov wrote:
> On Wed, Jul 12, 2023 at 11:08:47PM -0700, Yonghong Song wrote:
>> diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
>> index 751e657973f0..367f426d09a1 100644
>> --- a/Documentation/bpf/standardization/instruction-set.rst
>> +++ b/Documentation/bpf/standardization/instruction-set.rst
>> @@ -154,24 +154,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  description
>> -========  =====  ==========================================================
>> -BPF_ADD   0x00   dst += src
>> -BPF_SUB   0x10   dst -= src
>> -BPF_MUL   0x20   dst \*= src
>> -BPF_DIV   0x30   dst = (src != 0) ? (dst / src) : 0
>> -BPF_OR    0x40   dst \|= src
>> -BPF_AND   0x50   dst &= src
>> -BPF_LSH   0x60   dst <<= (src & mask)
>> -BPF_RSH   0x70   dst >>= (src & mask)
>> -BPF_NEG   0x80   dst = -src
>> -BPF_MOD   0x90   dst = (src != 0) ? (dst % src) : dst
>> -BPF_XOR   0xa0   dst ^= src
>> -BPF_MOV   0xb0   dst = src
>> -BPF_ARSH  0xc0   sign extending dst >>= (src & mask)
>> -BPF_END   0xd0   byte swap operations (see `Byte swap instructions`_ below)
>> -========  =====  ==========================================================
>> +========  =====  ============  ==========================================================
>> +code      value  offset value  description
> 
> How about just 'offset' ?

Ack.

> 
>> +========  =====  ============  ==========================================================
>> +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 = -src
>> +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,16,s32)src
>> +BPF_ARSH  0xc0   0             sign extending dst >>= (src & mask)
>> +BPF_END   0xd0   0             byte swap operations (see `Byte swap instructions`_ below)
>> +========  =====  ============  ==========================================================
>>   
>>   Underflow and overflow are allowed during arithmetic operations, meaning
>>   the 64-bit or 32-bit value will wrap. If eBPF program execution would
>> @@ -198,11 +201,19 @@ where '(u32)' indicates that the upper 32 bits are zeroed.
>>   
>>     dst = dst ^ imm32
>>   
>> -Also note that the division and modulo operations are unsigned. Thus, for
>> -``BPF_ALU``, 'imm' is first interpreted as an unsigned 32-bit value, whereas
>> -for ``BPF_ALU64``, 'imm' is first sign extended to 64 bits and the result
>> -interpreted as an unsigned 64-bit value. There are no instructions for
>> -signed division or modulo.
>> +Note that most instructions have instruction offset of 0. But three instructions
>> +(BPF_SDIV, BPF_SMOD, BPF_MOVSX) have non-zero offset.
>> +
>> +The devision and modulo operations support both unsigned and signed flavors.
>> +For unsigned operation (BPF_DIV and BPF_MOD), for ``BPF_ALU``, 'imm' is first
>> +interpreted as an unsigned 32-bit value, whereas for ``BPF_ALU64``, 'imm' is
>> +first sign extended to 64 bits and the result interpreted as an unsigned 64-bit
>> +value.  For signed operation (BPF_SDIV and BPF_SMOD), for both ``BPF_ALU`` and
>> +``BPF_ALU64``, 'imm' is interpreted as a signed value.
> 
> Probably worth clarifying that in case of S[DIV|MOD] | ALU64 the imm is sign
> extended from 32 to 64 and interpreted as signed 64-bit.

I thought this before but think "'imm' is interpreted as a signed value"
might be good enough since in this case BPF_ALU and BPF_ALU64 should 
have the same signed 'imm' value. But what you suggested will make it
more clearer. Will do.

> 
>> +
>> +Instruction BPF_MOVSX does move operation with sign extension. For ``BPF_ALU``
>> +mode, 8-bit and 16-bit sign extensions to 32-bit are supported. For ``BPF_ALU64``,
>> +8-bit, 16-bit and 32-bit sign extenstions to 64-bit are supported.
> 
> How about:
> 
> Instruction BPF_MOVSX does move operation with sign extension.
> BPF_ALU | MOVSX sign extendes 8-bit and 16-bit into 32-bit and upper 32-bit are zeroed.
> BPF_ALU64 | MOVSX sign extends 8-bit, 16-bit and 32-bit into 64-bit.

In previous doc, we already says that BPF_ALU operation has upper
32-bit zeroed. But since this sign extension is special, agree it
is worthwhile to mention upper 32-bit zeroed explictly.

> 
>>   
>> +``BPF_ALU64 | BPF_TO_LE | BPF_END`` with imm = 16 means::
>> +
>> +  dst = bswap16(dst)
> 
> Worth spelling out imm 32 and 64 too ?

Will do.

> 
>>   
>> +The ``BPF_MEMSX`` mode modifier is used to encode sign-extension load
>> +instructions that transfer data between a register and memory.
>> +
>> +``BPF_MEMSX | <size> | BPF_LDX`` means::
>> +
>> +  dst = *(sign-extension size *) (src + offset)
>> +
> 
> How about:
> 
> ``BPF_MEM | <size> | BPF_LDX`` means::
> 
>    dst = *(unsigned size *) (src + offset)
> 
> ``BPF_MEMSX | <size> | BPF_LDX`` means::
> 
>    dst = *(signed size *) (src + offset)

Will do.
> 