Yonghong Song <yhs@xxxxxxxx> wrote:
> 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.
Just posting to the list what we discussed in person today.
I will do this in a subsequent submission since that comment also affects
the comparison operators, so treating it as separate from this patch.
> 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?
Yes I can put it into the same patch as the other change
discussed above.
Would like to see the current one merged so I can base
the next one on top of this one.
Thanks,
Dave
