On 5/10/23 8:45 AM, Dave Thaler wrote:
Yonghong Song <yhs@xxxxxxxx> wrote:
On 5/9/23 11:08 AM, Dave Thaler wrote:
From: Dave Thaler <dthaler@xxxxxxxxxxxxx>
Update the documentation regarding shift operations to explain the use
of a mask, since otherwise shifting by a value out of range (like
negative) is undefined.
Signed-off-by: Dave Thaler <dthaler@xxxxxxxxxxxxx>
LGTM with a few nit below.
Acked-by: Yonghong Song <yhs@xxxxxx>
[...]
-BPF_ARSH 0xc0 sign extending shift right
+BPF_ARSH 0xc0 sign extending dst >>= (src & mask)
dst s>>= (src & mask)
?
I had thought about that, but based on Jose's LSF/MM/BPF
presentation yesterday there are multiple such syntaxes.
">>=" vs "s>>=" is only one of several. There's ">>" vs ">>>",
there's assembly-like, etc. So I thought that it would take
more text to define "s>>" as meaning signing extending right
shift, than just saying sign extending ">>=" here. And I didn't
want to just assume the reader knows what "s>>" means
without defining it since neither the C standard nor gcc use
"s>>".
gcc will implement clang asm syntax as well. So for the consistency
of verifier log, bpftool xlated dump, and llvm-objdump result.
I think using "s>>=" syntax is the best.
The following table is the alu opcode in kernel/bpf/disasm.c
(used by both kernel verifier and bpftool xlated dump):
const char *const bpf_alu_string[16] = {
[BPF_ADD >> 4] = "+=",
[BPF_SUB >> 4] = "-=",
[BPF_MUL >> 4] = "*=",
[BPF_DIV >> 4] = "/=",
[BPF_OR >> 4] = "|=",
[BPF_AND >> 4] = "&=",
[BPF_LSH >> 4] = "<<=",
[BPF_RSH >> 4] = ">>=",
[BPF_NEG >> 4] = "neg",
[BPF_MOD >> 4] = "%=",
[BPF_XOR >> 4] = "^=",
[BPF_MOV >> 4] = "=",
[BPF_ARSH >> 4] = "s>>=",
[BPF_END >> 4] = "endian",
};
Also, in Documentation/bpf/instruction-set.rst:
======== ===== ==========================================================
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
BPF_RSH 0x70 dst >>= src
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 shift right
BPF_END 0xd0 byte swap operations (see `Byte swap instructions`_ below)
======== ===== ==========================================================
In the above, the BPF_NEG is specified as
'dst = ~src`, which is not correct, it should be
'dst = -dst'.
See kernel/bpf/core.c:
ALU_NEG:
DST = (u32) -DST;
CONT;
ALU64_NEG:
DST = -DST;
CONT;
Could you help fix it?
Thanks for the Ack.
Dave
