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

+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:

quoted

On 1/26/21 8:12 AM, Florian Weimer via Libc-alpha wrote:

quoted

* Raoni Fassina Firmino:

quoted

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