On 04/06/2019 03:56 AM, Alexei Starovoitov wrote: > On Fri, Apr 05, 2019 at 10:59:27PM +0200, Daniel Borkmann wrote: >> >> -/* when bpf_ldimm64->src_reg == BPF_PSEUDO_MAP_FD, bpf_ldimm64->imm == fd */ >> +/* When BPF ldimm64's insn[0].src_reg != 0 then this can have >> + * two extensions: >> + * >> + * insn[0].src_reg: BPF_PSEUDO_MAP_FD BPF_PSEUDO_MAP_VALUE >> + * insn[0].imm: map fd map fd >> + * insn[1].imm: 0 offset into value >> + * insn[0].off: 0 lower 16 bit of map index >> + * insn[1].off: 0 higher 16 bit of map index >> + * ldimm64 rewrite: address of map address of map[index]+offset >> + * verifier type: CONST_PTR_TO_MAP PTR_TO_MAP_VALUE > ... >> + else if (insn->src_reg == BPF_PSEUDO_MAP_VALUE) >> + snprintf(dd->scratch_buff, sizeof(dd->scratch_buff), >> + "map[id:%u][%u]+%u", insn->imm, >> + ((__u32)(__u16)insn[0].off) | >> + ((__u32)(__u16)insn[1].off) << 16, >> + (insn + 1)->imm); > > Hopefully one last nit... > Do we really need to allow this odd split index support? > Later patches enforce array of 1 element and libbpf only uses that. > This index feature feels too quirky and not really useful at this moment. > Can we enforce that insn[0|1].off == 0 instead ? > Later we can extend it to mean index without breaking anything. I originally didn't have it in v2 of the series, but I ended up implementing it after feedback from Andrii back then complaining that it's too specific and not generic enough. I agreed with him that the limitation of max_elems = 1 wasn't too nice, so I went to implement that full 32 bit index can be used thus that it has the potential of efficient map lookup replacement for array maps in general which is quite nice since within single insn it allows to select index and offset into value all as simple 64 bit imm load. Thanks, Daniel
