Le 18/01/2023 à 18:42, Alexei Starovoitov a écrit : > 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. Ok, I understand. Then it means that ' bpf_jit_enable == 2' can be removed once 'bpftool' properly works for dumping userspace BPF progs. But for the time being it doesn't seem to work, see my other answer. Christophe
