Hi Dave, there is a lot of good thing in here, but it is a bit hard to review, mostly because it is a giant patch instead of a single well-documented patch per logical thing to change. A bunch of nitpicks below, mostly style or organizational: > +The current Instruction Set Architecture (ISA) version, sometimes referred to in other documents > +as a "CPU" version, is 3. This document also covers older versions of the ISA. Hmm, I thought the versioning was a bit more complicated based on the mailing list interactions and the call. Especially with things like the full atomics not even supported by all gits. > + **Note** > + > + *Linux implementation*: In the Linux kernel, the exit value for eBPF > + programs is passed as a 32 bit value. Is this Linux, a specific program type, or the ISA? > + *Linux implementation*: In the Linux kernel, all program types only use > + R1 which contains the "context", which is typically a structure containing all > + the inputs needed. I also think these Linux notes do not belong into the main instruction set document, which tries to really just describe the ISA. > - * the basic instruction encoding, which uses 64 bits to encode an instruction > - * the wide instruction encoding, which appends a second 64-bit immediate value > - (imm64) after the basic instruction for a total of 128 bits. > +* the basic instruction encoding, which uses 64 bits to encode an instruction > +* the wide instruction encoding, which appends a second 64-bit immediate (i.e., Btw, can you explain why you de-indent these? I picked the space before the * because that seems to be what most Linux RST documents do. > + For ISA versions prior to 3, Clang v7.0 and later can enable ``BPF_ALU`` support with > + ``-Xclang -target-feature -Xclang +alu32``. I also suspect the clang notes would be better off in a separate document from the main ISA. > -BPF_XOR | BPF_K | BPF_ALU means:: > + *Linux implementation*: In the Linux kernel, uint32_t is expressed as u32, > + uint64_t is expressed as u64, etc. This document uses the standard C terminology > + as the cross-platform specification. I don't think this makes sense in the document. Instead we probably need a "Conventions" section that defines the type and syntax we use.
