Re: FW: ebpf-docs: draft of ISA doc updates in progress

[Christoph Hellwig <hch@infradead.org>]

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.