Re: [Bpf] [PATCH bpf-next v2] bpf, docs: Explain helper functions

[David Vernet <void@manifault.com>]

On Wed, Feb 08, 2023 at 05:45:59PM +0000, Dave Thaler wrote:
> > -----Original Message-----
> > From: David Vernet <void@manifault.com>
> > Sent: Wednesday, February 8, 2023 9:40 AM
> > To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Cc: Dave Thaler <dthaler@microsoft.com>; Dave Thaler
> > <dthaler1968@googlemail.com>; bpf@vger.kernel.org; bpf@ietf.org
> > Subject: Re: [Bpf] [PATCH bpf-next v2] bpf, docs: Explain helper functions
> > 
> > On Wed, Feb 08, 2023 at 09:31:18AM -0800, Alexei Starovoitov wrote:
> > > On Wed, Feb 8, 2023 at 9:26 AM Dave Thaler
> > > <dthaler=40microsoft.com@dmarc.ietf.org> wrote:
> > > >
> > > > David Vernet wrote:
> > > > > > +Reserved instructions
> > > > > > +====================
> > > > >
> > > > > small nit: Missing a =
> > > >
> > > > Ack.
> > > >
> > > > > > +Clang will generate the reserved ``BPF_CALL | BPF_X | BPF_JMP``
> > > > > > +(0x8d)
> > > > > instruction if ``-O0`` is used.
> > > > >
> > > > > Are we calling this out here to say that BPF_CALL in clang -O0
> > > > > builds is not supported? That would seem to be the case given that
> > > > > we say that BPF_CALL
> > > > > | BPF_X | BPF_JMP in reserved and not permitted in instruction-set.rst.
> > > >
> > > > Yes, exactly.  I could update the language to add something like
> > > > "... so BPF_CALL in clang -O0 builds is not supported".
> > >
> > > That will not be a correct statement.
> > > BPF_CALL is a valid insn regardless of optimization flags.
> > > BPF_CALLX will be a valid insn when the verifier support is added.
> > > Compilers need to make a choice which insn to use on a case by case basis.
> > > When compilers have no choice, but to use call by register they will
> > > use callx. That what happens with = (void *)1 hack that we use for
> > > helpers.
> > > It can happen with -O2 just as well.
> > 
> > In that case, I suggest we update the verbiage in instruction-set.rst to
> > say:
> > 
> > Note that ``BPF_CALL | BPF_X | BPF_JMP`` (0x8d), where the helper function
> > integer would be read from a specified register, is not currently supported by
> > the verifier. Any programs with this instruction will fail to load until such
> > support is added.
> 
> The problem with that wording is that it implies that there is "the" verifier,
> whereas the point of standard documentation (since this file is also being used
> to generate the IETF spec) is to keep statements about any specific verifier
> or compiler out of instruction-set.rst.  That's why there's separate files like

Yes, good point.

> clang-notes.rst for the clang compiler, etc.   The instruction set rst is,
> in my view, should apply across all compilers, all verifiers, all runtimes, etc.
> It could potentially say certain things are optional to support, but there is
> a distinction between "defined" vs "reserved" where it currently means
> such support is "reserved" not "defined".

That makes sense. IMO we should just say that the instruction is valid
then, and not make a distinction. 'reserved' should imply that the bits
for the instruction in question have no definition whatsoever, e.g.
reserved bits in control registers in x86, etc. In this case, the
instruction is valid, we just haven't chosen to implement support for it
yet in the Linux verifier. That's par for the course for implementing
standards. Usually we don't implement something until it's needed.