On Thu, Dec 14, 2023 at 9:29 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > > We need the concept in the spec just to allow future extensability. Completely agree that the concept of the groups is necessary. I'm arguing that what was proposed: 1. "basic": all instructions not covered by another group below. 2. "atomic": all Atomic operations. 3. "divide": all division and modulo operations. 4. "legacy": all legacy packet access instructions (deprecated). 5. "map": 64-bit immediate instructions that deal with map fds or map indices. 6. "code": 64-bit immediate instruction that has a "code pointer" type. 7. "func": program-local functions. logically makes sense, but might not work for HW (based on the history of nfp offload). imo "basic" and "legacy" won't work either. So it's a lesser evil. Anyway, let's look at: | BPF_CALL | 0x8 | 0x0 | call helper | see Helper | | | | | function by address | functions | | | | | | (Section 3.3.1) | +----------+-------+-----+---------------------+-------------------+ | BPF_CALL | 0x8 | 0x1 | call PC += imm | see Program-local | | | | | | functions | | | | | | (Section 3.3.2) | +----------+-------+-----+---------------------+-------------------+ | BPF_CALL | 0x8 | 0x2 | call helper | see Helper | | | | | function by BTF ID | functions | | | | | | (Section 3.3. Having separate category 7 for single insn BPF_CALL 0x8 0x1 while keeping 0x8 0x0 and 0x8 0x2 in "basic" seems just as logical as having atomic_add insn in "basic" instead of "atomic". Then we have several kinds of ld_imm64. Sounds like the idea is to split 0x18 0x4 into "code" and the rest into "map" group? Is it logical or not? Maybe we should do risc-v like group instead? Just these 4: - Base Integer Instruction Set, 32-bit - Base Integer Instruction Set, 64-bit - Integer Multiplication and Division - Atomic Instructions And that's it. The rest of risc-v groups have no equivalent in bpf isa.
