RE: 64-bit immediate instructions clarification

[dthaler1968@googlemail.com]

> -----Original Message-----
> From: Yonghong Song <yonghong.song@linux.dev>
> Sent: Friday, January 26, 2024 7:41 PM
> To: dthaler1968@googlemail.com
> Cc: bpf@ietf.org; bpf@vger.kernel.org
> Subject: Re: 64-bit immediate instructions clarification
> 
> 
> On 1/26/24 2:27 PM, dthaler1968@googlemail.com wrote:
> > Yonghong Song <yonghong.song@linux.dev> wrote:
> >> On 1/25/24 5:12 PM, dthaler1968@googlemail.com wrote:
> >>> The spec defines:
> >>>> As discussed below in `64-bit immediate instructions`_, a 64-bit
> >>>> immediate instruction uses a 64-bit immediate value that is
> >>>> constructed as
> >> follows.
> >>>> The 64 bits following the basic instruction contain a pseudo
> >>>> instruction using the same format but with opcode, dst_reg,
> >>>> src_reg, and offset all set to zero, and imm containing the high 32
> >>>> bits of the
> >> immediate value.
> >>> [...]
> >>>> imm64 = (next_imm << 32) | imm
> >>> The 64-bit immediate instructions section then says:
> >>>> Instructions with the ``BPF_IMM`` 'mode' modifier use the wide
> >>>> instruction encoding defined in `Instruction encoding`_, and use
> >>>> the 'src' field of the basic instruction to hold an opcode subtype.
> >>> Some instructions then nicely state how to use the full 64 bit
> >>> immediate value, such as
> >>>> BPF_IMM | BPF_DW | BPF_LD  0x18    0x0  dst = imm64
> >> integer      integer
> >>>> BPF_IMM | BPF_DW | BPF_LD  0x18    0x2  dst =
> map_val(map_by_fd(imm))
> >> + next_imm   map fd       data pointer
> >>>> BPF_IMM | BPF_DW | BPF_LD  0x18    0x6  dst =
> map_val(map_by_idx(imm))
> >> + next_imm  map index    data pointer
> >>> Others don't:
> >>>> BPF_IMM | BPF_DW | BPF_LD  0x18    0x1  dst = map_by_fd(imm)
> >> map fd       map
> >>>> BPF_IMM | BPF_DW | BPF_LD  0x18    0x3  dst = var_addr(imm)
> >> variable id  data pointer
> >>>> BPF_IMM | BPF_DW | BPF_LD  0x18    0x4  dst = code_addr(imm)
> >> integer      code pointer
> >>>> BPF_IMM | BPF_DW | BPF_LD  0x18    0x5  dst = map_by_idx(imm)
> >> map index    map
> >>> How is next_imm used in those four?  Must it be 0?  Or can it be
> >>> anything and
> >> it's ignored?
> >>> Or is it used for something?
> >> The other four must have next_imm to be 0. No use of next_imm in thee
> >> four insns kindly implies this.
> >> See uapi bpf.h for details (search BPF_PSEUDO_MAP_FD).
> > Thanks for confirming.  The "Instruction encoding" section has
> > misleading text in my opinion.
> >
> > It nicely says:
> >> Note that most instructions do not use all of the fields. Unused fields shall
> be cleared to zero.
> > But then goes on to say:
> >> As discussed below in 64-bit immediate instructions (Section 4.4), a
> >> 64-bit immediate instruction uses a 64-bit immediate value that is
> constructed as follows.
> > [...]
> >> imm64 = (next_imm << 32) | imm
> > Under a normal English reading, that could imply that all 64-bit
> > immediate instructions use imm64, which is not the case.  The whole imm64
> discussion there only applies today to src=0 (though I
> > suppose it could be used by future 64-bit immediate instructions).   Minimally
> I think
> > "a 64-bit immediate instruction uses" should be "some 64-bit immediate
> instructions use"
> > but at present there's only one.
> >
> > It would actually be simpler to remove the imm64 text and just have
> > the definition of src 0x0 change from: "dst = imm64" to "dst = (next_imm <<
> 32) | imm".
> >
> > What do you think?
> 
> it does sound better. Something like below?
> 
> diff --git a/Documentation/bpf/standardization/instruction-set.rst
> b/Documentation/bpf/standardization/instruction-set.rst
> index af43227b6ee4..fceacca46299 100644
> --- a/Documentation/bpf/standardization/instruction-set.rst
> +++ b/Documentation/bpf/standardization/instruction-set.rst
> @@ -166,7 +166,7 @@ Note that most instructions do not use all of the fields.
>   Unused fields shall be cleared to zero.
> 
>   As discussed below in `64-bit immediate instructions`_, a 64-bit immediate -
> instruction uses a 64-bit immediate value that is constructed as follows.
> +instruction uses two 32-bit immediate values that are constructed as follows.
>   The 64 bits following the basic instruction contain a pseudo instruction
>   using the same format but with opcode, dst_reg, src_reg, and offset all set to
> zero,
>   and imm containing the high 32 bits of the immediate value.
> @@ -181,13 +181,8 @@ This is depicted in the following figure::
>                                      '--------------'
>                                     pseudo instruction
> 
> -Thus the 64-bit immediate value is constructed as follows:
> -
> -  imm64 = (next_imm << 32) | imm
> -
> -where 'next_imm' refers to the imm value of the pseudo instruction -following
> the basic instruction.  The unused bytes in the pseudo -instruction are reserved
> and shall be cleared to zero.
> +Here, the imm value of the pseudo instruction is called 'next_imm'. The
> +unused bytes in the pseudo instruction are reserved and shall be cleared to
> zero.
> 
>   Instruction classes
>   -------------------
> @@ -590,7 +585,7 @@ defined further below:
>   =========================  ======  ===
> =========================================  ===========
> ==============
>   opcode construction        opcode  src  pseudocode                                 imm type
> dst type
>   =========================  ======  ===
> =========================================  ===========
> ==============
> -BPF_IMM | BPF_DW | BPF_LD  0x18    0x0  dst = imm64
> integer      integer
> +BPF_IMM | BPF_DW | BPF_LD  0x18    0x0  dst = (next_imm << 32) | imm
> integer      integer
>   BPF_IMM | BPF_DW | BPF_LD  0x18    0x1  dst = map_by_fd(imm)
> map fd       map
>   BPF_IMM | BPF_DW | BPF_LD  0x18    0x2  dst = map_val(map_by_fd(imm)) +
> next_imm   map fd       data pointer
>   BPF_IMM | BPF_DW | BPF_LD  0x18    0x3  dst = var_addr(imm)
> variable id  data pointer

Acked-by: Dave Thaler <dthaler1968@gmail.com>