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

[Pu Lehui <pulehui@huaweicloud.com>]

[-- Attachment #1: Type: text/plain, Size: 3694 bytes --]



On 2024/2/1 21:35, Björn Töpel wrote:
> Pu Lehui <pulehui@huaweicloud.com> writes:
> 
>> On 2024/2/1 18:10, Björn Töpel wrote:
>>> Pu Lehui <pulehui@huaweicloud.com> 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 #2: tailcalls.JPG --]
[-- Type: image/jpeg, Size: 92740 bytes --]

[-- Attachment #3: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv