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

[Björn Töpel]

Pu Lehui <pulehui at huaweicloud.com> writes:

> On 2024/2/1 21:35, Björn Töpel wrote:
>> Pu Lehui <pulehui at huaweicloud.com> writes:
>> 
>>> On 2024/2/1 18:10, Björn Töpel wrote:
>>>> Pu Lehui <pulehui at 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:

Indeed! tailcall *is* already populating a6, and yes, the store can be
omitted. Nice!

Now, we still have the bug Alexei described, so until there's a
fix/workaround, this series can't be merged.


Cheers,
Björn