Re: [PATCH] powerpc64/bpf: support direct_call on livepatch function

[Hari Bathini <hbathini@linux.ibm.com>]

Thanks for the review, Naveen.
I was on leave for sometime and could not look into it in a while
after that.

On 15/10/25 11:48 am, Naveen N Rao wrote:
> On Fri, Oct 10, 2025 at 12:47:21PM +0530, Hari Bathini wrote:
>>
>>
>> On 09/10/25 4:57 pm, Naveen N Rao wrote:
>>> On Thu, Oct 09, 2025 at 11:19:45AM +0530, Hari Bathini wrote:
>>>>
>>>>
>>>> On 08/10/25 1:43 pm, Naveen N Rao wrote:
>>>>> On Mon, Oct 06, 2025 at 06:50:20PM +0530, Hari Bathini wrote:
>>>>>>
>>>>>>
>>>>>> On 06/10/25 1:22 pm, Naveen N Rao wrote:
>>>>>>> On Fri, Oct 03, 2025 at 12:57:54AM +0530, Hari Bathini wrote:
>>>>>>>> Today, livepatch takes precedence over direct_call. Instead, save the
>>>>>>>> state and make direct_call before handling livepatch.
>>>>>>>
>>>>>>> If we call into the BPF trampoline first and if we have
>>>>>>> BPF_TRAMP_F_CALL_ORIG set, does this result in the BPF trampoline
>>>>>>> calling the new copy of the live-patched function or the old one?
>>>>>>
>>>>>> Naveen, calls the new copy of the live-patched function..
>>>>>
>>>>> Hmm... I'm probably missing something.
>>>>>
>>>>> With ftrace OOL stubs, what I recall is that BPF trampoline derives the
>>>>> original function address from the OOL stub (which would be associated
>>>>> with the original function, not the livepatch one).
>>>>
>>>> Trampoline derives the address from LR.
>>>
>>> Does it? I'm referring to BPF_TRAMP_F_CALL_ORIG handling in
>>> __arch_prepare_bpf_trampoline().
>>
>>
>>> LR at BPF trampoline entry points at
>>> the ftrace OOL stub. We recover the "real LR" pointing to the function
>>> being traced from there so that we can call into it from within the BPF
>>> trampoline.
>>
>> Naveen, from the snippet in livepatch_handler code shared below,
>> the LR at BPF trmapoline entry points at the 'nop' after the call
>> to trampoline with 'bnectrl cr1' in the updated livepatch_handler.
>>
>> Mimic'ing ftrace OOL branch instruction in livepatch_handler
>> with 'b	1f' (the instruction after nop) to ensure the trmapoline
>> derives the real LR to '1f' and jumps back into the livepatch_handler..
>>
>> +       /* Jump to the direct_call */
>> +       bnectrl cr1
>> +
>> +       /*
>> +        * The address to jump after direct call is deduced based on ftrace
>> OOL stub sequence.
>> +        * The seemingly insignificant couple of instructions below is to
>> mimic that here to
>> +        * jump back to the livepatch handler code below.
>> +        */
>> +       nop
>> +       b       1f
>> +
>> +       /*
>> +        * Restore the state for livepatching from the livepatch stack.
>> +        * Before that, check if livepatch stack is intact. Use r0 for it.
>> +        */
>> +1:     mtctr   r0
> 
> Ah, so you are faking a ftrace OOL stub here. But, won't this mean that

Yeah.

> bpf_get_func_ip() won't return the function address anymore?

Right. I do agree it can have issues in some scenarios.

> 
> One of the other thoughts I had was if we could stuff the function
> address into the ftrace OOL stub. I had considered this back when I
> implemented the OOL stubs, but didn't do it due to the extra memory
> requirement. However, given the dance we're having to do, I'm now
> thinking that may make sense and can simplify the code. If we can also
> hook into livepatch, then we should be able to update the function
> address in the stub to point to the new address and the trampoline
> should then "just work" since it already saves/restores the TOC [We may
> additionally have to update the function IP in _R12, but that would be a
> minor change overall]
> 
> We will still need a way to restore livepatch TOC if the BPF trampoline
> doesn't itself call into the function, but we may be able to handle that
> if we change the return address to jump to a stub that restores the TOC
> from the livepatch stack.

Sounds doable. Looking into a couple of other things at the moment
though. Will try out this suggestion and get back post that.
Having said that, your thoughts on whether the current approach
is a viable option if bpf_get_func_ip() can be fixed somehow?

- Hari