On Tue, Nov 2, 2021 at 2:26 PM Mauricio Vásquez Bernal
<mauricio@xxxxxxxxxx> wrote:
>
> > Part #2 absolutely doesn't belong in libbpf. Libbpf exposes enough BTF
> > constructing APIs to implement this in any application, bpftool or
> > otherwise. It's also a relatively straightforward problem: mark used
> > types and fields, create a copy of BTF with only those types and
> > fields.
>
> Totally agree.
>
> > The last point is important, because to solve the problem 1b (exposing
> > CO-RE relo info), the best way to minimize public API commitments is
> > to (optionally, probably) request libbpf to record its CO-RE relo
> > decisions. Here's what I propose, specifically:
> > 1. Add something like "bool record_core_relo_info" (awful name,
> > don't use it) in bpf_object_open_opts.
> > 2. If it is set to true, libbpf will keep a "log" of CO-RE
> > relocation decisions, recording stuff like program name, instruction
> > index, local spec (i.e., root_type_id, spec string, relo kind, maybe
> > something else), target spec (kernel type_id, kernel spec string, also
> > module ID, if it's not vmlinux BTF). We can also record relocated
> > value (i.e., field offset, actual enum value, true/false for
> > existence, etc). All these are stable concepts, so I'd feel more
> > comfortable exposing them, compared to stuff like bpf_core_accessor
> > and other internal details.
> > 3. The memory for all that will be managed by libbpf for simplicity
> > of an API, and we'll expose accessors to get those arrays (at object
> > level or per-program level is TBD).
> > 4. This info will be available after the prepare() step and will be
> > discarded either at create_maps() or load().
>
> I like all this proposal. It fits very well with the BTFGen use case.
>
> Regarding the information to expose, IIUC that'd be slight versions of
> struct bpf_core_relo_res and struct bpf_core_spec. I think we could
> expose the following structures and a function to get it (please
> ignore the naming for now):
>
> ```
> /* reduced version of struct bpf_core_spec */
> struct bpf_core_spec_pub {
> const struct btf *btf;
> __u32 root_type_id;
> enum bpf_core_relo_kind kind;
> /* raw, low-level spec: 1-to-1 with accessor spec string */ --> we can
> also use access_str_off and let the user parse it
> int raw_spec[BPF_CORE_SPEC_MAX_LEN];
string might be a more "extensible" way, but we'll need to construct
that string for each relocation
> /* raw spec length */
> int raw_len;
using string would eliminate the need for this
> };
>
> struct bpf_core_relo_pub {
> const char *prog_name; --> if we expose it by program then it's not needed.
yep, not sure about per-program yet, but that's minor
> int insn_idx;
>
> bool poison; --> allows the user to understand if the relocation
> succeeded or not.
>
> /* new field offset for field based core relos */
> __u32 new_offset;
>
> // TODO: fields for type and enum-based relos
isn't it always just u64 new_value for all types of relos? We can also
expose old_value just for completeness
>
> struct bpf_core_spec_pub local_spec, targ_spec; --> BTFGen only needs
> targ_spec, I suppose local spec would be useful for other use cases.
targ_spec doesn't seem necessary given we have root_type_id, relo
kind, access_string (or raw_spec). What am I missing?
> };
>
> LIBBPF_API struct bpf_core_relo_pub *bpf_program__core_relos(struct
> bpf_program *prog);
need also size of this array and it should be const struct *, but
yeah, something like this
> ```
>
> I don't have strong opinions about exposing it by object or by
> program. Both cases should work the same for BTFGen.
>
> Does it make sense to you?
Yeah, more or less.
>
> Btw, I'm probably not the right person to give opinions about this API
> splitment. I'd be happy to have other opinions here and to make this
> change once we agree on a path forward.
