On Fri, Nov 19, 2021 at 09:25:03AM -0800, Andrii Nakryiko wrote: > On Tue, Nov 16, 2021 at 8:42 AM Mauricio Vásquez <mauricio@xxxxxxxxxx> wrote: > > > > The result of the CO-RE relocations can be useful for some use cases > > like BTFGen[0]. This commit adds a new ‘record_core_relos’ option to > > save the result of such relocations and a couple of functions to access > > them. > > > > [0]: https://github.com/kinvolk/btfgen/ > > > > Signed-off-by: Mauricio Vásquez <mauricio@xxxxxxxxxx> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@xxxxxxxxxxx> > > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@xxxxxxxxxx> > > Signed-off-by: Leonardo Di Donato <leonardo.didonato@xxxxxxxxxx> > > --- > > tools/lib/bpf/libbpf.c | 63 ++++++++++++++++++++++++++++++++++++++- > > tools/lib/bpf/libbpf.h | 49 +++++++++++++++++++++++++++++- > > tools/lib/bpf/libbpf.map | 2 ++ > > tools/lib/bpf/relo_core.c | 28 +++++++++++++++-- > > tools/lib/bpf/relo_core.h | 21 ++----------- > > 5 files changed, 140 insertions(+), 23 deletions(-) > > > > Ok, I've meditated on this patch set long enough. I still don't like > that libbpf will be doing all this just for the sake of BTFGen's use > case. > > In the end, I think giving bpftool access to internal APIs of libbpf > is more appropriate, and it seems like it's pretty easy to achieve. It > might actually clean up gen_loader parts a bit as well. So we'll need > to coordinate all this with Alexei's current work on CO-RE for kernel > as well. > > But here's what I think could be done to keep libbpf internals simple. > We split bpf_core_apply_relo() into two parts: 1) calculating the > struct bpf_core_relo_res and 2) patching the instruction. If you look > at bpf_core_apply_relo, it needs prog just for prog_name (which we can > just pass everywhere for logging purposes) and to extract one specific > instruction to be patched. This instruction is passed at the very end > to bpf_core_patch_insn() after bpf_core_relo_res is calculated. So I > propose to make those two explicitly separate steps done by libbpf. So > bpf_core_apply_relo() (which we should rename to bpf_core_calc_relo() > or something like that) won't do any modification to the program > instructions. bpf_object__relocate_core() will do bpf_core_calc_relo() > first, if that's successful, it will pass the result into > bpf_core_patch_insn(). Simple and clean, unless I missed some > complication (happens all the time, but..) I was thinking about such split as well, but for a different reason :) Since we've discussed future kernel flag 'check what libbpf had done' the idea is to use bpf_core_relo_res after first step and let kernel look at insn to see whether libbpf relocated the insn the same way as kernel is going to do. Also I was thinking to pass struct bpf_core_spec [3] and struct bpf_core_relo_res [2] as two arrays into bpf_core_calc_relo() to reduce stack size, since reduction of BPF_CORE_SPEC_MAX_LEN to 32 is not enough when all kconfig debugs are on on some architectures. I was planning to work on that as a follow up to my set. In the light of BTFgen I was thinking whether bpf_core_relo_res should be part of uapi returned by the kernel, but that is probably overkill.
