On Tue, Jan 17, 2023 at 11:36 PM Christophe Leroy <christophe.leroy@xxxxxxxxxx> wrote: > > > > Le 18/01/2023 à 03:21, Alexei Starovoitov a écrit : > > On Tue, Jan 17, 2023 at 6:13 PM Tonghao Zhang <tong@xxxxxxxxxxxxx> wrote: > >> > >> > >> > >>> On Jan 17, 2023, at 11:59 PM, Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > >>> > >>> On 1/17/23 3:22 PM, Tonghao Zhang wrote: > >>>>> On Jan 17, 2023, at 3:30 PM, Christophe Leroy <christophe.leroy@xxxxxxxxxx> wrote: > >>>>> > >>>>> > >>>>> > >>>>> Le 17/01/2023 à 06:30, Tonghao Zhang a écrit : > >>>>>> > >>>>>> > >>>>>>> On Jan 9, 2023, at 4:15 PM, Christophe Leroy <christophe.leroy@xxxxxxxxxx> wrote: > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> Le 06/01/2023 à 16:37, Daniel Borkmann a écrit : > >>>>>>>> On 1/5/23 6:53 PM, Christophe Leroy wrote: > >>>>>>>>> Le 05/01/2023 à 04:06, tong@xxxxxxxxxxxxx a écrit : > >>>>>>>>>> From: Tonghao Zhang <tong@xxxxxxxxxxxxx> > >>>>>>>>>> > >>>>>>>>>> The x86_64 can't dump the valid insn in this way. A test BPF prog > >>>>>>>>>> which include subprog: > >>>>>>>>>> > >>>>>>>>>> $ llvm-objdump -d subprog.o > >>>>>>>>>> Disassembly of section .text: > >>>>>>>>>> 0000000000000000 <subprog>: > >>>>>>>>>> 0: 18 01 00 00 73 75 62 70 00 00 00 00 72 6f 67 00 r1 > >>>>>>>>>> = 29114459903653235 ll > >>>>>>>>>> 2: 7b 1a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r1 > >>>>>>>>>> 3: bf a1 00 00 00 00 00 00 r1 = r10 > >>>>>>>>>> 4: 07 01 00 00 f8 ff ff ff r1 += -8 > >>>>>>>>>> 5: b7 02 00 00 08 00 00 00 r2 = 8 > >>>>>>>>>> 6: 85 00 00 00 06 00 00 00 call 6 > >>>>>>>>>> 7: 95 00 00 00 00 00 00 00 exit > >>>>>>>>>> Disassembly of section raw_tp/sys_enter: > >>>>>>>>>> 0000000000000000 <entry>: > >>>>>>>>>> 0: 85 10 00 00 ff ff ff ff call -1 > >>>>>>>>>> 1: b7 00 00 00 00 00 00 00 r0 = 0 > >>>>>>>>>> 2: 95 00 00 00 00 00 00 00 exit > >>>>>>>>>> > >>>>>>>>>> kernel print message: > >>>>>>>>>> [ 580.775387] flen=8 proglen=51 pass=3 image=ffffffffa000c20c > >>>>>>>>>> from=kprobe-load pid=1643 > >>>>>>>>>> [ 580.777236] JIT code: 00000000: cc cc cc cc cc cc cc cc cc cc cc > >>>>>>>>>> cc cc cc cc cc > >>>>>>>>>> [ 580.779037] JIT code: 00000010: cc cc cc cc cc cc cc cc cc cc cc > >>>>>>>>>> cc cc cc cc cc > >>>>>>>>>> [ 580.780767] JIT code: 00000020: cc cc cc cc cc cc cc cc cc cc cc > >>>>>>>>>> cc cc cc cc cc > >>>>>>>>>> [ 580.782568] JIT code: 00000030: cc cc cc > >>>>>>>>>> > >>>>>>>>>> $ bpf_jit_disasm > >>>>>>>>>> 51 bytes emitted from JIT compiler (pass:3, flen:8) > >>>>>>>>>> ffffffffa000c20c + <x>: > >>>>>>>>>> 0: int3 > >>>>>>>>>> 1: int3 > >>>>>>>>>> 2: int3 > >>>>>>>>>> 3: int3 > >>>>>>>>>> 4: int3 > >>>>>>>>>> 5: int3 > >>>>>>>>>> ... > >>>>>>>>>> > >>>>>>>>>> Until bpf_jit_binary_pack_finalize is invoked, we copy rw_header to > >>>>>>>>>> header > >>>>>>>>>> and then image/insn is valid. BTW, we can use the "bpftool prog dump" > >>>>>>>>>> JITed instructions. > >>>>>>>>> > >>>>>>>>> NACK. > >>>>>>>>> > >>>>>>>>> Because the feature is buggy on x86_64, you remove it for all > >>>>>>>>> architectures ? > >>>>>>>>> > >>>>>>>>> On powerpc bpf_jit_enable == 2 works and is very usefull. > >>>>>>>>> > >>>>>>>>> Last time I tried to use bpftool on powerpc/32 it didn't work. I don't > >>>>>>>>> remember the details, I think it was an issue with endianess. Maybe it > >>>>>>>>> is fixed now, but it needs to be verified. > >>>>>>>>> > >>>>>>>>> So please, before removing a working and usefull feature, make sure > >>>>>>>>> there is an alternative available to it for all architectures in all > >>>>>>>>> configurations. > >>>>>>>>> > >>>>>>>>> Also, I don't think bpftool is usable to dump kernel BPF selftests. > >>>>>>>>> That's vital when a selftest fails if you want to have a chance to > >>>>>>>>> understand why it fails. > >>>>>>>> > >>>>>>>> If this is actively used by JIT developers and considered useful, I'd be > >>>>>>>> ok to leave it for the time being. Overall goal is to reach feature parity > >>>>>>>> among (at least major arch) JITs and not just have most functionality only > >>>>>>>> available on x86-64 JIT. Could you however check what is not working with > >>>>>>>> bpftool on powerpc/32? Perhaps it's not too much effort to just fix it, > >>>>>>>> but details would be useful otherwise 'it didn't work' is too fuzzy. > >>>>>>> > >>>>>>> Sure I will try to test bpftool again in the coming days. > >>>>>>> > >>>>>>> Previous discussion about that subject is here: > >>>>>>> https://patchwork.kernel.org/project/linux-riscv/patch/20210415093250.3391257-1-Jianlin.Lv@xxxxxxx/#24176847= > >>>>>> Hi Christophe > >>>>>> Any progress? We discuss to deprecate the bpf_jit_enable == 2 in 2021, but bpftool can not run on powerpc. > >>>>>> Now can we fix this issue? > >>>>> > >>>>> Hi Tong, > >>>>> > >>>>> I have started to look at it but I don't have any fruitfull feedback yet. > >>>>> > >>>>> In the meantime, were you able to confirm that bpftool can also be used > >>>>> to dump jitted tests from test_bpf.ko module on x86_64 ? In that can you > >>>>> tell me how to proceed ? > >>>> Now I do not test, but we can dump the insn after bpf_prog_select_runtime in test_bpf.ko. bpf_map_get_info_by_fd can copy the insn to userspace, but we can > >>>> dump them in test_bpf.ko in the same way. > >>> > >>> Issue is that these progs are not consumable from userspace (and therefore not bpftool). > >>> it's just simple bpf_prog_alloc + copy of test insns + bpf_prog_select_runtime() to test > >>> JITs (see generate_filter()). Some of them could be converted over to test_verifier, but > >>> not all might actually pass verifier, iirc. Don't think it's a good idea to allow exposing > >>> them via fd tbh. > >> Hi > >> I mean that, can we invoke the bpf_jit_dump in test_bpf.ko directly ?. bpf_prog_get_info_by_fd copy the insn to userspace, but we only dump insn in test_bpf.ko > >> > >> if (bpf_dump_raw_ok(file->f_cred)) {// code copied from bpf_prog_get_info_by_fd, not tested > >> > >> /* for multi-function programs, copy the JITed > >> * instructions for all the functions > >> */ > >> if (prog->aux->func_cnt) { > >> for (i = 0; i < prog->aux->func_cnt; i++) { > >> len = prog->aux->func[i]->jited_len; > >> img = (u8 *) prog->aux->func[i]->bpf_func; > >> bpf_jit_dump(1, len, 1, img); > >> } > >> } else { > >> bpf_jit_dump(1, ulen, 1, prog->bpf_func); > >> } > >> } > > > > Let's not reinvent the wheel. > > bpftool prog dump jited > > is our supported command. > > ppc issue with bpftool is related to endianness of embedded skeleton. > > which means that none of the bpftool prog commands work on ppc. > > It's a bigger issue to address with cross compilation of bpftool. > > > > bpftool supports gnu and llvm disassembler. It retrieves and > > prints BTF, line info and source code along with asm. > > The user experience is at different level comparing to bpf_jit_dump. > > Hi Alexei, > > Fair enough, we are going to try and fix bpftool. > > But for test_bpf.ko module, how do you use bpftool to dump the BPF tests > ? Even on x86 I have not been able to use bpftool for that until now. > Can you tell me how you do ? test_bpf.ko is useful to JIT developers when they're starting to work on it, but its test coverage is inadequate for real world bpf usage comparing to selftests/bpf. Johan Almbladh did some great additions to test_bpf.ko back in 2021. Since then there wasn't much. Here it's important to distinguish the target user. Is it a kernel JIT developer or user space bpf prog developer? When it's a kernel developer they can easily add print_hex_dump() in the right places. That's what I did when I was developing bpf trampoline. bpf is more than just JIT. There are trampoline, kfuncs, dispatch. The kernel devs should not add a debug code. Long ago bpf_jit_enable=2 was useful to user space bpf developers. They wanted to see how JITed code look like to optimize it and what not. Now 'perf record' captures bpf asm and annotates it in 'perf report', so performance analysis problem is solved that way. bpftool prog dump jit addressed the needs of users and admins who want to understand what bpf progs are loaded and what are they doing. Both 'dump jited' and 'dump xlated' are useful for this case. So bpf_jit_enable=2 remained useful to kernel developers only and in that sense it become a kernel debug feature for a narrow set of JIT developers. On x86 bpf_jit_dump() was neglected and broken. I suspect the other archs will follow the same fate. If not already. Having a sysctl for kernel developers is not something the kernel developers should have around. Hence the cleanup of this patch.
