Re: [PATCH v2 14/14] bpf,x86: Respect X86_FEATURE_RETPOLINE*

From: Peter Zijlstra <peterz@infradead.org>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: x86@kernel.org, jpoimboe@redhat.com, andrew.cooper3@citrix.com,
	linux-kernel@vger.kernel.org, ndesaulniers@google.com,
	daniel@iogearbox.net, bpf@vger.kernel.org, andrii@kernel.org
Subject: Re: [PATCH v2 14/14] bpf,x86: Respect X86_FEATURE_RETPOLINE*
Date: Thu, 21 Oct 2021 10:47:00 +0200	[thread overview]
Message-ID: <YXEpBKxUICIPVj14@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20211021000502.ltn5o6ji6offwzeg@ast-mbp.dhcp.thefacebook.com>

On Wed, Oct 20, 2021 at 05:05:02PM -0700, Alexei Starovoitov wrote:
> On Wed, Oct 20, 2021 at 01:09:51PM +0200, Peter Zijlstra wrote:

> > @@ -446,25 +440,8 @@ static void emit_bpf_tail_call_indirect(
> >  {
> >  	int tcc_off = -4 - round_up(stack_depth, 8);
> >  	u8 *prog = *pprog, *start = *pprog;
> > -	int pop_bytes = 0;
> > -	int off1 = 42;
> > -	int off2 = 31;
> > -	int off3 = 9;
> > -
> > -	/* count the additional bytes used for popping callee regs from stack
> > -	 * that need to be taken into account for each of the offsets that
> > -	 * are used for bailing out of the tail call
> > -	 */
> > -	pop_bytes = get_pop_bytes(callee_regs_used);
> > -	off1 += pop_bytes;
> > -	off2 += pop_bytes;
> > -	off3 += pop_bytes;
> > -
> > -	if (stack_depth) {
> > -		off1 += 7;
> > -		off2 += 7;
> > -		off3 += 7;
> > -	}
> > +	static int out_label = -1;
> 
> Interesting idea!

I nicked it from emit_bpf_tail_call() in the 32bit jit :-) It seemed a
lot more robust than the 64bit one and I couldn't figure out why the
difference.

> All insn emits trying to do the right thing from the start.
> Here the logic assumes that there will be at least two passes over image.
> I think that is correct, but we never had such assumption.

That's not exactly true; I think image is NULL on every first run, so
all insn that depend on it will be wrong to start with. Equally there's
a number of insn that seem to depend on addrs[i], that also requires at
least two passes.

> A comment is certainly must have.

I can certainly add one, although I think we'll disagree on the comment
style :-)

> The race is possible too. Not sure whether READ_ONCE/WRITE_ONCE
> are really warranted though. Might be overkill.

Is there concurrency on the jit?

> Once you have a git branch with all the changes I can give it a go.

Ok, I'll go polish this thing and stick it in the tree mentioned in the
cover letter.

> Also you can rely on our BPF CI.
> Just cc your patchset to bpf@vger and add [PATCH bpf-next] to a subject.
> In patchwork there will be "bpf/vmtest-bpf-next" link that
> builds kernel, selftests and runs everything.

What's a patchwork and where do I find it?

> It's pretty much the same as selftests/bpf/vmtest.sh, but with the latest
> clang nightly and other deps like pahole.

nice.