Received-SPF: pass (google.com: domain of linux-kernel+bounces-16579-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Jiri Olsa <olsajiri@gmail.com>
Date: Thu, 4 Jan 2024 12:23:03 +0100
To: Daniel Xu <dxu@dxuuu.xyz>
Cc: martin.lau@linux.dev, andrii@kernel.org, daniel@iogearbox.net,
	ast@kernel.org, alexei.starovoitov@gmail.com, olsajiri@gmail.com,
	quentin@isovalent.com, alan.maguire@oracle.com, memxor@gmail.com,
	song@kernel.org, yonghong.song@linux.dev, john.fastabend@gmail.com,
	kpsingh@kernel.org, sdf@google.com, haoluo@google.com,
	bpf@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next 1/2] bpf: btf: Support optional flags for
 BTF_SET8 sets
Message-ID: <ZZaVFxMvmMjbOlra@krava>
References: <cover.1704324602.git.dxu@dxuuu.xyz>
 <29644dc7906c7c0e6843d8acf92c3e29089845d0.1704324602.git.dxu@dxuuu.xyz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <29644dc7906c7c0e6843d8acf92c3e29089845d0.1704324602.git.dxu@dxuuu.xyz>

On Wed, Jan 03, 2024 at 04:31:55PM -0700, Daniel Xu wrote:
> This commit adds support for optional flags on BTF_SET8s.
> struct btf_id_set8 already supported 32 bits worth of flags, but was
> only used for alignment purposes before.
> 
> We now use these bits to encode flags. The next commit will tag all
> kfunc sets with a flag so that pahole can recognize which
> BTF_ID_FLAGS(func, ..) are actual kfuncs.
> 
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  include/linux/btf_ids.h | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index a9cb10b0e2e9..88f914579fa1 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -183,17 +183,21 @@ extern struct btf_id_set name;
>   * .word (1 << 3) | (1 << 1) | (1 << 2)
>   *
>   */
> -#define __BTF_SET8_START(name, scope)			\
> +#define ___BTF_SET8_START(name, scope, flags)		\
>  asm(							\
>  ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
>  "." #scope " __BTF_ID__set8__" #name ";        \n"	\
>  "__BTF_ID__set8__" #name ":;                   \n"	\
> -".zero 8                                       \n"	\
> +".zero 4                                       \n"	\
> +".long " #flags                               "\n"	\
>  ".popsection;                                  \n");
>  
> -#define BTF_SET8_START(name)				\
> +#define __BTF_SET8_START(name, scope, flags, ...)	\
> +___BTF_SET8_START(name, scope, flags)
> +
> +#define BTF_SET8_START(name, ...)			\
>  __BTF_ID_LIST(name, local)				\
> -__BTF_SET8_START(name, local)
> +__BTF_SET8_START(name, local, ##__VA_ARGS__, 0)

I think it'd better to use something like:

  BTF_SET8_KFUNCS_START(fsverity_set_ids)

instead of:

  BTF_SET8_START(fsverity_set_ids, BTF_SET8_KFUNC)

and to keep current BTF_SET8_START without flags argument, like:

  #define BTF_SET8_START(name) \
    __BTF_SET8_START(... , 0, ...

  #define BTF_SET8_KFUNCS_START(name) \
    __BTF_SET8_START(... , BTF_SET8_KFUNC, ...


also I'd rename BTF_SET8_KFUNC to BTF_SET8_KFUNCS (with S)

do you have the pahole changes somewhere? would be great to
see all the related changes and try the whole thing

jirka


>  
>  #define BTF_SET8_END(name)				\
>  asm(							\
> @@ -214,7 +218,7 @@ extern struct btf_id_set8 name;
>  #define BTF_SET_START(name) static struct btf_id_set __maybe_unused name = { 0 };
>  #define BTF_SET_START_GLOBAL(name) static struct btf_id_set __maybe_unused name = { 0 };
>  #define BTF_SET_END(name)
> -#define BTF_SET8_START(name) static struct btf_id_set8 __maybe_unused name = { 0 };
> +#define BTF_SET8_START(name, ...) static struct btf_id_set8 __maybe_unused name = { 0 };
>  #define BTF_SET8_END(name)
>  
>  #endif /* CONFIG_DEBUG_INFO_BTF */
> -- 
> 2.42.1
>