Messages in this thread | | | Date | Mon, 25 Oct 2021 16:19:16 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH 2/4] arm64: implement support for static call trampolines |
| |
On Mon, Oct 25, 2021 at 04:08:37PM +0200, Ard Biesheuvel wrote: > On Mon, 25 Oct 2021 at 15:57, Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Mon, Oct 25, 2021 at 02:21:00PM +0200, Frederic Weisbecker wrote: > > > > > +#define __ARCH_DEFINE_STATIC_CALL_TRAMP(name, insn) \ > > > + asm(" .pushsection .static_call.text, \"ax\" \n" \ > > > + " .align 4 \n" \ > > > + " .globl " STATIC_CALL_TRAMP_STR(name) " \n" \ > > > + "0: .quad 0x0 \n" \ > > > + STATIC_CALL_TRAMP_STR(name) ": \n" \ > > > + " hint 34 /* BTI C */ \n" \ > > > + insn " \n" \ > > > + " ldr x16, 0b \n" \ > > > + " cbz x16, 1f \n" \ > > > + " br x16 \n" \ > > > + "1: ret \n" \ > > > + " .popsection \n") > >
> > OK, that's pretty magical... > > > > So you're writing the literal and the two instructions with 2 u64 > > stores. Relying on alignment to guarantee both are in a single page and > > that copy_to_kernel_nofault() selects u64 writes. > > > > To be honest, it just seemed tidier and less likely to produce weird > corner cases to put the literal and the patched insn in the smallest > possible power-of-2 aligned window, as it ensures that the D-side view > is always consistent. > > However, the actual fetch of the instruction could still produce a > stale value before the cache maintenance completes. > > > By unconditionally writing the literal, you avoid there ever being an > > stale value, which in turn avoids there being a race where you switch > > from 'J @func' relative addressing to 'NOP; do-literal-thing' and cross > > CPU execution gets the ordering inverted. > > > > Indeed. > > > Ooohh, but what if you go from !func to NOP. > > > > assuming: > > > > .literal = 0 > > BTI C > > RET > > > > Then > > > > CPU0 CPU1 > > > > [S] literal = func [I] NOP > > [S] insn[1] = NOP [L] x16 = literal (NULL) > > b x16 > > *BANG* > > > > Is that possible? (total lack of memory ordering etc..) > > > > The CBZ will branch to the RET instruction if x16 == 0x0, so this > should not happen.
Oooh, I missed that :/ I was about to suggest writing the address of a bare 'ret' trampoline instead of NULL into the literal.
> > On IRC you just alluded to the fact that this relies on it all being in > > a single cacheline (i-fetch windows don't need to be cacheline sized, > > but provided they're at least 16 bytes, this should still work given the > > alignment). > > > > But is I$ and D$ coherent? One load is through I-fetch, the other is a > > regular D-fetch. > > > > However, Will has previously expressed reluctance to rely on such > > things. > > > > No they are not. That is why the CBZ is there. So the only issue we > might see is where the branch instruction is out of sync with the > literal, and so we may call the old function while switching to the > new one and the I-cache maintenance hasn't completed yet.
OK, agreed. Perhaps put in a comment to explain some of this though. The next poor sod trying to untangle this code is sure to have a question or two :-)
| |