Re: [PATCH bpf-next v2 4/4] riscv, bpf: Mixing bpf2bpf and tailcalls

[Pu Lehui <pulehui@xxxxxxxxxxxxxxx>]

On 2024/2/1 21:35, Björn Töpel wrote:
> Pu Lehui <pulehui@xxxxxxxxxxxxxxx> writes:
>
> > On 2024/2/1 18:10, Björn Töpel wrote:
> > > Pu Lehui <pulehui@xxxxxxxxxxxxxxx> writes:
> > >
> > > > > > @@ -252,10 +220,7 @@ static void __build_epilogue(bool is_tail_call, struct rv_jit_context *ctx)
> > > > > >     		emit_ld(RV_REG_S5, store_offset, RV_REG_SP, ctx);
> > > > > >     		store_offset -= 8;
> > > > > >     	}
> > > > > > -	if (seen_reg(RV_REG_S6, ctx)) {
> > > > > > -		emit_ld(RV_REG_S6, store_offset, RV_REG_SP, ctx);
> > > > > > -		store_offset -= 8;
> > > > > > -	}
> > > > > > +	emit_ld(RV_REG_TCC, store_offset, RV_REG_SP, ctx);
> > > > >
> > > > > Why do you need to restore RV_REG_TCC? We're passing RV_REG_TCC (a6) as
> > > > > an argument at all call-sites, and for tailcalls we're loading from the
> > > > > stack.
> > > > >
> > > > > Is this to fake the a6 argument for the tail-call? If so, it's better to
> > > > > move it to emit_bpf_tail_call(), instead of letting all programs pay for
> > > > > it.
> > > >
> > > > Yes, we can remove this duplicate load. will do that at next version.
> > >
> > > Hmm, no remove, but *move* right? Otherwise a6 can contain gargabe on
> > > entering the tailcall?
> > >
> > > Move it before __emit_epilogue() in the tailcall, no?
> >
> > IIUC, we don't need to load it again. In emit_bpf_tail_call function, we
> > load TCC from stack to A6, A6--, then store A6 back to stack. Then
> > unwind the current stack and jump to target bpf prog, during this
> > period, we did not touch the A6 register, do we still need to load it again?
>
> a6 has to be populated prior each call -- including tailcalls. An
> example, how it can break:
>
> main_prog() -> prologue (a6 := 0; push a6) -> bpf_helper() (random
> kernel path that clobbers a6) -> tailcall(foo()) (unwinds stack, enters

It's OK to clobbers A6 reg for helper/kfunc call, because we will load 
TCC from stack to A6 reg before jump to tailcall target prog. In 
addition, I found that we can remove the store A6 back to stack command 
from the tailcall process. I try to describe the process involved:

PS: I'm attaching a picture to avoid the formatting below.

Main prog
	A6 reg = 33
	STORE A6 value(TCC=33) to main prog stack
	...
	/* helper call/kfunc call (not call to bpf prog)*/
	LOAD TCC=33 from main prog stack to A6 reg
	call bpf_helper_prog1/kfunc1
		bpf_helper_prog1/kfunc1
			break A6 reg
			return Main prog
	/* it's ok to break A6 reg, because we have stored A6 value to main 
prog stack */
	...
	/* start tailcall(foo) */
	LOAD TCC=33 from main prog stack to A6 reg
	A6--: TCC=32
	STORE A6 value(TCC=32) to main prog stack
	UNWIND Main prog stack (at this time, A6 value 32 will not on any stack)
	/* at this time, A6 reg is not affected. */
	jump to foo()
		foo() --- tailcall target
			STORE A6 value(TCC=32) to foo prog stack
			...
			/* subprog call (call to bpf prog)*/
			LOAD TCC=32 from foo prog stack to A6 reg
			call subprog1
				subprog1
					STORE A6 value(TCC=32) to subprog1 stack
					...
					/* start tailcall(foo2) */
					LOAD TCC=32 from subprog1 stack to A6 reg
					A6--:TCC=31
					STORE A6 value(TCC=31) to subprog1 stack
					UNWIND subprog1 stack (at this time, `old` A6 value 32 still in foo 
prog stack)
					/* at this time, A6 reg is not affected. */
					jump to foo2()
						foo2() --- tailcall target
							STORE A6 value(TCC=31) to foo2 prog stack
							...
							UNWIND foo2 prog stack (at this time, `old` A6 value 32 still in 
foo prog stack)
							return to foo()
		...
		/* if have any call will load `old` A6 value 32 to A6 reg */
		...
		UNWIND foo prog stack (at this time, old A6 32 will not on any stack)
		return to the caller of Main prog

> foo() with a6 garbage, and push a6).
>
> Am I missing something?

Attachment: tailcalls.JPG
Description: JPEG image