Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jiri Olsa <olsajiri@gmail.com>
Date:   Mon, 31 Oct 2022 22:53:47 +0100
To:     Peter Zijlstra <peterz@infradead.org>,
        =?iso-8859-1?Q?Bj=F6rn_T=F6pel?= <bjorn@kernel.org>,
        Toke =?iso-8859-1?Q?H=F8iland-J=F8rgensen?= <toke@redhat.com>
Cc:     Jiri Olsa <olsajiri@gmail.com>,
        David Laight <David.Laight@aculab.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        bpf@vger.kernel.org
Subject: Re: Linux 6.1-rc3 build fail in include/linux/bpf.h
Message-ID: <Y2BD6xZ108lv3j7J@krava>
References: <439d8dc735bb4858875377df67f1b29a@AcuMS.aculab.com>
 <Y1+8zIdf8mgQXwHg@krava>
 <Y1/oBlK0yFk5c/Im@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Y1/oBlK0yFk5c/Im@hirez.programming.kicks-ass.net>
Precedence: bulk

On Mon, Oct 31, 2022 at 04:21:42PM +0100, Peter Zijlstra wrote:
> On Mon, Oct 31, 2022 at 01:17:16PM +0100, Jiri Olsa wrote:
> > On Mon, Oct 31, 2022 at 11:14:31AM +0000, David Laight wrote:
> > > The 6.1-rc3 sources fail to build because bpf.h unconditionally
> > > #define BPF_DISPATCHER_ATTRIBUTES __attribute__((patchable_function_entry(5)))
> > > for X86_64 builds.
> > > 
> > > I'm pretty sure that should depend on some other options
> > > since the compiler isn't required to support it.
> > > (The gcc 7.5.0 on my Ubunti 18.04 system certainly doesn't)
> > > 
> > > The only other reference to that attribute is in the definition
> > > of 'notrace' in compiler.h.
> > 
> > I guess we need to make some __has_attribute check and make all that conditional
> > 
> > cc-ing Peter
> 
> Does something crazy like the below work? It compiles but is otherwise
> totally untested.

looks good

it has now the ftrace nop and the jump to the dispatcher image
or the bpf_dispatcher_nop_func.. great :)

	bpf_dispatcher_xdp_func:

	ffffffff81cc87b0 <load1+0xcc87b0>:
	ffffffff81cc87b0:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
	ffffffff81cc87b5:       e9 a6 fe 65 ff          jmp    0xffffffff81328660

tests work for me..  Toke, Bj?rn, could you please check?

thanks,
jirka


> 
> ---
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9e7d46d16032..7d7a00306d19 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -953,6 +953,10 @@ struct bpf_dispatcher {
>  	void *rw_image;
>  	u32 image_off;
>  	struct bpf_ksym ksym;
> +#ifdef CONFIG_HAVE_STATIC_CALL
> +	struct static_call_key *sc_key;
> +	void *sc_tramp;
> +#endif
>  };
>  
>  static __always_inline __nocfi unsigned int bpf_dispatcher_nop_func(
> @@ -970,6 +974,20 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key,
>  					  struct bpf_attach_target_info *tgt_info);
>  void bpf_trampoline_put(struct bpf_trampoline *tr);
>  int arch_prepare_bpf_dispatcher(void *image, void *buf, s64 *funcs, int num_funcs);
> +
> +
> +#ifdef CONFIG_HAVE_STATIC_CALL
> +#define BPF_DISPATCH_CALL(name)	static_call(bpf_dispatcher_##name##_call)(ctx, insnsi, bpf_func)
> +
> +#define __BPF_DISPATCHER_SC_INIT(_name)				\
> +	.sc_key = &STATIC_CALL_KEY(_name),			\
> +	.sc_tramp = STATIC_CALL_TRAMP_ADDR(_name),
> +
> +#else
> +#define BPF_DISPATCH_CALL(name)	bpf_func(ctx, insnsi)
> +#define __BPF_DISPATCHER_SC_INIT(name)
> +#endif
> +
>  #define BPF_DISPATCHER_INIT(_name) {				\
>  	.mutex = __MUTEX_INITIALIZER(_name.mutex),		\
>  	.func = &_name##_func,					\
> @@ -981,32 +999,29 @@ int arch_prepare_bpf_dispatcher(void *image, void *buf, s64 *funcs, int num_func
>  		.name  = #_name,				\
>  		.lnode = LIST_HEAD_INIT(_name.ksym.lnode),	\
>  	},							\
> +	__BPF_DISPATCHER_SC_INIT(_name##_call)			\
>  }
>  
> -#ifdef CONFIG_X86_64
> -#define BPF_DISPATCHER_ATTRIBUTES __attribute__((patchable_function_entry(5)))
> -#else
> -#define BPF_DISPATCHER_ATTRIBUTES
> -#endif
> -
>  #define DEFINE_BPF_DISPATCHER(name)					\
> -	notrace BPF_DISPATCHER_ATTRIBUTES				\
> +	DEFINE_STATIC_CALL(bpf_dispatcher_##name##_call, bpf_dispatcher_nop_func); \
>  	noinline __nocfi unsigned int bpf_dispatcher_##name##_func(	\
>  		const void *ctx,					\
>  		const struct bpf_insn *insnsi,				\
>  		bpf_func_t bpf_func)					\
>  	{								\
> -		return bpf_func(ctx, insnsi);				\
> +		return BPF_DISPATCH_CALL(name);				\
>  	}								\
>  	EXPORT_SYMBOL(bpf_dispatcher_##name##_func);			\
>  	struct bpf_dispatcher bpf_dispatcher_##name =			\
>  		BPF_DISPATCHER_INIT(bpf_dispatcher_##name);
> +
>  #define DECLARE_BPF_DISPATCHER(name)					\
>  	unsigned int bpf_dispatcher_##name##_func(			\
>  		const void *ctx,					\
>  		const struct bpf_insn *insnsi,				\
>  		bpf_func_t bpf_func);					\
>  	extern struct bpf_dispatcher bpf_dispatcher_##name;
> +
>  #define BPF_DISPATCHER_FUNC(name) bpf_dispatcher_##name##_func
>  #define BPF_DISPATCHER_PTR(name) (&bpf_dispatcher_##name)
>  void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
> index fa64b80b8bca..1ca8bd6da6bb 100644
> --- a/kernel/bpf/dispatcher.c
> +++ b/kernel/bpf/dispatcher.c
> @@ -4,6 +4,7 @@
>  #include <linux/hash.h>
>  #include <linux/bpf.h>
>  #include <linux/filter.h>
> +#include <linux/static_call.h>
>  
>  /* The BPF dispatcher is a multiway branch code generator. The
>   * dispatcher is a mechanism to avoid the performance penalty of an
> @@ -106,7 +107,6 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
>  {
>  	void *old, *new, *tmp;
>  	u32 noff;
> -	int err;
>  
>  	if (!prev_num_progs) {
>  		old = NULL;
> @@ -128,11 +128,10 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
>  			return;
>  	}
>  
> -	err = bpf_arch_text_poke(d->func, BPF_MOD_JUMP, old, new);
> -	if (err || !new)
> -		return;
> +	__static_call_update(d->sc_key, d->sc_tramp, new ?: &bpf_dispatcher_nop_func);
>  
> -	d->image_off = noff;
> +	if (new)
> +		d->image_off = noff;
>  }
>  
>  void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,