On 01/12/2018 03:17 AM, Jakub Kicinski wrote:
> On Thu, 11 Jan 2018 16:47:47 -0800, Jakub Kicinski wrote:
>> Hi!
>>
>> Jiong is working on dumping JITed NFP image via bpftool, Francois will be
>> submitting support for NFP in binutils soon (whoop! :)).
>>
>> We would appreciate if you could weigh in on the uAPI. Is it OK to reuse
>> the existing jited_prog_len/jited_prog_insns or should we add separate
>> 2 new fields (plus the arch name) to avoid confusing old user space?
>
> Ah, I skipped one chunk of Jiong's patch here, this would also be
> necessary if we reuse fields:
What kind of string would sit in jited_arch_name for nfp? Given you know
the {ifindex, netns_dev, netns_ino} 3‑tuple, isn't it possible to infer
the driver info from ethtool (e.g. nfp_net_get_drvinfo()) already and do
the mapping for binutils? Given we know when ifindex is 0, then its
always host JITed and the current approach with bfd_openr() works fine,
and if ifindex is non-0 it is /always/ device offloaded to the one bound
in the ifindex so JIT dump will be specific to that device only and
never host one. Not at all opposed to extending uapi, just a question
on my side to get a better understanding first wrt arch string (maybe
rough but complete patch with nfp + bpftool bits might help too).
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 2bac0dc..c7831cd 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1673,19 +1673,6 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog
> *prog,
> goto done;
> }
>
> - ulen = info.jited_prog_len;
> - info.jited_prog_len = prog->jited_len;
> - if (info.jited_prog_len && ulen) {
> - if (bpf_dump_raw_ok()) {
> - uinsns = u64_to_user_ptr(info.jited_prog_insns);
> - ulen = min_t(u32, info.jited_prog_len, ulen);
> - if (copy_to_user(uinsns, prog->bpf_func, ulen))
> - return -EFAULT;
> - } else {
> - info.jited_prog_insns = 0;
> - }
> - }
> -
> ulen = info.xlated_prog_len;
> info.xlated_prog_len = bpf_prog_insn_size(prog);
> if (info.xlated_prog_len && ulen) {
> @@ -1711,6 +1698,21 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog
> *prog,
> err = bpf_prog_offload_info_fill(&info, prog);
> if (err)
> return err;
> + else
> + goto done;
> + }
> +
> + ulen = info.jited_prog_len;
> + info.jited_prog_len = prog->jited_len;
> + if (info.jited_prog_len && ulen) {
> + if (bpf_dump_raw_ok()) {
> + uinsns = u64_to_user_ptr(info.jited_prog_insns);
> + ulen = min_t(u32, info.jited_prog_len, ulen);
> + if (copy_to_user(uinsns, prog->bpf_func, ulen))
> + return -EFAULT;
> + } else {
> + info.jited_prog_insns = 0;
> + }
> }
>
> done:
>
> info.jited_prog_len is an in/out parameter, so we can't write it twice
> if we share fields.. Sorry for messing up.
