Re: [PATCH] powerpc64: Workaround sigtramp vdso return call.

[Nicholas Piggin <npiggin@gmail.com>]

+linuxppc-dev

Excerpts from Raoni Fassina Firmino's message of January 28, 2021 2:21 am:
> On Tue, Jan 26, 2021 at 08:45:00AM -0600, AL glibc-alpha wrote:
>> 
>> 
>> On 1/26/21 8:12 AM, Florian Weimer via Libc-alpha wrote:
>> > * Raoni Fassina Firmino:
>> > 
>> > > A not so recent kernel change[1] changed how the trampoline
>> > > `__kernel_sigtramp_rt64` is used to call signal handlers.
>> > > 
>> > > This was exposed on the test misc/tst-sigcontext-get_pc
>> > > 
>> > > Before kernel 5.9, the kernel set LR to the trampoline address and
>> > > jumped directly to the signal handler, and at the end the signal
>> > > handler, as any other function, would `blr` to the address set.  In
>> > > other words, the trampoline was executed just at the end of the signal
>> > > handler and the only thing it did was call sigreturn.  But since
>> > > kernel 5.9 the kernel set CTRL to the signal handler and calls to the
>> > > trampoline code, the trampoline then `bctrl` to the address in CTRL,
>> > > setting the LR to the next instruction in the middle of the
>> > > trampoline, when the signal handler returns, the rest of the
>> > > trampoline code executes the same code as before.
>> > 
>> > Thanks for the patch, byt:
>> > 
>> > No one has explained so far why the original blr instruction couldn't be
>> > augmented with the appropriate branch predictor hint.  The 2.07 ISA
>> > manual suggests that it's possible, but maybe I'm reading it wrong.
>> 
>> bctrl is the preferred form of making indirect calls.  You can add hint 0b01
>> to bclr (blr) to get similar behavior on power8/9, but as noted in the ISA,
>> it is optional.
> 
> What branch prediction we are talking about? I think there is only one
> blr that is relevant, the one returning from the signal handler to the
> trampoline. In this case it if it is a simple blr is already hinted
> correctly with 0b00 (I think it is the default BH for blr), that it is a
> subroutine return. We don't have control over the blr from the signal
> handler to change the hint to 0b01 anyway. So IIUC, the return address
> predictor failed before because the signal handler don't go back from
> the same place (+4) it was called, and it changes with the added bctrl.
> 
> I am CC'ing Nicholas and maybe he has more insight.

Prior to the kernel patch, if the signal handler code used blr BH=0b01
for returns that would indeed prevent the unbalance on processors which
implement it.

But you are right, as explained in Linux commit 0138ba5783ae, the blr is 
in the signal handler function so we can't change that.

> (I know that now this discussion is split in two places, the original
> thread Florian started and this on for the patch. Not sure where best to
> continue this)

linuxppc-dev doesn't mind responsible cross posts to other lists,
hopefully libc-alpha is too.

Thanks,
Nick