Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 20 Oct 2021 17:05:02 -0700
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     Peter Zijlstra <peterz@infradead.org>
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*
Message-ID: <20211021000502.ltn5o6ji6offwzeg@ast-mbp.dhcp.thefacebook.com>
References: <20211020104442.021802560@infradead.org>
 <20211020105843.345016338@infradead.org>
 <YW/4/7MjUf3hWfjz@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YW/4/7MjUf3hWfjz@hirez.programming.kicks-ass.net>
Precedence: bulk

On Wed, Oct 20, 2021 at 01:09:51PM +0200, Peter Zijlstra wrote:
> > -	RETPOLINE_RCX_BPF_JIT();
> > +	emit_indirect_jump(&prog, 1 /* rcx */, ip + (prog - start));
> >  
> >  	/* out: */
> >  	*pprog = prog;
> 
> Alexei; could the above not be further improved with something like the
> below?

sorry for delay. I was traveling last week
and Daniel is on PTO this week.

> Despite several hours trying and Song helping, I can't seem to run
> anything bpf, that stuff is cursed. So I've no idea if the below
> actually works, but it seems reasonable.

It's certainly delicate.

> @@ -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!
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.
A comment is certainly must have.
The race is possible too. Not sure whether READ_ONCE/WRITE_ONCE
are really warranted though. Might be overkill.

Nice that Josh's test_verifier is passing, but it doesn't provide
a ton of coverage. test_progs has a lot more.
Once you have a git branch with all the changes I can give it a go.
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.
It's pretty much the same as selftests/bpf/vmtest.sh, but with the latest
clang nightly and other deps like pahole.