On 25/06/17 05:57PM, Eduard Zingerman wrote:
> On Sun, 2025-06-15 at 08:59 +0000, Anton Protopopov wrote:
>
> Meta: "instruction set" is a super confusing name, at-least for me the
> first thought is about actual set of instructions supported by
> some h/w. instruction_info? instruction_offset? just
> "iset"/"ioffset"?
>
> [...]
>
> > On map creation/initialization, before loading the program, each
> > element of the map should be initialized to point to an instruction
> > offset within the program. Before the program load such maps should
> > be made frozen. After the program verification xlated and jitted
> > offsets can be read via the bpf(2) syscall.
>
> I think such maps would be a bit more ergonomic it original
> instruction index would be saved as well, e.g:
>
> (original_offset, xlated_offset, jitted_offset)
>
> Otherwise user would have to recover original offset from some
> external mapping. This information is stored in orig_xlated_off
> anyway.
I do agree that this might be convenient to have the original_offset.
But the only use case I see here is "BPF debuggers". Such programs
will be able to build this mapping themselves.
I would add it as is, the only obstacle I see now is map key size.
Now from BPF point of view and from userspace point of view it is 8.
Userspace sees (u32 xlated, u32 jitted), and BPF sees *ip. I haven't
looked at how much work it is to have different key sizes for
userspace and BPF, and if this breaks things too much.
> [...]
>
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index 15672cb926fc..923c38f212dc 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -1615,6 +1615,8 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image
>
> [...]
>
> > @@ -2642,6 +2645,14 @@ st: if (is_imm8(insn->off))
> > return -EFAULT;
> > }
> > memcpy(rw_image + proglen, temp, ilen);
> > +
> > + /*
> > + * Instruction sets need to know how xlated code
> > + * maps to jited code
> > + */
> > + abs_xlated_off = bpf_prog->aux->subprog_start + i - 1 - adjust_off;
>
> Nit: `adjust_off` is a bit hard to follow, maybe move the following:
>
> abs_xlated_off = bpf_prog->aux->subprog_start + i - 1;
>
> to the beginning of the loop?
Thank, this isn't transparent indeed. I will move things to be more
readable.
>
> > + bpf_prog_update_insn_ptr(bpf_prog, abs_xlated_off, proglen, ilen,
> > + jmp_offset, image + proglen);
>
> Nit: initialize `jmp_offset` at each loop iteration to 0?
> otherwise it would denote jump offset of the last processed
> jump instruction for all following non-jump instructions.
Yes, thanks.
> > }
> > proglen += ilen;
> > addrs[i] = proglen;
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 8189f49e43d6..008bcd44c60e 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -3596,4 +3596,25 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
> > return prog->aux->func_idx != 0;
> > }
> >
> > +int bpf_insn_set_init(struct bpf_map *map, const struct bpf_prog *prog);
> > +int bpf_insn_set_ready(struct bpf_map *map);
> > +void bpf_insn_set_release(struct bpf_map *map);
> > +void bpf_insn_set_adjust(struct bpf_map *map, u32 off, u32 len);
> > +void bpf_insn_set_adjust_after_remove(struct bpf_map *map, u32 off, u32 len);
> > +
> > +struct bpf_insn_ptr {
>
> Could you please add comments describing each field?
> E.g.: "address of the instruction in the jitted image",
> "for jump instructions, the relative offset of the jump target",
> "index of the original instruction",
> "original value of the corresponding bpf_insn_set_value.xlated_off".
Sure, will add.
(Also, not to repeat "yes" many times, all your comments below look
to make sense, will address them. Thanks.)
> > + void *jitted_ip;
> > + u32 jitted_len;
> > + int jitted_jump_offset;
> > + struct bpf_insn_set_value user_value; /* userspace-visible value */
> > + u32 orig_xlated_off;
> > +};
>
> [...]
>
> > diff --git a/kernel/bpf/bpf_insn_set.c b/kernel/bpf/bpf_insn_set.c
> > new file mode 100644
>
> [...]
>
> > +static int insn_set_check_btf(const struct bpf_map *map,
> > + const struct btf *btf,
> > + const struct btf_type *key_type,
> > + const struct btf_type *value_type)
> > +{
> > + u32 int_data;
> > +
> > + if (BTF_INFO_KIND(key_type->info) != BTF_KIND_INT)
> > + return -EINVAL;
> > +
> > + if (BTF_INFO_KIND(value_type->info) != BTF_KIND_INT)
> > + return -EINVAL;
> > +
> > + int_data = *(u32 *)(key_type + 1);
>
> Nit: use btf_type_int() accessor?
>
> > + if (BTF_INT_BITS(int_data) != 32 || BTF_INT_OFFSET(int_data))
> > + return -EINVAL;
> > +
> > + int_data = *(u32 *)(value_type + 1);
> > + if (BTF_INT_BITS(int_data) != 32 || BTF_INT_OFFSET(int_data))
>
> Should this check for `BTF_INT_BITS(int_data) != 64`?
>
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
>
> [...]
>
> > +int bpf_insn_set_init(struct bpf_map *map, const struct bpf_prog *prog)
> > +{
> > + struct bpf_insn_set *insn_set = cast_insn_set(map);
> > + int i;
> > +
> > + if (!is_frozen(map))
> > + return -EINVAL;
> > +
> > + if (!valid_offsets(insn_set, prog))
> > + return -EINVAL;
> > +
> > + /*
> > + * There can be only one program using the map
> > + */
> > + mutex_lock(&insn_set->state_mutex);
> > + if (insn_set->state != INSN_SET_STATE_FREE) {
> > + mutex_unlock(&insn_set->state_mutex);
> > + return -EBUSY;
> > + }
> > + insn_set->state = INSN_SET_STATE_INIT;
> > + mutex_unlock(&insn_set->state_mutex);
> > +
> > + /*
> > + * Reset all the map indexes to the original values. This is needed,
> > + * e.g., when a replay of verification with different log level should
> > + * be performed.
> > + */
> > + for (i = 0; i < map->max_entries; i++)
> > + insn_set->ptrs[i].user_value.xlated_off = insn_set->ptrs[i].orig_xlated_off;
> > +
> > + return 0;
> > +}
> > +
> > +int bpf_insn_set_ready(struct bpf_map *map)
>
> What is the reasoning for not needing to take the mutex here and in
> the bpf_insn_set_release?
>
> > +{
> > + struct bpf_insn_set *insn_set = cast_insn_set(map);
> > + int i;
> > +
> > + for (i = 0; i < map->max_entries; i++) {
> > + if (insn_set->ptrs[i].user_value.xlated_off == INSN_DELETED)
> > + continue;
> > + if (!insn_set->ips[i])
> > + return -EFAULT;
> > + }
> > +
> > + insn_set->state = INSN_SET_STATE_READY;
> > + return 0;
> > +}
> > +
> > +void bpf_insn_set_release(struct bpf_map *map)
> > +{
> > + struct bpf_insn_set *insn_set = cast_insn_set(map);
> > +
> > + insn_set->state = INSN_SET_STATE_FREE;
> > +}
>
> [...]
>
> (... I'll continue reading through patch-set a bit later ...)
