2022-03-09 09:54 UTC+0100 ~ Niklas Söderlund <niklas.soderlund@xxxxxxxxxxxx>
> Commit 1a56c18e6c2e4e74 ("bpftool: Stop supporting BPF offload-enabled
> feature probing") removed the support to probe for BPF offload features.
> This is still something that is useful for NFP NIC that can support
> offloading of BPF programs.
>
> The reason for the dropped support was that libbpf starting with v1.0
> would drop support for passing the ifindex to the BPF prog/map/helper
> feature probing APIs. In order to keep this useful feature for NFP
> restore the functionality by moving it directly into bpftool.
>
> The code restored is a simplified version of the code that existed in
> libbpf which supposed passing the ifindex. The simplification is that it
> only targets the cases where ifindex is given and call into libbpf for
> the cases where it's not.
[...]
> +static bool probe_prog_type_ifindex(enum bpf_prog_type prog_type, __u32 ifindex)
> +{
> + struct bpf_insn insns[2] = {
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN()
> + };
> +
> + switch (prog_type) {
> + case BPF_PROG_TYPE_SCHED_CLS:
> + /* nfp returns -EINVAL on exit(0) with TC offload */
> + insns[0].imm = 2;
> + break;
> + case BPF_PROG_TYPE_XDP:
> + break;
> + default:
> + return false;
> + }
Looking again at this block, you can remove this switch/case completely.
Moving 0 by default into R0 was best in generic libbpf's probes because
there were a number of types that wouldn't accept 2 as a return value;
but here you can just return 2 all the time, for XDP this will mean
XDP_PASS and the nfp accepts it. (Do keep the comment on nfp returning
-EINVAL on exit(0), though, please.)
> +
> + return probe_prog_load_ifindex(prog_type, insns, ARRAY_SIZE(insns),
> + NULL, 0, ifindex);
> +}
> +
> static void
> probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types,
> const char *define_prefix, __u32 ifindex)
> @@ -488,11 +564,19 @@ probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types,
> bool res;
>
> if (ifindex) {
> - p_info("BPF offload feature probing is not supported");
> - return;
> + switch (prog_type) {
> + case BPF_PROG_TYPE_SCHED_CLS:
> + case BPF_PROG_TYPE_XDP:
> + break;
> + default:
> + return;
> + }
> +
> + res = probe_prog_type_ifindex(prog_type, ifindex);
> + } else {
> + res = libbpf_probe_bpf_prog_type(prog_type, NULL);
> }
>
> - res = libbpf_probe_bpf_prog_type(prog_type, NULL);
> #ifdef USE_LIBCAP
> /* Probe may succeed even if program load fails, for unprivileged users
> * check that we did not fail because of insufficient permissions
> @@ -521,6 +605,35 @@ probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types,
> define_prefix);
> }
>
> +static bool probe_map_type_ifindex(enum bpf_map_type map_type, __u32 ifindex)
> +{
> + LIBBPF_OPTS(bpf_map_create_opts, opts);
> + int key_size, value_size, max_entries;
> + int fd;
> +
> + opts.map_ifindex = ifindex;
> +
> + key_size = sizeof(__u32);
> + value_size = sizeof(__u32);
> + max_entries = 1;
> +
> + switch (map_type) {
> + case BPF_MAP_TYPE_HASH:
> + case BPF_MAP_TYPE_ARRAY:
> + break;
> + default:
> + return false;
> + }
This switch/case should probably not be here. Either skip
probe_map_type_ifindex() entirely if you assume other map types are not
supported, or just probe all types for real.
Speaking of which, we agreed yesterday on probing for all map types. But
I think my suggestion was wrong (apologies!), because the simplified
probes that you're adding back does not contain the necessary
adjustments to work with all map types (the switch(map_type) in libbpf's
probe_map_create()). So we should probably probe just hash/array maps,
but move the switch/case above to probe_map_type(), like we do for prog
types, to avoid printing any output for other map types. This will
prevent other drivers to probe for additional map types, but a large
portion of those probes would be broken, so it's for the best. We can
always extend probe_map_type_ifindex() in the future to support more
types if necessary.
> +
> + fd = bpf_map_create(map_type, NULL, key_size, value_size, max_entries,
> + &opts);
> +
> + if (fd >= 0)
> + close(fd);
> +
> + return fd >= 0;
> +}
> +
> static void
> probe_map_type(enum bpf_map_type map_type, const char *define_prefix,
> __u32 ifindex)
