Re: [PATCH v3 bpf-next 11/11] bpftool: support displaying relocated-with-base split BTF

[Quentin Monnet <qmo@kernel.org>]

2024-05-10 11:32 UTC+0100 ~ Alan Maguire <alan.maguire@oracle.com>
> If the -R <base_btf> option is used, we can display BTF that has been
> generated with distilled base BTF in its relocated form.  For example
> for bpf_testmod.ko (which is built as an out-of-tree module, so has
> a distilled .BTF.base section:
> 
> bpftool btf dump file bpf_testmod.ko
> 
> Alternatively, we can display content relocated with
> (a possibly changed) base BTF via
> 
> bpftool btf dump -R /sys/kernel/btf/vmlinux bpf_testmod.ko
> 
> The latter mirrors how the kernel will handle such split
> BTF; it relocates its representation with the running
> kernel, and if successful, renumbers BTF ids to reference
> the current vmlinux BTF.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/bpf/bpftool/Documentation/bpftool-btf.rst | 15 ++++++++++++++-
>  tools/bpf/bpftool/bash-completion/bpftool       |  7 ++++---
>  tools/bpf/bpftool/btf.c                         | 11 ++++++++++-
>  tools/bpf/bpftool/main.c                        | 14 +++++++++++++-
>  tools/bpf/bpftool/main.h                        |  2 ++
>  5 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> index eaba24320fb2..fd6bb1280e7b 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> @@ -16,7 +16,7 @@ SYNOPSIS
>  
>  **bpftool** [*OPTIONS*] **btf** *COMMAND*
>  
> -*OPTIONS* := { |COMMON_OPTIONS| | { **-B** | **--base-btf** } }
> +*OPTIONS* := { |COMMON_OPTIONS| | { **-B** | **--base-btf** } { **-R** | **relocate-base-btf** } }


The double-dash is missing at the beginning of --relocate-base-btf.


>  
>  *COMMANDS* := { **dump** | **help** }
>  
> @@ -85,6 +85,19 @@ OPTIONS
>      BTF object is passed through other handles, this option becomes
>      necessary.
>  
> +-R, --relocate-base-btf *FILE*
> +    When split BTF is generated with distilled base BTF for relocation,
> +    the latter is stored in a .BTF.base section and allows us to later
> +    relocate split BTF and a potentially-changed base BTF by using
> +    information in the .BTF.base section about the base types referenced
> +    from split BTF.  Relocation is carried out against the split BTF
> +    supplied via this parameter and the split BTF will then refer to
> +    the base types supplied in *FILE*.
> +
> +    If this option is not used, split BTF is shown relative to the
> +    .BTF.base, which contains just enough information to support later
> +    relocation.
> +
>  EXAMPLES
>  ========
>  **# bpftool btf dump id 1226**
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 04afe2ac2228..878cf3d49a76 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -262,7 +262,7 @@ _bpftool()
>      # Deal with options
>      if [[ ${words[cword]} == -* ]]; then
>          local c='--version --json --pretty --bpffs --mapcompat --debug \
> -            --use-loader --base-btf'
> +            --use-loader --base-btf --relocate-base-btf'
>          COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
>          return 0
>      fi
> @@ -283,7 +283,7 @@ _bpftool()
>              _sysfs_get_netdevs
>              return 0
>              ;;
> -        file|pinned|-B|--base-btf)
> +        file|pinned|-B|-R|--base-btf|--relocate-base-btf)
>              _filedir
>              return 0
>              ;;
> @@ -297,7 +297,8 @@ _bpftool()
>      local i pprev
>      for (( i=1; i < ${#words[@]}; )); do
>          if [[ ${words[i]::1} == - ]] &&
> -            [[ ${words[i]} != "-B" ]] && [[ ${words[i]} != "--base-btf" ]]; then
> +            [[ ${words[i]} != "-B" ]] && [[ ${words[i]} != "--base-btf" ]] &&
> +            [[ ${words[i]} != "-R" ]] && [[ ${words[i]} != "--relocate-base-btf" ]]; then
>              words=( "${words[@]:0:i}" "${words[@]:i+1}" )
>              [[ $i -le $cword ]] && cword=$(( cword - 1 ))
>          else
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 0ca1f2417801..34f60d9e433d 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -638,6 +638,14 @@ static int do_dump(int argc, char **argv)
>  			base_btf = btf__parse_opts(*argv, &optp);
>  			if (base_btf)
>  				btf = btf__parse_split(*argv, base_btf);
> +			if (btf && relocate_base_btf) {
> +				err = btf__relocate(btf, relocate_base_btf);
> +				if (err) {
> +					p_err("could not relocate BTF from '%s' with base BTF '%s': %s\n",
> +					      *argv, relocate_base_btf_path, strerror(-err));
> +					goto done;
> +				}
> +			}
>  		}
>  		if (!btf) {
>  			err = -errno;
> @@ -1075,7 +1083,8 @@ static int do_help(int argc, char **argv)
>  		"       " HELP_SPEC_MAP "\n"
>  		"       " HELP_SPEC_PROGRAM "\n"
>  		"       " HELP_SPEC_OPTIONS " |\n"
> -		"                    {-B|--base-btf} }\n"
> +		"                    {-B|--base-btf} |\n"
> +		"                    {-R|--relocate-base-btf} }\n"
>  		"",
>  		bin_name, "btf");
>  
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 08d0ac543c67..69d4906bec5c 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -32,6 +32,8 @@ bool verifier_logs;
>  bool relaxed_maps;
>  bool use_loader;
>  struct btf *base_btf;
> +struct btf *relocate_base_btf;
> +const char *relocate_base_btf_path;
>  struct hashmap *refs_table;
>  
>  static void __noreturn clean_and_exit(int i)
> @@ -448,6 +450,7 @@ int main(int argc, char **argv)
>  		{ "debug",	no_argument,	NULL,	'd' },
>  		{ "use-loader",	no_argument,	NULL,	'L' },
>  		{ "base-btf",	required_argument, NULL, 'B' },
> +		{ "relocate-base-btf", required_argument, NULL, 'R' },

Nit: The lines above yours use tabs to visually align the different
fields, would you mind (optionally) re-aligning them, or at least using
tabs in your own line, please?

Other than these, the changes look good to me, thank you

Reviewed-by: Quentin Monnet <qmo@kernel.org>