> Date: Sat, 14 Mar 2020 23:33:37 -0400
> From: George Koehler <kern...@gmail.com>
>
> Hello tech@ list!
>
> With this diff, it becomes possible to build macppc kernel with
> base-clang. The default compiler for macppc is base-gcc.
>
> rgc <rgci...@disroot.org> mailed the ppc@ list to report problems with
> clang kernel, and got a working kernel with clang -Oz (to stop clang
> inlining some functions in openfirm.c) and my modified ppc_mftb().
> In this diff, I use __attribute__((noinline)) so I don't need -Oz.
> I built my kernel with
>
> # clang CC=clang COMPILER_VERSION=clang
>
> because someone has already prepared the Makefile for macppc to pass
> different flags when COMPILER_VERSION is clang. I booted my clang
> kernel and it worked well enough to build another kernel with the
> same diff, but with gcc.
>
> In the second half of the diff, I change ppc_mftb(), because
> "mftb %0+1" doesn't always work in clang. For example, clang can
> put "=r"(tb) in register pair r27:r29, but 27+1 isn't 29. I don't
> know the syntax for the 2nd register of a pair, so I instead split
> "=r"(tb) into 2 variables. We use ppc_mftb() as timecounter.
>
> (gcc and clang emit different machine code for "mftbu" and "mftb", but
> both forms work for me. "objdump -dlr obj/clock.o" ppc_mftb() would
> show "mftb" from gcc or "mfspr" from clang. Power ISA 2.03 says that
> mfspr time base can't work on 601 or POWER3. A few Old World Mac
> models had PowerPC 601, but we only run on New World models.)
>
> Is the ppc_mftb() change by itself OK to commit?
I had a look at what NetBSD does, and they use '%L0' instead of
'%0+1'. As far as I can tell this works on both gcc and clang. The
diff below produces a working kernel when building with gcc. Not yet
in a position to test a clang-built kernel myself yet. But if this
produces a working kernel with clang as well, I'd prefer this diff
instead of yours.
> I am less sure about the first half of the diff, the noinline part.
> I don't like the design of this code. These functions call ofw_stack()
> like
>
> noinline int
> OF_peer(int phandle)
> {
> static struct ... args = ...;
>
> ofw_stack();
> args.phandle = phandle;
> if (openfirmware(&args) == -1)
> return 0;
> return args.sibling;
> }
>
> but ofw_stack() is assembly code that changes the cpu's msr, moves the
> stack pointer %r1 to a different stack, copies the stack frame of
> OF_peer() to this new stack, and hijacks the saved return address so
> that, when OF_peer() returns, it switches back to the old stack and
> restores the msr. If clang inlines a function like OF_peer(), it no
> longer has its own stack frame, so the return-hijack stops working.
>
> We don't have retguard on powerpc, but if OF_peer() would use
> retguard to protect its return address, then the return-hijack might
> become impossible.
>
> Perhaps the code should look like
>
> msr = do_something_to_msr();
> args.phandle = phandle;
> if (openfirmware(&args) == -1)
> ret = 0;
> else
> ret = args.sibling;
> ppc_mtmsr(msr);
> return ret;
>
> and openfirmware() should do the stack-switching, but I have not yet
> tried to make such a change. (I suspect that we need the msr to
> disable interrupts and protect the static args.)
Interrupts have to be blocked while we're not on the normal kernel stack.
I'm still working out for myself how this all works. NetBSD's code
stull uses ofw_stack(), so looking at their code doesn't help us.
FreeBSD seems to solve this in quite a different way, but tries to
support many different variations of firmware.
Cheers,
Mark
Index: arch/powerpc/include/cpu.h
===================================================================
RCS file: /cvs/src/sys/arch/powerpc/include/cpu.h,v
retrieving revision 1.65
diff -u -p -r1.65 cpu.h
--- arch/powerpc/include/cpu.h 23 Mar 2019 05:27:53 -0000 1.65
+++ arch/powerpc/include/cpu.h 16 Mar 2020 11:30:42 -0000
@@ -336,7 +336,7 @@ ppc_mftb(void)
u_long scratch;
u_int64_t tb;
- __asm volatile ("1: mftbu %0; mftb %0+1; mftbu %1;"
+ __asm volatile ("1: mftbu %0; mftb %L0; mftbu %1;"
" cmpw 0,%0,%1; bne 1b" : "=r"(tb), "=r"(scratch));
return tb;
}
