> -----Original Message----- > From: Yonghong Song <yonghong.song@xxxxxxxxx> > Sent: Friday, January 26, 2024 7:41 PM > To: dthaler1968@xxxxxxxxxxxxxx > Cc: bpf@xxxxxxxx; bpf@xxxxxxxxxxxxxxx > Subject: Re: 64-bit immediate instructions clarification > > > On 1/26/24 2:27 PM, dthaler1968@xxxxxxxxxxxxxx wrote: > > Yonghong Song <yonghong.song@xxxxxxxxx> wrote: > >> On 1/25/24 5:12 PM, dthaler1968@xxxxxxxxxxxxxx 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@xxxxxxxxx>
