Re: [Bpf] ISA: BPF_MSH and deprecated packet access instructions

[Alexei Starovoitov <[e-mail]>]

On Tue, Jan 30, 2024 at 8:39 AM <[e-mail]> wrote:
>
> > Alexei Starovoitov <[e-mail]> wrote:
> > [...]
> > > > Although the Linux verifier doesn't support them, the fact that gcc
> > > > does support them tells me that it's probably safest to list the DW
> > > > and LDX variants as deprecated as well, which is what the draft
> > > > already did in the appendix so that's good (nothing to change there,
> > > > I think).
> > >
> > > DW never existed in classic bpf, so abs/ind never had DW flavor.
> > > If some assembler/compiler decided to "support" them it's on them.
> > > The standard must not list such things as deprecated. They never
> > > existed. So nothing is deprecated.
> >
> > Ack, I will remove the ABS/IND + DW lines from the appendix.
> >
> > > Same with MSH. BPF_LDX | BPF_MSH | BPF_B is the only insn ever existed.
> > > It's a legacy insn. Just like abs/ind.
> >
> > Should it be listed in the legacy conformance group then?
> >
> > Currently it's not mentioned in instruction-set.rst at all, so the opcode is
> > available to use by any new instruction.  If we do list it in instruction-set.rst
> > then, like abs/ind, it will be avoided by anyone proposing new instructions.
>
> Here's my understanding of this thread so far:
>
> * (IND/ABS) | (W/H/B) | LD : these are accepted by the Linux verifier and are supported
>    by clang and gcc.  They should be in the legacy conformance group of deprecated
>    instructions.

yes

> * (IND/ABS) | DW | (LD/LDX) : these are not accepted by the Linux verifier and were
>    never used.  Clang doesn't generate them but gcc did which is now removed
>    based on this discussion.  They should NOT be in the legacy conformance group of
>    deprecated instructions because they were never defined in the first place, and
>    instruction-set.rst should be updated to clarify this.

yes

> * (IND/ABS) | (W/H/B) | LDX : these are not accepted by the Linux verifier and were
>    never used.  Clang doesn't generate them but gcc does. They should NOT
>    be in the legacy conformance group of deprecated instructions because they were
>    never defined in the first place, and instruction-set.rst should be updated to clarify this.

yes.

> * (IND/ABS) | (W/H/B/DW) | (ST/STX): these are not accepted by the Linux verifier and were
>    never used.  I don't know whether clang or gcc generates them.  They should NOT
>    be in the legacy conformance group of deprecated instructions because they were
>    never defined in the first place, and instruction-set.rst should be updated to clarify this.

yes

> * MSH | B | LDX: this existed in classic BPF but does not exist in (e)BPF since it is not accepted
>    by the Linux verifier.  I don't know whether clang ever generated them, but gcc never did.
>    The "Legacy BPF Packet access instructions" section of instruction-set.rst says
>    > BPF previously introduced special instructions for access to packet data that were carried
>    > over from classic BPF. However, these instructions are deprecated and should no longer be used.
>    I read Alexei's comment "It's a legacy insn. Just like abs/ind" as a possible argument that MSH|B|LDX
>    should be mentioned in instruction-set.rst, pointing to the above section, like IND/ABS do.
>    But Yonghong argued that it was never accepted by the verifier, so need not be mentioned.

Yonghong is actually more correct here.

MSH | B | LDX is only accepted by _classic_ BPF.

It was never accepted by eBPF verifier,
so I have to back track my earlier suggestion.
I think it's undefined opcode from 'eBPF standardization' pov.
The standard doesn't talk about 'classic BPF' at all.
So it's fine to use MSH | B | LDX for something in the future.

> * MSH | (W/H/DW) | (LD/ST/STX): These are not accepted by the Linux verifier and were
>    never used.  They should NOT be in the legacy conformance group of deprecated instructions
>    because they were never defined in the first place.

yes.