Date:   Mon, 6 May 2019 21:40:38 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jiong Wang <jiong.wang@...ronome.com>
Cc:     daniel@...earbox.net, bpf@...r.kernel.org, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH v6 bpf-next 04/17] bpf: introduce new alu insn BPF_ZEXT
 for explicit zero extension

On Tue, May 07, 2019 at 05:29:09AM +0100, Jiong Wang wrote:
> 
> Jiong Wang writes:
> 
> > Alexei Starovoitov writes:
> >
> >> On Fri, May 03, 2019 at 11:42:31AM +0100, Jiong Wang wrote:
> >>> This patch introduce new alu32 insn BPF_ZEXT, and allocate the unused
> >>> opcode 0xe0 to it.
> >>> 
> >>> Compared with the other alu32 insns, zero extension on low 32-bit is the
> >>> only semantics for this instruction. It also allows various JIT back-ends
> >>> to do optimal zero extension code-gen.
> >>> 
> >>> BPF_ZEXT is supposed to be encoded with BPF_ALU only, and is supposed to be
> >>> generated by the latter 32-bit optimization code inside verifier for those
> >>> arches that do not support hardware implicit zero extension only.
> >>> 
> >>> It is not supposed to be used in user's program directly at the moment.
> >>> Therefore, no need to recognize it inside generic verification code. It
> >>> just need to be supported for execution on interpreter or related JIT
> >>> back-ends.
> >>
> >> uapi and the doc define it, but "it is not supposed to be used" ?!
> >>
> >>> Signed-off-by: Jiong Wang <jiong.wang@...ronome.com>
> >>> ---
> >>>  Documentation/networking/filter.txt | 10 ++++++++++
> >>>  include/uapi/linux/bpf.h            |  3 +++
> >>>  kernel/bpf/core.c                   |  4 ++++
> >>>  tools/include/uapi/linux/bpf.h      |  3 +++
> >>>  4 files changed, 20 insertions(+)
> >>> 
> >>> diff --git a/Documentation/networking/filter.txt b/Documentation/networking/filter.txt
> >>> index 319e5e0..1cb3e42 100644
> >>> --- a/Documentation/networking/filter.txt
> >>> +++ b/Documentation/networking/filter.txt
> >>> @@ -903,6 +903,16 @@ If BPF_CLASS(code) == BPF_ALU or BPF_ALU64 [ in eBPF ], BPF_OP(code) is one of:
> >>>    BPF_MOV   0xb0  /* eBPF only: mov reg to reg */
> >>>    BPF_ARSH  0xc0  /* eBPF only: sign extending shift right */
> >>>    BPF_END   0xd0  /* eBPF only: endianness conversion */
> >>> +  BPF_ZEXT  0xe0  /* eBPF BPF_ALU only: zero-extends low 32-bit */
> >>> +
> >>> +Compared with BPF_ALU | BPF_MOV which zero-extends low 32-bit implicitly,
> >>> +BPF_ALU | BPF_ZEXT zero-extends low 32-bit explicitly. Such zero extension is
> >>
> >> wait. that's an excellent observation. alu|mov is exactly it.
> >> we do not need another insn.
> >> we probably can teach the verifier to recognize <<32, >>32 and replace
> >> with mov32
> >
> > Hmm, I am silly, in v6, patched insn will be conservatively marked as
> > always needing zext, so looks like no problem to just insert mov32 as
> > zext. But some backends needs minor opt, because this will be special mov,
> > with the same src and dst, just need to clear high 32-bit, no need of
> > mov.
> 
> I take it back.
> 
> Recalled the reason why new ZEXT was introduced. It was because instruction
> insertion based approach doesn't push analysis results down to JIT
> back-ends, instead, it removes the clear-high-32bit semantics from
> all sub-register write instructions, then insert new explicit ZEXT insn,
> either by 64bit shifts combination or this new introduced ZEXT, what's
> important, the inserted "ZEXT" should not be affected by
> "env->verifier_zext", and be performed unconditionally.
> 
> That is to say, for the current zero extension insertion based approach,
> JIT back-ends trust verifier has done full zero extension insertion and
> rewritten the instruction sequence once the flag env->verifier_zext is set,
> and then JIT back-end do NOT clear high 32-bit for all existing
> sub-register write instructions, for example ALU32 and narrowed load, they
> rely on those new inserted "unconditional ZEXT" to do the job if it is
> needed. So, if we insert mov32, there actually won't be zero extension
> insertion performed for it.
> 
> The inserted "ZEXT" needs to have zero extension semantics that is not
> affected by env->verifier_zext. BPF_ZEXT was introduced because of this,
> low32 zext is its only semantics and should be unconditionally done.
> 
> "mov32" could be used as "ZEXT" only when there is no such removal of zero
> extension semantics from alu32, or if JIT back-end could have instruction
> level information, for example the analyzed instruction level zero extension
> information pushed down to JIT back-ends. The new inserted "mov32" would
> then has information like "zext_dst" be true, JIT back-end then will
> generate zero extension for it.

JITs could simply always do zext for mov32. No need to for extra flags.

Powered by blists - more mailing lists