On Tue, Nov 29, 2022 at 7:00 PM Florent Revest <revest@xxxxxxxxxxxx> wrote: > > On Thu, Nov 24, 2022 at 4:50 PM Benjamin Tissoires > <benjamin.tissoires@xxxxxxxxxx> wrote: > > > > On Wed, Nov 23, 2022 at 9:14 PM Alexei Starovoitov > > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > > > On Wed, Nov 23, 2022 at 6:53 AM Benjamin Tissoires > > > <benjamin.tissoires@xxxxxxxxxx> wrote: > > > > > > > > Hi Jon, > > > > > > > > On Wed, Nov 23, 2022 at 2:25 PM Jon Hunter <jonathanh@xxxxxxxxxx> wrote: > > > > > > > > > > We have a kernel test that checks for new warning and error messages on > > > > > boot and with this change I am now seeing the following error message on > > > > > our Tegra platforms ... > > > > > > > > > > WARNING KERN hid_bpf: error while preloading HID BPF dispatcher: -13 > > > > > > > > > > I have a quick look at the code, but I can't say I am familiar with > > > > > this. So I wanted to ask if a way to fix this or avoid this? I see the > > > > > code returns 0, so one option would be to make this an informational or > > > > > debug print. > > > > > > > > I am not in favor of debug in that case, because I suspect it'll hide > > > > too much when getting a bug report. Informational could do, yes. > > > > > > > > However, before that, I'd like to dig a little bit more on why it is > > > > failing. I thought arm64 now has support of tracing bpf programs, so I > > > > would not expect this to fail. > > We have BPF trampolines on arm64 already but no ftrace direct calls > right now. (so trampolines get jitted but don't get called eheh :)) So > indeed BPF tracing programs (fentry/fexit/fmod_ret) do not work on > arm64 at the moment. Oh, OK. Thanks for the explanation. > > > > Unfortunately the patches to add support for such tracing bpf progs got stuck. > > > Florent/Mark can probably share the latest status. > > We are working on an implementation of ftrace direct calls that would > fit within the constraints of arm64 and play nice with the other users > of the ftrace call site. Hopefully we have a patch to share in the > next couple of weeks I'd say! yay! > > > Oh... I noticed Jon's config was lacking CONFIG_FTRACE. So should I > > also add a depends on CONFIG_FTRACE to enable HID-BPF? > > If HID-BPF fundamentally depends on a fmod_ret program being attached > to function, it seems to me that it should depend on both: > CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS (CONFIG_FTRACE or even > CONFIG_DYNAMIC_FTRACE aren't enough, there can be architectures that > do not support direct calls. here you noticed it on arm64 which > luckily should get fixed someday soon but there could be other > architectures with that issue too) > and OK > CONFIG_FUNCTION_ERROR_INJECTION (since [1] error injection needs > to be explicitly opted-in, it's easy to miss it and fail to attach > fmod_ret programs in mysterious ways) Ok as well. > > I'm thinking that maybe encoding these two dependencies in the > CONFIG_HID_BPF is leaking too much of the bpf tracing abstraction to > the user. Maybe the BPF Kconfig could provide "proxy" configs like > HAVE_BPF_FENTRY_FEXIT, HAVE_BPF_FMOD_RET (naming is hard) to expose > these dependencies better ? That would be nice, but requires some sort of synchronization between our 2 trees, so I'll take the 2 configs in the HID tree, and try to submit a patch for the bpf tree with the macro. Then I can reattach to that macro when it hits Linus'. > > 1: https://lore.kernel.org/lkml/20221121104403.1545f9b5@xxxxxxxxxxxxxxxxxx/ > That thread is a little bit worrying to me. HID-BPF relies on CONFIG_FUNCTION_ERROR_INJECTION, and I would definitely like to see HID-BPF in production kernels. I don't really care about cloud servers, but chromebooks are something I'd like to have enabled. We'll see how this thread goes I guess. Anyway, thanks a lot for the deep explanation and understanding of my code :) Cheers, Benjamin
