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

[Yonghong Song <yonghong.song@linux.dev>]

On 1/30/24 8:39 AM, dthaler1968@googlemail.com wrote:
>> Alexei Starovoitov <alexei.starovoitov@gmail.com> 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.
>
> * (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.
>
> * (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.
>
> * (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.
>
> * 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.

clang never generated this insn either.

>     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.

It is just my opinion. Standardization is complicated. I guess adding it to the legacy insn
is okay to prevent anybody using the same opcode.

>
> * 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.
>
> Let me know if any of the above is incorrect and I can submit a doc patch.
>
> Dave
>